[libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Libvirtd has set SIGPIPE to ignored, and virFork resets all signal handlers to the defaults. But child process may write logs to stderr/stdout, that may generate SIGPIPE if journald has stopped. So block SIGPIPE in virFork, and unblock it before execve. Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn> --- v1 patch: https://www.redhat.com/archives/libvir-list/2019-October/msg00720.html Changes in v2: - use pthread_sigmask to block SIGPIPE Changes in v3: - pass SIG_UNBLOCK(not SIG_SETMASK) to pthread_sigmask when unlock SIGPIPE --- src/util/vircommand.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 79e1e87..bd3a582 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -321,6 +321,15 @@ virFork(void) virDispatchError(NULL); _exit(EXIT_CANCELED); } + + sigemptyset(&newmask); + sigaddset(&newmask, SIGPIPE); + if (pthread_sigmask(SIG_BLOCK, &newmask, NULL) != 0) { + virReportSystemError(errno, "%s", _("cannot block SIGPIPE")); + virDispatchError(NULL); + _exit(EXIT_CANCELED); + } + } return pid; } @@ -553,6 +562,7 @@ virExec(virCommandPtr cmd) struct sigaction waxon, waxoff; VIR_AUTOFREE(gid_t *) groups = NULL; int ngroups; + sigset_t set; if (cmd->args[0][0] != '/') { if (!(binary = binarystr = virFindFileInPath(cmd->args[0]))) { @@ -792,6 +802,13 @@ virExec(virCommandPtr cmd) /* Close logging again to ensure no FDs leak to child */ virLogReset(); + sigemptyset(&set); + sigaddset(&set, SIGPIPE); + if (pthread_sigmask(SIG_UNBLOCK, &set, NULL) != 0) { + virReportSystemError(errno, "%s", _("cannot unblock SIGPIPE")); + goto fork_error; + } + if (cmd->env) execve(binary, cmd->args, cmd->env); else -- 1.8.3.1
On 10/15/19 9:25 PM, Wang Yechao wrote:
Libvirtd has set SIGPIPE to ignored, and virFork resets all signal handlers to the defaults. But child process may write logs to stderr/stdout, that may generate SIGPIPE if journald has stopped.
So block SIGPIPE in virFork, and unblock it before execve.
How does that help? If writing to stderr hits EOF, it will queue up a SIGPIPE to be delivered as soon as you unblock SIGPIPE. The correct fix is to not unblock SIGPIPE until after any point at which you would be performing I/O that must not fail due to SIGPIPE, rather than messing with signal masks to just delay when SIGPIPE is delivered. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 10/15/19 9:43 PM, Eric Blake wrote:
On 10/15/19 9:25 PM, Wang Yechao wrote:
Libvirtd has set SIGPIPE to ignored, and virFork resets all signal handlers to the defaults. But child process may write logs to stderr/stdout, that may generate SIGPIPE if journald has stopped.
So block SIGPIPE in virFork, and unblock it before execve.
How does that help? If writing to stderr hits EOF, it will queue up a SIGPIPE to be delivered as soon as you unblock SIGPIPE.
The correct fix is to not unblock SIGPIPE until after any point at which you would be performing I/O that must not fail due to SIGPIPE, rather than messing with signal masks to just delay when SIGPIPE is delivered.
Let's word this better: The correct fix is to keep SIGPIPE ignored until after any point at which you would be performing I/O that must not cause SIGPIPE, reverting SIGPIPE back to SIG_DFL at the last possible moment. Messing with signal masks merely delays when SIGPIPE is delivered, rather than avoiding it. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Tue, Oct 15, 2019 at 09:59:46PM -0500, Eric Blake wrote:
On 10/15/19 9:43 PM, Eric Blake wrote:
On 10/15/19 9:25 PM, Wang Yechao wrote:
Libvirtd has set SIGPIPE to ignored, and virFork resets all signal handlers to the defaults. But child process may write logs to stderr/stdout, that may generate SIGPIPE if journald has stopped.
So block SIGPIPE in virFork, and unblock it before execve.
How does that help? If writing to stderr hits EOF, it will queue up a SIGPIPE to be delivered as soon as you unblock SIGPIPE.
The correct fix is to not unblock SIGPIPE until after any point at which you would be performing I/O that must not fail due to SIGPIPE, rather than messing with signal masks to just delay when SIGPIPE is delivered.
Let's word this better:
The correct fix is to keep SIGPIPE ignored until after any point at which you would be performing I/O that must not cause SIGPIPE, reverting SIGPIPE back to SIG_DFL at the last possible moment. Messing with signal masks merely delays when SIGPIPE is delivered, rather than avoiding it.
The challenge here is that we're in between fork + execve and want signal handlers back to their defaults at time of execve. If we set SIGPIPE to SIG_IGN and then execve() will that get reset back to SIG_DFL automatically ? Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:
The challenge here is that we're in between fork + execve and want signal handlers back to their defaults at time of execve.
If we set SIGPIPE to SIG_IGN and then execve() will that get reset back to SIG_DFL automatically ?
Sadly, no. execve() does not change whether a signal is ignored or masked (ignored is more common - a number of CI systems have had issues where the child inherits SIGPIPE ignored because the parent forgot to reset it, but the child wasn't expecting it; but inheriting a signal masked is also a real issue), with the lone exception of SIGCHLD. However, execve() _does_ change a signal that is being caught in the parent into SIG_DFL post-exec. That does mean, however, that it is viable to install a no-op SIGPIPE handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired), then post-exec the new process will have SIG_DFL. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Oct 16, 2019 at 06:50:33AM -0500, Eric Blake wrote:
On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:
The challenge here is that we're in between fork + execve and want signal handlers back to their defaults at time of execve.
If we set SIGPIPE to SIG_IGN and then execve() will that get reset back to SIG_DFL automatically ?
Sadly, no. execve() does not change whether a signal is ignored or masked (ignored is more common - a number of CI systems have had issues where the child inherits SIGPIPE ignored because the parent forgot to reset it, but the child wasn't expecting it; but inheriting a signal masked is also a real issue), with the lone exception of SIGCHLD. However, execve() _does_ change a signal that is being caught in the parent into SIG_DFL post-exec.
That does mean, however, that it is viable to install a no-op SIGPIPE handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired), then post-exec the new process will have SIG_DFL.
Yeah, that's workable. So we need virFork() to install a dummy SIGPIPE handler function that is a no-op, *before* it unmasks signals. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 10/16/19 9:04 AM, Daniel P. Berrangé wrote:
On Wed, Oct 16, 2019 at 06:50:33AM -0500, Eric Blake wrote:
On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:
The challenge here is that we're in between fork + execve and want signal handlers back to their defaults at time of execve.
If we set SIGPIPE to SIG_IGN and then execve() will that get reset back to SIG_DFL automatically ?
Sadly, no. execve() does not change whether a signal is ignored or masked (ignored is more common - a number of CI systems have had issues where the child inherits SIGPIPE ignored because the parent forgot to reset it, but the child wasn't expecting it; but inheriting a signal masked is also a real issue), with the lone exception of SIGCHLD. However, execve() _does_ change a signal that is being caught in the parent into SIG_DFL post-exec.
That does mean, however, that it is viable to install a no-op SIGPIPE handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired), then post-exec the new process will have SIG_DFL.
Yeah, that's workable.
So we need virFork() to install a dummy SIGPIPE handler function that is a no-op, *before* it unmasks signals.
Why mask signals at all? You either mask the signal before I/O, install the dummy handler, then unmask (and any intermediate SIGPIPE is now ignored by no-op), or you can merely install the dummy handler before I/O (any SIGPIPE is ignored by no-op). That is, by the time you identify a a safe place to install a mask (ie. no I/O between fork() and that point, but where there will be potential I/O between that point and exec), with plans to release it later, that same place is just as good for changing from SIG_IGN to a no-op handler without messing with masks. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Oct 16, 2019 at 09:33:59AM -0500, Eric Blake wrote:
On 10/16/19 9:04 AM, Daniel P. Berrangé wrote:
On Wed, Oct 16, 2019 at 06:50:33AM -0500, Eric Blake wrote:
On 10/16/19 4:02 AM, Daniel P. Berrangé wrote:
The challenge here is that we're in between fork + execve and want signal handlers back to their defaults at time of execve.
If we set SIGPIPE to SIG_IGN and then execve() will that get reset back to SIG_DFL automatically ?
Sadly, no. execve() does not change whether a signal is ignored or masked (ignored is more common - a number of CI systems have had issues where the child inherits SIGPIPE ignored because the parent forgot to reset it, but the child wasn't expecting it; but inheriting a signal masked is also a real issue), with the lone exception of SIGCHLD. However, execve() _does_ change a signal that is being caught in the parent into SIG_DFL post-exec.
That does mean, however, that it is viable to install a no-op SIGPIPE handler (SIGPIPE is generated but ignored, I/O gets the EPIPE as desired), then post-exec the new process will have SIG_DFL.
Yeah, that's workable.
So we need virFork() to install a dummy SIGPIPE handler function that is a no-op, *before* it unmasks signals.
Why mask signals at all? You either mask the signal before I/O, install the dummy handler, then unmask (and any intermediate SIGPIPE is now ignored by no-op), or you can merely install the dummy handler before I/O (any SIGPIPE is ignored by no-op). That is, by the time you identify a a safe place to install a mask (ie. no I/O between fork() and that point, but where there will be potential I/O between that point and exec), with plans to release it later, that same place is just as good for changing from SIG_IGN to a no-op handler without messing with masks.
The fork'd child process inherits all signal handlers from the parent. These handlers may no longer be valid since we are mass-closing all file descriptors. We thus need to kill off all the signal handlers in the forkd child. There's a window between fork & sigaction where signals can still get delivered to the child & run the undesirable handlers. So we must mask all signals immediately before fork, only unmasking them after we have set all signal handlers back to their defaults. We just need to set SIGPIPE to a dummy no-op handler before this unmask Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
The fork'd child process inherits all signal handlers from the parent. These handlers may no longer be valid since we are mass-closing all file descriptors. We thus need to kill off all the signal handlers in the forkd child.
There's a window between fork & sigaction where signals can still get delivered to the child & run the undesirable handlers. So we must mask all signals immediately before fork, only unmasking them after we have set all signal handlers back to their defaults.
We just need to set SIGPIPE to a dummy no-op handler before this unmask
If do that, i think the SIGPIPE ignored code before running hooks in virExec() can be removed. --- Best wishes Wang Yechao
participants (4)
-
Daniel P. Berrangé -
Eric Blake -
Wang Yechao -
wang.yechao255＠zte.com.cn