1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process

Wang Yechao posted 1 patch 4 years, 6 months ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1571192740-31219-1-git-send-email-wang.yechao255@zte.com.cn
src/util/vircommand.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
[libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Wang Yechao 4 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Eric Blake 4 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Eric Blake 4 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Daniel P. Berrangé 4 years, 6 months ago 
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Eric Blake 4 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Daniel P. Berrangé 4 years, 6 months ago 
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Eric Blake 4 years, 6 months ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v3] util: Block SIGPIPE until execve in child process
Posted by Daniel P. Berrangé 4 years, 6 months ago 
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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt][PATCH v3] util: Block SIGPIPE until execve in childprocess
Posted by wang.yechao255@zte.com.cn 4 years, 6 months ago 
> 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--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.