1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH v5] util: Set SIGPIPE to a no-op handler in virFork

Wang Yechao posted 1 patch 4 years, 6 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1571392510-17190-1-git-send-email-wang.yechao255@zte.com.cn
There is a newer version of this series
src/util/vircommand.c | 27 ++++++++++-----------------
1 file changed, 10 insertions(+), 17 deletions(-)
[libvirt] [PATCH v5] util: Set SIGPIPE to a no-op handler in virFork
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 set SIGPIPE to a dummy no-op handler before unmask signals in
virFork. And there is no need to set SIGPIPE ignored before running
hooks in virExec. At last, set SIGPIPE to defaults before execve.

Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn>
---
v3 patch:
https://www.redhat.com/archives/libvir-list/2019-October/msg00934.html

Changes in v4:
 -  don't block SIGPIPE, ignore it when invoke VIR_FORCE_CLOSE and virCommandMassClose

Changes in v5:
 - chang from SIG_IGN to a no-op handler in child process
---
 src/util/vircommand.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 93b3dd2..c13739c 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -217,6 +217,8 @@ virCommandFDSet(virCommandPtr cmd,
 
 #ifndef WIN32
 
+static void virDummyHandler(int sig G_GNUC_UNUSED) {}
+
 /**
  * virFork:
  *
@@ -312,6 +314,9 @@ virFork(void)
             ignore_value(sigaction(i, &sig_action, NULL));
         }
 
+        sig_action.sa_handler = virDummyHandler;
+        ignore_value(sigaction(SIGPIPE, &sig_action, NULL));
+
         /* Unmask all signals in child, since we've no idea what the
          * caller's done with their signal mask and don't want to
          * propagate that to children */
@@ -550,7 +555,7 @@ virExec(virCommandPtr cmd)
     g_autofree char *binarystr = NULL;
     const char *binary = NULL;
     int ret;
-    struct sigaction waxon, waxoff;
+    struct sigaction sig_action;
     g_autofree gid_t *groups = NULL;
     int ngroups;
 
@@ -718,21 +723,6 @@ virExec(virCommandPtr cmd)
         }
     }
 
-    /* virFork reset all signal handlers to the defaults.
-     * This is good for the child process, but our hook
-     * risks running something that generates SIGPIPE,
-     * so we need to temporarily block that again
-     */
-    memset(&waxoff, 0, sizeof(waxoff));
-    waxoff.sa_handler = SIG_IGN;
-    sigemptyset(&waxoff.sa_mask);
-    memset(&waxon, 0, sizeof(waxon));
-    if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) {
-        virReportSystemError(errno, "%s",
-                             _("Could not disable SIGPIPE"));
-        goto fork_error;
-    }
-
     if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0)
         goto fork_error;
     if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0)
@@ -783,7 +773,10 @@ virExec(virCommandPtr cmd)
     if (virCommandHandshakeChild(cmd) < 0)
        goto fork_error;
 
