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 <ler...@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
> +'
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
