Richard W.M. Jones
2023-Aug-31 08:50 UTC
[Libguestfs] [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
Laszlo Ersek
2023-Aug-31 10:19 UTC
[Libguestfs] [PATCH nbdkit] sh: In pwrite, allow scripts to ignore stdin
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 > +'
Eric Blake
2023-Aug-31 12:59 UTC
[Libguestfs] [PATCH nbdkit] sh: In pwrite, allow scripts to ignore stdin
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. > + passI 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
Apparently Analagous Threads
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data
- [nbdkit PATCH] sh: Allow pwrite to not consume all data