5:19 a.m.
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(a)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
+'