Libguestfs - Aug 2019 - [nbdkit PATCH 2/3] sh: Avoid setenv after fork

I'm pushing the first one as blatantly obvious.

The second one is also simple enough, but not enough of a bug for me
to push tonight.

The third is something I noticed while working on sh, but is really
more about docs vs. plugins in general.  There, we could either change
the code to match the docs (breaking backwards behavior for a plugin
that set .errno_is_preserved=2) [what my patch did], or change the
docs to match the code (mention that any non-zero value will do).
Preference?

Eric Blake (3):
  sh: Fix flags when none are present
  sh: Avoid setenv after fork
  plugins: Match docs for .errno_is_preserved

 docs/nbdkit-plugin.pod | 16 ++++++++++++++++
 plugins/sh/call.c      |  3 ---
 plugins/sh/sh.c        |  8 ++++++++
 server/plugins.c       |  2 +-
 tests/test-shell.sh    | 15 +++++++++++++++
 5 files changed, 40 insertions(+), 4 deletions(-)

-- 
2.20.1

fbuf was used uninitialized, which meant we printed garbage data from
the heap when flags was 0.  Update the test to prevent regressions.

Fixes: bcac9c40
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/sh/sh.c     |  2 ++
 tests/test-shell.sh | 15 +++++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index aeb01918..737c38cf 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -453,6 +453,8 @@ flags_string (uint32_t flags, char *buf, size_t len)
 {
   bool comma = false;

+  buf[0] = '\0';
+
   if (flags & NBDKIT_FLAG_FUA)
     flag_append ("fua", &comma, &buf, &len);

diff --git a/tests/test-shell.sh b/tests/test-shell.sh
index a744b275..53c0b943 100755
--- a/tests/test-shell.sh
+++ b/tests/test-shell.sh
@@ -42,6 +42,11 @@ case "$1" in
         ;;

     pwrite)
+	case $5 in
+	    '' | fua) ;;
+	    *) 	echo "garbage flags: '$5'" >&2
+		exit 1;
+	esac
         dd oflag=seek_bytes conv=notrunc seek=$4 of=$f || exit 1
         ;;
     can_write)
@@ -54,6 +59,11 @@ case "$1" in
         ;;

     trim)
+	case $5 in
+	    '' | fua) ;;
+	    *) 	echo "garbage flags: '$5'" >&2
+		exit 1;
+	esac
         fallocate -p -o $4 -l $3 -n $f
         ;;
     can_trim)
@@ -62,6 +72,11 @@ case "$1" in
         ;;

     zero)
+	case $5 in
+	    '' | fua | may_trim | fua,may_trim ) ;;
+	    *) 	echo "garbage flags: '$5'" >&2
+		exit 1;
+	esac
         fallocate -z -o $4 -l $3 -n $f
         ;;
     can_zero)
-- 
2.20.1

setenv() is not async-signal-safe and as such should not be used
between fork/exec of a multi-threaded app: if one thread is
manipulating the current environment (which may entail obtaining a
malloc() mutex) when another thread calls fork(), the resulting
child's attempt to use setenv() could deadlock or see a broken environ
because the thread owning the lock no longer exists to release it.
Besides, it is more efficient to update the environment exactly once
in the parent, rather than after every fork().

While at it, check for (unlikely) failure of setenv.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 plugins/sh/call.c | 3 ---
 plugins/sh/sh.c   | 6 ++++++
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 871de5c6..9b8b48e2 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -127,9 +127,6 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin */
     /* 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);
-
     execvp (argv[0], (char **) argv);
     perror (argv[0]);
     _exit (EXIT_FAILURE);
diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
index 737c38cf..e3d3c2f1 100644
--- a/plugins/sh/sh.c
+++ b/plugins/sh/sh.c
@@ -60,6 +60,12 @@ sh_load (void)
     nbdkit_error ("mkdtemp: /tmp: %m");
     exit (EXIT_FAILURE);
   }
+  /* Set $tmpdir for the script. */
+  if (setenv ("tmpdir", tmpdir, 1) == -1) {
+    nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
+    exit (EXIT_FAILURE);
+  }
+
   nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
 }

-- 
2.20.1