-    if (sigaction(SIGPIPE, &waxon, NULL) < 0) {
+    memset(&sig_action, 0, sizeof(sig_action));
+    sig_action.sa_handler = SIG_DFL;
+    sigemptyset(&sig_action.sa_mask);
+    if (sigaction(SIGPIPE, &sig_action, NULL) < 0) {
         virReportSystemError(errno, "%s",
                              _("Could not re-enable SIGPIPE"));
         goto fork_error;
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v5] util: Set SIGPIPE to a no-op handler in virFork
Posted by Daniel P. Berrangé 4 years, 5 months ago 
On Fri, Oct 18, 2019 at 05:55:10PM +0800, 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 set SIGPIPE to a dummy no-op handler before unmask signals in
> virFork. And there is no need to set SIGPIPE ignored before running
> hooks in virExec. At last, set SIGPIPE to defaults before execve.
> 
> Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn>
> ---
> v3 patch:
> https://www.redhat.com/archives/libvir-list/2019-October/msg00934.html
> 
> Changes in v4:
>  -  don't block SIGPIPE, ignore it when invoke VIR_FORCE_CLOSE and virCommandMassClose
> 
> Changes in v5:
>  - chang from SIG_IGN to a no-op handler in child process
> ---
>  src/util/vircommand.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
> 
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 93b3dd2..c13739c 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -217,6 +217,8 @@ virCommandFDSet(virCommandPtr cmd,
>  
>  #ifndef WIN32
>  
> +static void virDummyHandler(int sig G_GNUC_UNUSED) {}
> +
>  /**
>   * virFork:
>   *
> @@ -312,6 +314,9 @@ virFork(void)
>              ignore_value(sigaction(i, &sig_action, NULL));
>          }
>

Put a comment here

    /* Code that runs between fork & execve might trigger
     * SIG_PIPE, so we must explicitly set that to a no-op
     * handler. This handler will get reset to SIG_DFL when
     * execve() runs
     */

> +        sig_action.sa_handler = virDummyHandler;
> +        ignore_value(sigaction(SIGPIPE, &sig_action, NULL));
> +
>          /* Unmask all signals in child, since we've no idea what the
>           * caller's done with their signal mask and don't want to
>           * propagate that to children */
> @@ -550,7 +555,7 @@ virExec(virCommandPtr cmd)
>      g_autofree char *binarystr = NULL;
>      const char *binary = NULL;
>      int ret;
> -    struct sigaction waxon, waxoff;
> +    struct sigaction sig_action;
>      g_autofree gid_t *groups = NULL;
>      int ngroups;
>  
> @@ -718,21 +723,6 @@ virExec(virCommandPtr cmd)
>          }
>      }
>  
> -    /* virFork reset all signal handlers to the defaults.
> -     * This is good for the child process, but our hook
> -     * risks running something that generates SIGPIPE,
> -     * so we need to temporarily block that again
> -     */
> -    memset(&waxoff, 0, sizeof(waxoff));
> -    waxoff.sa_handler = SIG_IGN;
> -    sigemptyset(&waxoff.sa_mask);
> -    memset(&waxon, 0, sizeof(waxon));
> -    if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("Could not disable SIGPIPE"));
> -        goto fork_error;
> -    }
> -
>      if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0)
>          goto fork_error;
>      if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0)



