Libguestfs - Aug 2023 - [PATCH nbdkit] sh: In pwrite, allow scripts to ignore stdin

See comment for explanation and
https://listman.redhat.com/archives/libguestfs/2023-August/032468.html
---
 tests/Makefile.am                    |  2 +
 plugins/sh/call.c                    | 33 +++++++-----
 tests/test-sh-pwrite-ignore-stdin.sh | 77 ++++++++++++++++++++++++++++
 3 files changed, 100 insertions(+), 12 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index d57eb01b8..e69893e0d 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -1325,11 +1325,13 @@ test-shell.img:
 TESTS += \
 	test-sh-errors.sh \
 	test-sh-extents.sh \
+	test-sh-pwrite-ignore-stdin.sh \
 	test-sh-tmpdir-leak.sh \
 	$(NULL)
 EXTRA_DIST += \
 	test-sh-errors.sh \
 	test-sh-extents.sh \
+	test-sh-pwrite-ignore-stdin.sh \
 	test-sh-tmpdir-leak.sh \
 	$(NULL)
 
diff --git a/plugins/sh/call.c b/plugins/sh/call.c
index 888c6459a..621465252 100644
--- a/plugins/sh/call.c
+++ b/plugins/sh/call.c
@@ -275,22 +275,31 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to stdin
(can be NULL) */
       r = write (pfds[0].fd, wbuf, wbuflen);
       if (r == -1) {
         if (errno == EPIPE) {
-          /* We tried to write to the script but it didn't consume
-           * the data.  Probably the script exited without reading
-           * from stdin.  This is an error in the script.
+          /* In nbdkit <= 1.35.11 we gave an error here, arguing that
+           * scripts must always consume or discard their full input
+           * when 'pwrite' is called.  Previously we had many cases
+           * where scripts forgot to discard the data on a path out of
+           * pwrite (such as an error or where the script is not
+           * interested in the data being written), resulting in
+           * intermittent test failures.
+           *
+           * It is valid for a script to ignore the written data
+           * (plenty of non-sh plugins do this), or for a script to be
+           * gradually processing the data, encounter an error and
+           * wish to exit immediately.  Therefore ignore this error.
            */
-          nbdkit_error ("%s: write to script failed because of a broken
pipe: "
-                        "this can happen if the script exits without
"
-                        "consuming stdin, which usually indicates a bug
"
-                        "in the script",
-                        argv0);
+          nbdkit_debug ("%s: write: %m (ignored)", argv0);
+          wbuflen = 0;          /* discard the rest */
         }
-        else
+        else {
           nbdkit_error ("%s: write: %m", argv0);
-        goto error;
+          goto error;
+        }
+      }
+      else {
+        wbuf += r;
+        wbuflen -= r;
       }
-      wbuf += r;
-      wbuflen -= r;
       /* After writing all the data we close the pipe so that
        * the reader on the other end doesn't wait for more.
        */
diff --git a/tests/test-sh-pwrite-ignore-stdin.sh
b/tests/test-sh-pwrite-ignore-stdin.sh
new file mode 100755
index 000000000..3448eca17
--- /dev/null
+++ b/tests/test-sh-pwrite-ignore-stdin.sh
@@ -0,0 +1,77 @@
+#!/usr/bin/env bash
+# nbdkit
+# Copyright Red Hat
+#
+# Redistribution and use in source and binary forms, with or without
+# modification, are permitted provided that the following conditions are
+# met:
+#
+# * Redistributions of source code must retain the above copyright
+# notice, this list of conditions and the following disclaimer.
+#
+# * Redistributions in binary form must reproduce the above copyright
+# notice, this list of conditions and the following disclaimer in the
+# documentation and/or other materials provided with the distribution.
+#
+# * Neither the name of Red Hat nor the names of its contributors may be
+# used to endorse or promote products derived from this software without
+# specific prior written permission.
+#
+# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+# SUCH DAMAGE.
+
+# Test that pwrite is allowed to ignore stdin (in nbdkit >= 1.36).
+
+source ./functions.sh
+set -e
+set -x
+
+requires_plugin sh
+requires_nbdsh_uri
+
+sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
+pid=sh-pwrite-ignore-stdin.pid
+files="$sock $pid"
+rm -f $files
+cleanup_fn rm -f $files
+
+start_nbdkit -P $pid -U $sock sh - <<'EOF'
+case "$1" in
+    can_write) echo 0 ;;
+    pwrite)
+        # Always ignore the input.  If the offset >= 32M return an error.
+        if [ $4 -ge 33554432 ]; then
+            echo 'ENOSPC Out of space' >&2
+            exit 1
+        fi
+        ;;
+    get_size)
+        echo 64M
+        ;;
+    pread)
+        ;;
+    *) exit 2 ;;
+esac
+EOF
+
+nbdsh -u "nbd+unix://?socket=$sock" -c '
+buf = bytearray(16*1024*1024)
+h.pwrite(buf, 0)
+
+try:
+    h.pwrite(buf, 32*1024*1024)
+    assert False
+except nbd.Error:
+    # Expect an error here.
+    pass
+'
-- 
2.41.0