Ever since commit 69ae137f, the docs claimed that we check
.errno_is_preserved == 1, but the code has checked for != 0.
Furthermore, the mention of .errno_is_preserved in the docs was rather
hidden; a new section will make it easier to add future knobs that
likewise affect the plugin as a whole and not an individual
connection.

Fixes: 69ae137f
Signed-off-by: Eric Blake <eblake@redhat.com>
---
 docs/nbdkit-plugin.pod | 16 ++++++++++++++++
 server/plugins.c       |  2 +-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod
index c764f8b0..d778d6af 100644
--- a/docs/nbdkit-plugin.pod
+++ b/docs/nbdkit-plugin.pod
@@ -915,6 +915,22 @@ If there is an error, C<.cache> should call
C<nbdkit_error> with an
 error message, and C<nbdkit_set_error> to record an appropriate error
 (unless C<errno> is sufficient), then return C<-1>.

+=head1 OTHER FIELDS
+
+The plugin struct also contains an integer field used as a
+boolean in C code, but unlikely to be exposed in other language
+bindings:
+
+=over 4
+
+=item C<.errno_is_preserved>
+
+This defaults to 0; if non-zero, nbdkit can reliably use the value of
+C<errno> when a callback reports failure, rather than the plugin
+having to call C<nbdkit_set_error>.
+
+=back
+
 =head1 THREADS

 Each nbdkit plugin must declare its maximum thread safety model by
diff --git a/server/plugins.c b/server/plugins.c
index 8eed2dd8..7da2329e 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -492,7 +492,7 @@ get_error (struct backend_plugin *p)
 {
   int ret = threadlocal_get_error ();

-  if (!ret && p->plugin.errno_is_preserved)
+  if (!ret && p->plugin.errno_is_preserved == 1)
     ret = errno;
   return ret ? ret : EIO;
 }
-- 
2.20.1

On Thu, Aug 01, 2019 at 10:42:58PM -0500, Eric Blake
wrote:
> setenv() is not async-signal-safe and as such should not be used
> between fork/exec of a multi-threaded app: if one thread is
> manipulating the current environment (which may entail obtaining a
> malloc() mutex) when another thread calls fork(), the resulting
> child's attempt to use setenv() could deadlock or see a broken environ
> because the thread owning the lock no longer exists to release it.
> Besides, it is more efficient to update the environment exactly once
> in the parent, rather than after every fork().
> 
> While at it, check for (unlikely) failure of setenv.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>
> ---
>  plugins/sh/call.c | 3 ---
>  plugins/sh/sh.c   | 6 ++++++
>  2 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/plugins/sh/call.c b/plugins/sh/call.c
> index 871de5c6..9b8b48e2 100644
> --- a/plugins/sh/call.c
> +++ b/plugins/sh/call.c
> @@ -127,9 +127,6 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
stdin */
>      /* 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);
> -
>      execvp (argv[0], (char **) argv);
>      perror (argv[0]);
>      _exit (EXIT_FAILURE);
> diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c
> index 737c38cf..e3d3c2f1 100644
> --- a/plugins/sh/sh.c
> +++ b/plugins/sh/sh.c
> @@ -60,6 +60,12 @@ sh_load (void)
>      nbdkit_error ("mkdtemp: /tmp: %m");
>      exit (EXIT_FAILURE);
>    }
> +  /* Set $tmpdir for the script. */
> +  if (setenv ("tmpdir", tmpdir, 1) == -1) {
> +    nbdkit_error ("setenv: tmpdir=%s: %m", tmpdir);
> +    exit (EXIT_FAILURE);
> +  }
> +
>    nbdkit_debug ("sh: load: tmpdir: %s", tmpdir);
>  }
This sets $tmpdir in the whole nbdkit process ... which may or may not
be a problem.  It's probably not a problem in reality, but I wonder if
it's better to to use ‘execvpe’ (or more portably but more difficult
to use ‘execve’) to set the environment in the exec call?  I guess
we'd have to copy and extend environ in the parent since we want to
pass existing environment variables through.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html

So the series is fine, ACK.  I made a comment about an alternate way
to implement patch 2, but I don't know if it's better or not.

Thanks,

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html