> @@ -783,7 +773,10 @@ virExec(virCommandPtr cmd)
>      if (virCommandHandshakeChild(cmd) < 0)
>         goto fork_error;
>  
> -    if (sigaction(SIGPIPE, &waxon, NULL) < 0) {
> +    memset(&sig_action, 0, sizeof(sig_action));
> +    sig_action.sa_handler = SIG_DFL;
> +    sigemptyset(&sig_action.sa_mask);
> +    if (sigaction(SIGPIPE, &sig_action, NULL) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Could not re-enable SIGPIPE"));
>          goto fork_error;

We can just delete this sigaction() call entirely. The execve() call
will purge the no-op handler function we registered in virFork(),
resetting it back to SIG_DFL, so no need todo it manually here.

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 v5] util: Set SIGPIPE to a no-op handler invirFork
Posted by wang.yechao255@zte.com.cn 4 years, 5 months ago 
> > @@ -783,7 +773,10 @@ virExec(virCommandPtr cmd)
> >      if (virCommandHandshakeChild(cmd) < 0)
> >         goto fork_error;
> >
> > -    if (sigaction(SIGPIPE, &waxon, NULL) < 0) {
> > +    memset(&sig_action, 0, sizeof(sig_action));
> > +    sig_action.sa_handler = SIG_DFL;
> > +    sigemptyset(&sig_action.sa_mask);
> > +    if (sigaction(SIGPIPE, &sig_action, NULL) < 0) {
> >          virReportSystemError(errno, "%s",
> >                               _("Could not re-enable SIGPIPE"));
> >          goto fork_error;
>
> We can just delete this sigaction() call entirely. The execve() call
> will purge the no-op handler function we registered in virFork(),
> resetting it back to SIG_DFL, so no need todo it manually here.

ok, i'll adjust the code and push another patch.
---
Best wishes
Wang Yechao--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt][PATCH v5] util: Set SIGPIPE to a no-op handler in virFork
Posted by wang.yechao255@zte.com.cn 4 years, 5 months ago 
ping.
> Subject: [PATCH v5] util: Set SIGPIPE to a no-op handler in virFork
>
> 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 set SIGPIPE to a dummy no-op handler before unmask signals in
> virFork. And there is no need to set SIGPIPE ignored before running
> hooks in virExec. At last, set SIGPIPE to defaults before execve.
>
> Signed-off-by: Wang Yechao <wang.yechao255@zte.com.cn>
> ---
> v3 patch:
> https://www.redhat.com/archives/libvir-list/2019-October/msg00934.html
>
> Changes in v4:
>  -  don't block SIGPIPE, ignore it when invoke VIR_FORCE_CLOSE and virCommandMassClose
>
> Changes in v5:
>  - chang from SIG_IGN to a no-op handler in child process
> ---
>  src/util/vircommand.c | 27 ++++++++++-----------------
>  1 file changed, 10 insertions(+), 17 deletions(-)
>
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 93b3dd2..c13739c 100644
> --- a/src/util/vircommand.c
> +++ b/src/util/vircommand.c
> @@ -217,6 +217,8 @@ virCommandFDSet(virCommandPtr cmd,
>
>  #ifndef WIN32
>
> +static void virDummyHandler(int sig G_GNUC_UNUSED) {}
> +
>  /**
>   * virFork:
>   *
> @@ -312,6 +314,9 @@ virFork(void)
>              ignore_value(sigaction(i, &sig_action, NULL));
>          }
>
> +        sig_action.sa_handler = virDummyHandler;
> +        ignore_value(sigaction(SIGPIPE, &sig_action, NULL));
> +
>          /* Unmask all signals in child, since we've no idea what the
>           * caller's done with their signal mask and don't want to
>           * propagate that to children */
> @@ -550,7 +555,7 @@ virExec(virCommandPtr cmd)
>      g_autofree char *binarystr = NULL;
>      const char *binary = NULL;
>      int ret;
> -    struct sigaction waxon, waxoff;
> +    struct sigaction sig_action;
>      g_autofree gid_t *groups = NULL;
>      int ngroups;
>
> @@ -718,21 +723,6 @@ virExec(virCommandPtr cmd)
>          }
>      }
>
> -    /* virFork reset all signal handlers to the defaults.
> -     * This is good for the child process, but our hook
> -     * risks running something that generates SIGPIPE,
> -     * so we need to temporarily block that again
> -     */
> -    memset(&waxoff, 0, sizeof(waxoff));
> -    waxoff.sa_handler = SIG_IGN;
> -    sigemptyset(&waxoff.sa_mask);
> -    memset(&waxon, 0, sizeof(waxon));
> -    if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) {
> -        virReportSystemError(errno, "%s",
> -                             _("Could not disable SIGPIPE"));
> -        goto fork_error;
> -    }
> -
>      if (virProcessSetMaxMemLock(0, cmd->maxMemLock) < 0)
>          goto fork_error;
>      if (virProcessSetMaxProcesses(0, cmd->maxProcesses) < 0)
> @@ -783,7 +773,10 @@ virExec(virCommandPtr cmd)
>      if (virCommandHandshakeChild(cmd) < 0)
>         goto fork_error;
>
> -    if (sigaction(SIGPIPE, &waxon, NULL) < 0) {
> +    memset(&sig_action, 0, sizeof(sig_action));
> +    sig_action.sa_handler = SIG_DFL;
> +    sigemptyset(&sig_action.sa_mask);
> +    if (sigaction(SIGPIPE, &sig_action, NULL) < 0) {
>          virReportSystemError(errno, "%s",
>                               _("Could not re-enable SIGPIPE"));
>          goto fork_error;

---
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.