On 8/31/23 10:50, Richard W.M. Jones wrote:
> See comment for explanation and
> https://listman.redhat.com/archives/libguestfs/2023-August/032468.html
> ---
>  tests/Makefile.am                    |  2 +
>  plugins/sh/call.c                    | 33 +++++++-----
>  tests/test-sh-pwrite-ignore-stdin.sh | 77 ++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index d57eb01b8..e69893e0d 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -1325,11 +1325,13 @@ test-shell.img:
>  TESTS += \
>  	test-sh-errors.sh \
>  	test-sh-extents.sh \
> +	test-sh-pwrite-ignore-stdin.sh \
>  	test-sh-tmpdir-leak.sh \
>  	$(NULL)
>  EXTRA_DIST += \
>  	test-sh-errors.sh \
>  	test-sh-extents.sh \
> +	test-sh-pwrite-ignore-stdin.sh \
>  	test-sh-tmpdir-leak.sh \
>  	$(NULL)
>  
> diff --git a/plugins/sh/call.c b/plugins/sh/call.c
> index 888c6459a..621465252 100644
> --- a/plugins/sh/call.c
> +++ b/plugins/sh/call.c
> @@ -275,22 +275,31 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
stdin (can be NULL) */
>        r = write (pfds[0].fd, wbuf, wbuflen);
>        if (r == -1) {
>          if (errno == EPIPE) {
> -          /* We tried to write to the script but it didn't consume
> -           * the data.  Probably the script exited without reading
> -           * from stdin.  This is an error in the script.
> +          /* In nbdkit <= 1.35.11 we gave an error here, arguing that
> +           * scripts must always consume or discard their full input
> +           * when 'pwrite' is called.  Previously we had many
cases
> +           * where scripts forgot to discard the data on a path out of
> +           * pwrite (such as an error or where the script is not
> +           * interested in the data being written), resulting in
> +           * intermittent test failures.
> +           *
> +           * It is valid for a script to ignore the written data
> +           * (plenty of non-sh plugins do this), or for a script to be
> +           * gradually processing the data, encounter an error and
> +           * wish to exit immediately.  Therefore ignore this error.
>             */
I'm not reviewing the code changes in detail, just asking for a comment
extension here:

Can you highlight that, "if a script fails to consume all input due to a
crash, we're going to catch that with waitpid() / WIFSIGNALED() below"?

Basically I'd like us to show that we *know* that we cover for the case when
an EPIPE might not be a conscious decision from the child script.


... Hmmm, let me check something:

  #!/bin/bash
  ulimit -f 1
  cat >f

$ ./script </dev/zero
./script: line 3: 18032 File size limit exceeded(core dumped) cat > f

$ echo $?
153

$ kill -l 153
XFSZ

Good!

(This example simulates a subprocess of the pwrite script crashing with SIGXFSZ,
due to exceeding the permitted file size limit, *and* the shell propagating that
crashing exit status up to nbdkit's sh plugin.)

Acked-by: Laszlo Ersek <lersek at redhat.com>

Laszlo

> -          nbdkit_error ("%s: write to script failed because of a
broken pipe: "
> -                        "this can happen if the script exits without
"
> -                        "consuming stdin, which usually indicates a
bug "
> -                        "in the script",
> -                        argv0);
> +          nbdkit_debug ("%s: write: %m (ignored)", argv0);
> +          wbuflen = 0;          /* discard the rest */
>          }
> -        else
> +        else {
>            nbdkit_error ("%s: write: %m", argv0);
> -        goto error;
> +          goto error;
> +        }
> +      }
> +      else {
> +        wbuf += r;
> +        wbuflen -= r;
>        }
> -      wbuf += r;
> -      wbuflen -= r;
>        /* After writing all the data we close the pipe so that
>         * the reader on the other end doesn't wait for more.
>         */
> diff --git a/tests/test-sh-pwrite-ignore-stdin.sh
b/tests/test-sh-pwrite-ignore-stdin.sh
> new file mode 100755
> index 000000000..3448eca17
> --- /dev/null
> +++ b/tests/test-sh-pwrite-ignore-stdin.sh
> @@ -0,0 +1,77 @@
> +#!/usr/bin/env bash
> +# nbdkit
> +# Copyright Red Hat
> +#
> +# Redistribution and use in source and binary forms, with or without
> +# modification, are permitted provided that the following conditions are
> +# met:
> +#
> +# * Redistributions of source code must retain the above copyright
> +# notice, this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +# notice, this list of conditions and the following disclaimer in the
> +# documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Red Hat nor the names of its contributors may be
> +# used to endorse or promote products derived from this software without
> +# specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
> +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
> +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
> +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
> +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
> +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
> +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> +# SUCH DAMAGE.
> +
> +# Test that pwrite is allowed to ignore stdin (in nbdkit >= 1.36).
> +
> +source ./functions.sh
> +set -e
> +set -x
> +
> +requires_plugin sh
> +requires_nbdsh_uri
> +
> +sock=$(mktemp -u /tmp/nbdkit-test-sock.XXXXXX)
> +pid=sh-pwrite-ignore-stdin.pid
> +files="$sock $pid"
> +rm -f $files
> +cleanup_fn rm -f $files
> +
> +start_nbdkit -P $pid -U $sock sh - <<'EOF'
> +case "$1" in
> +    can_write) echo 0 ;;
> +    pwrite)
> +        # Always ignore the input.  If the offset >= 32M return an
error.
> +        if [ $4 -ge 33554432 ]; then
> +            echo 'ENOSPC Out of space' >&2
> +            exit 1
> +        fi
> +        ;;
> +    get_size)
> +        echo 64M
> +        ;;
> +    pread)
> +        ;;
> +    *) exit 2 ;;
> +esac
> +EOF
> +
> +nbdsh -u "nbd+unix://?socket=$sock" -c '
> +buf = bytearray(16*1024*1024)
> +h.pwrite(buf, 0)
> +
> +try:
> +    h.pwrite(buf, 32*1024*1024)
> +    assert False
> +except nbd.Error:
> +    # Expect an error here.
> +    pass
> +'

