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

Wang Yechao posted 1 patch 34 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1573172715-15369-1-git-send-email-wang.yechao255@zte.com.cn
src/util/vircommand.c | 32 ++++++++++----------------------
1 file changed, 10 insertions(+), 22 deletions(-)

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

Posted by Wang Yechao 34 weeks 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 the handler will get reset to SIG_DFL when execve()
runs. Now we can delete sigaction() call entirely in virExec().

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

Changes in v6:
 - add a comment and delete sigaction() call entirely in virExec
---
 src/util/vircommand.c | 32 ++++++++++----------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 93b3dd2..8b10253 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,14 @@ virFork(void)
             ignore_value(sigaction(i, &sig_action, NULL));
         }
 
+        /* 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 +560,6 @@ virExec(virCommandPtr cmd)
     g_autofree char *binarystr = NULL;
     const char *binary = NULL;
     int ret;
-    struct sigaction waxon, waxoff;
     g_autofree gid_t *groups = NULL;
     int ngroups;
 
@@ -718,21 +727,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,12 +777,6 @@ virExec(virCommandPtr cmd)
     if (virCommandHandshakeChild(cmd) < 0)
        goto fork_error;
 
-    if (sigaction(SIGPIPE, &waxon, NULL) < 0) {
-        virReportSystemError(errno, "%s",
-                             _("Could not re-enable SIGPIPE"));
-        goto fork_error;
-    }
-
     /* Close logging again to ensure no FDs leak to child */
     virLogReset();
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

Posted by Daniel P. Berrangé 34 weeks ago
On Fri, Nov 08, 2019 at 08:25:15AM +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 the handler will get reset to SIG_DFL when execve()
> runs. Now we can delete sigaction() call entirely in virExec().
> 
> 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
> 
> Changes in v6:
>  - add a comment and delete sigaction() call entirely in virExec
> ---
>  src/util/vircommand.c | 32 ++++++++++----------------------
>  1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/src/util/vircommand.c b/src/util/vircommand.c
> index 93b3dd2..8b10253 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) {}

'make syntax-check' didn't like {} on the same line like this.

I've fixed that trivial style issue and pushed to git.


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