Libguestfs - Dec 2018 - [nbdkit PATCH] sh: Don't let child inherit SIGPIPE ignored

While nbdkit itself must run with SIGPIPE ignored, many applications
expect to inherit SIGPIPE in the default state. What's worse, POSIX
states that a non-interactive shell script cannot use 'trap' to
undo an inherited SIG_IGN on SIGPIPE.  I have seen several bug
reports over the years of something that works for a developer but
fails under a CI environment, where the root cause was the CI
leaking an ignored SIGPIPE into the file under test.

Testing this proved to be the more interesting part of the patch.
'yes' is probably the easiest way to generate lots of data and
which behaves differently if SIGPIPE is ignored, but I'm not
certain if coreutils is always installed on BSD machines.  Also
fix a missing redirect to stderr in the probe for $tmpdir.

Signed-off-by: Eric Blake <eblake at redhat.com>
---
 plugins/sh/call.c   |  3 +++
 tests/test-shell.sh | 14 +++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 9b3eca8..42923da 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -121,6 +121,9 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
     close (out_fd[1]);
     close (err_fd[1]);

+    /* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN */
+    signal (SIGPIPE, SIG_DFL);
+
     /* Set $tmpdir for the script. */
     setenv ("tmpdir", tmpdir, 1);

diff --git a/tests/test-shell.sh b/tests/test-shell.sh
index ef438ec..eabd4fe 100755
--- a/tests/test-shell.sh
+++ b/tests/test-shell.sh
@@ -11,10 +11,22 @@ fi

 # nbdkit is supposed to set $tmpdir.  If it doesn't, it's an error.
 if [ ! -d $tmpdir ]; then
-    echo "\$tmpdir was not set"
+    echo "\$tmpdir was not set" >&2
     exit 1
 fi

+# Check that SIGPIPE is not ignored unless we want it that way.
+if (type yes) >/dev/null 2>&1; then
+    ignored=$(trap "" 13;
+	      ({ yes; echo $? >&3; } | head -c1) 3>&1 >/dev/null
2>&1)
+    default=$(trap - 13;
+	      ({ yes; echo $? >&3; } | head -c1) 3>&1 >/dev/null
2>&1)
+    if [ $ignored = $default ]; then
+	echo "SIGPIPE was inherited ignored" >&2
+	exit 1;
+    fi
+fi
+
 case "$1" in
     open)
         echo handle
-- 
2.17.2

On Sat, Dec 01, 2018 at 04:16:12PM -0600, Eric Blake
wrote:
> While nbdkit itself must run with SIGPIPE ignored, many applications
> expect to inherit SIGPIPE in the default state. What's worse, POSIX
> states that a non-interactive shell script cannot use 'trap' to
> undo an inherited SIG_IGN on SIGPIPE.  I have seen several bug
> reports over the years of something that works for a developer but
> fails under a CI environment, where the root cause was the CI
> leaking an ignored SIGPIPE into the file under test.
> 
> Testing this proved to be the more interesting part of the patch.
> 'yes' is probably the easiest way to generate lots of data and
> which behaves differently if SIGPIPE is ignored, but I'm not
> certain if coreutils is always installed on BSD machines.  Also
> fix a missing redirect to stderr in the probe for $tmpdir.
> 
> Signed-off-by: Eric Blake <eblake at redhat.com>
> ---
>  plugins/sh/call.c   |  3 +++
>  tests/test-shell.sh | 14 +++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/plugins/sh/call.c b/plugins/sh/call.c
> index 9b3eca8..42923da 100644
> --- a/plugins/sh/call.c
> +++ b/plugins/sh/call.c
> @@ -121,6 +121,9 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
stdin */
>      close (out_fd[1]);
>      close (err_fd[1]);
> 
> +    /* Restore SIGPIPE back to SIG_DFL, since shell can't undo SIG_IGN
*/
> +    signal (SIGPIPE, SIG_DFL);
> +
>      /* Set $tmpdir for the script. */
>      setenv ("tmpdir", tmpdir, 1);
> 
> diff --git a/tests/test-shell.sh b/tests/test-shell.sh
> index ef438ec..eabd4fe 100755
> --- a/tests/test-shell.sh
> +++ b/tests/test-shell.sh
> @@ -11,10 +11,22 @@ fi
> 
>  # nbdkit is supposed to set $tmpdir.  If it doesn't, it's an
error.
>  if [ ! -d $tmpdir ]; then
> -    echo "\$tmpdir was not set"
> +    echo "\$tmpdir was not set" >&2
>      exit 1
>  fi
> 
> +# Check that SIGPIPE is not ignored unless we want it that way.
> +if (type yes) >/dev/null 2>&1; then
> +    ignored=$(trap "" 13;
> +	      ({ yes; echo $? >&3; } | head -c1) 3>&1 >/dev/null
2>&1)
> +    default=$(trap - 13;
> +	      ({ yes; echo $? >&3; } | head -c1) 3>&1 >/dev/null
2>&1)
> +    if [ $ignored = $default ]; then
> +	echo "SIGPIPE was inherited ignored" >&2
> +	exit 1;
> +    fi
> +fi
> +
>  case "$1" in
>      open)
>          echo handle
> -- 
> 2.17.2
ACK

SIGPIPE leakage is always a nasty problem.  I've noticed this one
before in Python, and we had a similar problem (SIGCHLD leaking)
recently found in collectd.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v