On Thu, Aug 31, 2023 at 09:50:22AM +0100, Richard W.M. Jones
wrote:
> See comment for explanation and
> https://listman.redhat.com/archives/libguestfs/2023-August/032468.html
> ---
>  tests/Makefile.am                    |  2 +
>  plugins/sh/call.c                    | 33 +++++++-----
>  tests/test-sh-pwrite-ignore-stdin.sh | 77 ++++++++++++++++++++++++++++
>  3 files changed, 100 insertions(+), 12 deletions(-)
Having read the rest of the conversation, I now agree with you: no
matter when EPIPE happens, it is more important that we honor the exit
status rather than blindly turning the child process's refusal to
empty the pipe as a fatal error.
> +++ b/plugins/sh/call.c
> @@ -275,22 +275,31 @@ call3 (const char *wbuf, size_t wbuflen, /* sent to
stdin (can be NULL) */
>        r = write (pfds[0].fd, wbuf, wbuflen);
>        if (r == -1) {
>          if (errno == EPIPE) {
> -          /* We tried to write to the script but it didn't consume
> -           * the data.  Probably the script exited without reading
> -           * from stdin.  This is an error in the script.
> +          /* In nbdkit <= 1.35.11 we gave an error here, arguing that
> +           * scripts must always consume or discard their full input
> +           * when 'pwrite' is called.  Previously we had many
cases
> +           * where scripts forgot to discard the data on a path out of
> +           * pwrite (such as an error or where the script is not
> +           * interested in the data being written), resulting in
> +           * intermittent test failures.
> +           *
> +           * It is valid for a script to ignore the written data
> +           * (plenty of non-sh plugins do this), or for a script to be
> +           * gradually processing the data, encounter an error and
> +           * wish to exit immediately.  Therefore ignore this error.
I agree with Laszlo's observation that we may want to enhance this
comment to mention that we still obey the exit status (a plugin that
quit reading stdin because it was killed by a signal will show up as
an error and not a successful .pwrite).
>             */
> -          nbdkit_error ("%s: write to script failed because of a
broken pipe: "
> -                        "this can happen if the script exits without
"
> -                        "consuming stdin, which usually indicates a
bug "
> -                        "in the script",
> -                        argv0);
> +          nbdkit_debug ("%s: write: %m (ignored)", argv0);
> +          wbuflen = 0;          /* discard the rest */
>          }
> -        else
> +        else {
>            nbdkit_error ("%s: write: %m", argv0);
> -        goto error;
> +          goto error;
> +        }
Yes, moving the 'goto error' into the else branch is something I ended
up realizing I missed in my version of the patch, when I tried writing
a unit test.
> +      }
> +      else {
> +        wbuf += r;
> +        wbuflen -= r;
>        }
> -      wbuf += r;
> -      wbuflen -= r;
>        /* After writing all the data we close the pipe so that
>         * the reader on the other end doesn't wait for more.
>         */
This code change does what we need, and is less complex than my attempt.
> diff --git a/tests/test-sh-pwrite-ignore-stdin.sh
b/tests/test-sh-pwrite-ignore-stdin.sh
> new file mode 100755
> index 000000000..3448eca17
> --- /dev/null
> +++ b/tests/test-sh-pwrite-ignore-stdin.sh
> +
> +start_nbdkit -P $pid -U $sock sh - <<'EOF'
> +case "$1" in
> +    can_write) echo 0 ;;
> +    pwrite)
> +        # Always ignore the input.  If the offset >= 32M return an
error.
> +        if [ $4 -ge 33554432 ]; then
> +            echo 'ENOSPC Out of space' >&2
> +            exit 1
> +        fi
> +        ;;
In my testing, I could not reliably reproduce an EPIPE error, either
with your patch or mine, and whether or not I used your unit test or
my (unpublished) attempt at one (I had tried to 'exec 0</dev/null' to
forcefully close the pipe before the child process exits, but even
that still doesn't necessarily result in the parent process seeing
EPIPE - it really boils down to kernel timing of the window in the
parent between poll() saying readable vs. write() seeing a closed
pipe(); and the child does not have easy control over widening that
window).  With just the unit test but not the code change, I tried:

$ for i in `seq 100`; do make -C tests check
TESTS=test-sh-pwrite-ignore-stdin.sh || break; done

while running a heavy compile load on another project in parallel, and
the loop ended at i == 100 (meaning I did not hit EPIPE at all during
my 100 tests).  I do think that on a system under heavy enough load
(our CI system tends to be such a setup), you will eventually see the
EPIPE failures with enough iterations.  So the unit test is
worthwhile, even if it is not deterministic at demonstrating the
problem being solved.

Meanwhile, I like that your unit test covers the fact that we DO want
to allow the child process to conditionally return errors while not
fully reading stdin, which is somthing my atempt at a patch did not
do.

Overnight, I also thought about experimenting whether adding
O_NONBLOCK to the nbdkit's side of the pipe to the plugin would make
the EPIPE error more likely.  But as sh is not intended as performance
critical, and we aren't using epoll() with its edge-triggered
notifications, that was getting too invasive.

[https://eklitzke.org/blocking-io-nonblocking-io-and-epoll was a nice
read]
> +    get_size)
> +        echo 64M
> +        ;;
> +    pread)
> +        ;;
> +    *) exit 2 ;;
> +esac
> +EOF
> +
> +nbdsh -u "nbd+unix://?socket=$sock" -c '
> +buf = bytearray(16*1024*1024)
> +h.pwrite(buf, 0)
> +
> +try:
> +    h.pwrite(buf, 32*1024*1024)
> +    assert False
> +except nbd.Error:
> +    # Expect an error here.
> +    pass
I would prefer if we go one step further, and

import errno
...
except nbd.Error as ex:
    assert ex.errnum == errno.ENOSPC

to prove that we got the client's intended ENOSPC, rather than an
accidental nbdkit treating EPIPE writing to the plugin as an instant
EIO to the plugin (the pre-patch behavior).

With that change,

Reviewed-by: Eric Blake <eblake at redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org