Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 0/5] CVE-2022-0485 fix: nbdcopy silent corruption
Here's the summary of the diff to v1:
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively
001/5:[down] 'python: tests: Fix error handling'
002/5:[down] 'ocaml: tests: Fix error handling'
003/5:[0083] [FC] 'docs: Clarify how callbacks should handle errors'
004/5:[down] 'copy: Pass in dummy variable rather than &errno to
callback'
005/5:[down] 'copy: CVE-2022-0485: Fail nbdcopy if NBD read or write
fails'
except that my version of the git backport-diff script doesn't handle
renamed patches well.
Patch 3 was originally at:
https://listman.redhat.com/archives/libguestfs/2022-February/msg00037.html
Patch 5 was originally at:
https://listman.redhat.com/archives/libguestfs/2022-February/msg00039.html
the rest are indeed new to this posting. Enough has changed that I
didn't carry forward any positive reviews from Rich, Nir, or Laszlo.
At the bottom of the patch, I'll include the full diff of how v2
improves over v1.
Eric Blake (5):
python: tests: Fix error handling
ocaml: tests: Fix error handling
docs: Clarify how callbacks should handle errors
copy: Pass in dummy variable rather than &errno to callback
copy: CVE-2022-0485: Fail nbdcopy if NBD read or write fails
docs/libnbd.pod | 69 +++++++++++++++++++++-------
generator/API.ml | 24 +++++++---
python/t/590-aio-copy.py | 4 +-
ocaml/tests/test_590_aio_copy.ml | 8 ++--
TODO | 1 +
copy/Makefile.am | 4 +-
copy/copy-nbd-error.sh | 78 ++++++++++++++++++++++++++++++++
copy/file-ops.c | 28 +++++-------
copy/multi-thread-copying.c | 23 ++++++++--
copy/nbdcopy.h | 7 +--
copy/null-ops.c | 18 +++-----
11 files changed, 204 insertions(+), 60 deletions(-)
create mode 100755 copy/copy-nbd-error.sh
diff --git c/docs/libnbd.pod w/docs/libnbd.pod
index 006d530c..15bdf0a8 100644
--- c/docs/libnbd.pod
+++ w/docs/libnbd.pod
@@ -826,11 +826,15 @@ This can be used to free associated C<user_data>.
For example:
(nbd_chunk_callback) { .callback = my_fn,
.user_data = my_data,
.free = free },
- NBD_NULL_CALLBACK(completion),
+ NBD_NULL_COMPLETION,
0);
-will call L<free(3)> on C<my_data> after the last time that the
-S<C<chunk.callback = my_fn>> function is called.
+will call L<free(3)> once on C<my_data> after the point where it is
+known that the S<C<chunk.callback = my_fn>> function can no longer
be
+called, regardless of how many times C<my_fn> was actually called. If
+both a mid-command and completion callback are supplied, the functions
+will be reached in this order: mid-function callbacks, completion
+callback, mid-function free, and finally completion free.
The free function is only accessible in the C API as it is not needed
in garbage collected programming languages.
@@ -858,14 +862,16 @@ same nbd object, as it would cause deadlock.
=head2 Completion callbacks
-All of the low-level commands have a completion callback variant that
-registers a callback function used right before the command is marked
-complete.
+All of the asychronous commands have an optional completion callback
+function that is used right before the command is marked complete,
+after any mid-command callbacks have finished, and before any free
+functions.
When the completion callback returns C<1>, the command is
automatically retired (there is no need to call
-L<nbd_aio_command_completed(3)>); for any other return value, the command
-still needs to be retired.
+L<nbd_aio_command_completed(3)>); for any other return value, the
+command still needs to be manually retired (otherwise, the command
+will tie up resources until L<nbd_close(3)> is eventually reached).
=head2 Callbacks with C<int *error> parameter
@@ -874,37 +880,42 @@ L<nbd_block_status(3)>) involve the use of a
callback function invoked
by the state machine at appropriate points in the server's reply
before the overall command is complete. These callback functions,
along with all of the completion callbacks, include a parameter
-C<error> containing the value of any error detected so far. If a
-callback function fails and wants to change the resulting error of the
-overall command visible later in the API sequence, it should assign
-back into C<error> and return C<-1>. Assignments into
C<error> are
-ignored for any other return value; similarly, assigning C<0> into
-C<error> does not have an effect.
+C<error> which is a pointer containing the value of any error detected
+so far. If a callback function fails and wants to change the
+resulting error of the overall command visible later in the API
+sequence, it should assign back into C<error> and return C<-1> in
the
+C API. Assignments into C<error> are ignored for any other return
+value; similarly, assigning C<0> into C<error> does not have an
+effect. In other language bindings, reporting callback errors is
+generally done by raising an exception rather than by return value.
Note that a mid-command callback might never be reached, such as if
libnbd detects that the command was invalid to send (see
-L<nbd_set_strict_mode(3)>) or if the server reports a failure. It is
-safe for a mid-command callback to ignore non-zero C<error>: all the
-other parameters to the mid-command callback will still be valid
-(corresponding to the current portion of the server's reply), and the
-overall command will still fail (at the completion callback or
-L<nbd_aio_command_completed(3)> for an asynchronous command, or as the
-result of the overall synchronous command). Returing C<-1> from a
-mid-command callback does not prevent that callback from being reached
-again, if the server sends more mid-command replies that warrant
-another use of that callback. A mid-command callback may be reached
-more times than expected if the server is non-compliant.
+L<nbd_set_strict_mode(3)>) or if the server reports a failure that
+concludes the command. It is safe for a mid-command callback to
+ignore non-zero C<error>: all the other parameters to the mid-command
+callback will still be valid (corresponding to the current portion of
+the server's reply), and the overall command will still fail (at the
+completion callback or L<nbd_aio_command_completed(3)> for an
+asynchronous command, or as the result of the overall synchronous
+command). Returing C<-1> from a mid-command callback does not prevent
+that callback from being reached again, if the server sends more
+mid-command replies that warrant another use of that callback. A
+mid-command callback may be reached more times than expected if the
+server is non-compliant.
On the other hand, if a completion callback is supplied (only possible
with asynchronous commands), it will always be reached exactly once,
-and the completion callback must not ignore the value of C<error>. In
-particular, the content of a buffer passed to L<nbd_aio_pread(3)> or
-L<nbd_aio_pread_structured(3)> is unspecified if C<error> is set on
-entry to the completion callback. It is recommended that if your code
-uses automatic command retirement, then the completion function should
-return C<1> on all control paths, even when handling errors (note that
-with automatic retirement, assigning into C<error> is pointless as
-there is no later API to see that value).
+and the completion callback must not ignore the value pointed to by
+C<error>. In particular, the content of a buffer passed to
+L<nbd_aio_pread(3)> or L<nbd_aio_pread_structured(3)> is undefined
+if C<*error> is non-zero on entry to the completion callback. It is
+recommended that if you choose to use automatic command retirement
+(where the completion callback returns C<1> to avoid needing to call
+L<nbd_aio_command_completed(3)> later), your completion function
+should return C<1> on all control paths, even when handling errors
+(note that with automatic retirement, assigning into C<error> is
+pointless as there is no later API to see that value).
=head1 COMPILING YOUR PROGRAM
diff --git c/generator/API.ml w/generator/API.ml
index bdb8e7c8..012016bc 100644
--- c/generator/API.ml
+++ w/generator/API.ml
@@ -1831,7 +1831,7 @@ "pread", {
The C<flags> parameter must be C<0> for now (it exists for future
NBD
protocol extensions).
-Note that if this command fails, the contents of C<buf> are
unspecified."
+Note that if this command fails, the contents of C<buf> are
undefined."
^ strict_call_description;
see_also = [Link "aio_pread"; Link "pread_structured";
Link "get_block_size"; Link
"set_strict_mode"];
@@ -1918,7 +1918,7 @@ "pread_structured", {
this, see L<nbd_can_df(3)>). Libnbd does not validate that the server
actually obeys the flag.
-Note that if this command fails, the contents of C<buf> are
unspecified."
+Note that if this command fails, the contents of C<buf> are
undefined."
^ strict_call_description;
see_also = [Link "can_df"; Link "pread";
Link "aio_pread_structured"; Link
"get_block_size";
@@ -2459,7 +2459,7 @@ "aio_pread", {
as described in L<libnbd(3)/Completion callbacks>.
Note that you must ensure C<buf> is valid until the command has
-completed. Furthermore, the contents of C<buf> are unspecified if the
+completed. Furthermore, the contents of C<buf> are undefined if the
C<error> parameter to C<completion_callback> is set, or if
L<nbd_aio_command_completed(3)> reports failure. Other parameters behave
as documented in L<nbd_pread(3)>."
@@ -2487,7 +2487,7 @@ "aio_pread_structured", {
as described in L<libnbd(3)/Completion callbacks>.
Note that you must ensure C<buf> is valid until the command has
-completed. Furthermore, the contents of C<buf> are unspecified if the
+completed. Furthermore, the contents of C<buf> are undefined if the
C<error> parameter to C<completion_callback> is set, or if
L<nbd_aio_command_completed(3)> reports failure. Other parameters behave
as documented in L<nbd_pread_structured(3)>."
diff --git c/python/t/590-aio-copy.py w/python/t/590-aio-copy.py
index 6cde5934..861fa6c8 100644
--- c/python/t/590-aio-copy.py
+++ w/python/t/590-aio-copy.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -36,6 +36,7 @@ def asynch_copy(src, dst):
# This callback is called when any pread from the source
# has completed.
def read_completed(buf, offset, error):
+ assert not error
global bytes_read
bytes_read += buf.size()
wr = (buf, offset)
@@ -46,6 +47,7 @@ def asynch_copy(src, dst):
# This callback is called when any pwrite to the destination
# has completed.
def write_completed(buf, error):
+ assert not error
global bytes_written
bytes_written += buf.size()
# By returning 1 here we auto-retire the pwrite command.
diff --git c/ocaml/tests/test_590_aio_copy.ml w/ocaml/tests/test_590_aio_copy.ml
index 11d89256..a0339c8b 100644
--- c/ocaml/tests/test_590_aio_copy.ml
+++ w/ocaml/tests/test_590_aio_copy.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* libnbd OCaml test case
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -45,7 +45,8 @@ let
* next iteration of the loop.
*)
let writes = ref [] in
- let read_completed buf offset _ + let read_completed buf offset err +
assert (!err = 0);
bytes_read := !bytes_read + NBD.Buffer.size buf;
(* Get ready to issue a write command. *)
writes := (buf, offset) :: !writes;
@@ -56,7 +57,8 @@ let
(* This callback is called when any pwrite to the destination
* has completed.
*)
- let write_completed buf _ + let write_completed buf err + assert (!err =
0);
bytes_written := !bytes_written + NBD.Buffer.size buf;
(* By returning 1 here we auto-retire the pwrite command. *)
1
diff --git c/copy/Makefile.am w/copy/Makefile.am
index 85989798..e729f86a 100644
--- c/copy/Makefile.am
+++ w/copy/Makefile.am
@@ -33,7 +33,7 @@ EXTRA_DIST = \
copy-nbd-to-small-nbd-error.sh \
copy-nbd-to-sparse-file.sh \
copy-nbd-to-stdout.sh \
- copy-nbd-error.sh \
+ copy-nbd-error.sh \
copy-progress-bar.sh \
copy-sparse.sh \
copy-sparse-allocated.sh \
@@ -125,7 +125,7 @@ TESTS += \
copy-stdin-to-nbd.sh \
copy-stdin-to-null.sh \
copy-nbd-to-stdout.sh \
- copy-nbd-error.sh \
+ copy-nbd-error.sh \
copy-progress-bar.sh \
copy-sparse.sh \
copy-sparse-allocated.sh \
diff --git c/copy/copy-nbd-error.sh w/copy/copy-nbd-error.sh
index 50723195..89f0a2f1 100755
--- c/copy/copy-nbd-error.sh
+++ w/copy/copy-nbd-error.sh
@@ -16,6 +16,9 @@
# License along with this library; if not, write to the Free Software
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+# Tests several scenarios of handling NBD server errors
+# Serves as a regression test for the CVE-2022-0485 fix.
+
. ../tests/functions.sh
set -e
@@ -23,32 +26,53 @@ set -x
requires nbdkit --exit-with-parent --version
-pidfile=copy-nbd-error.pid
-sock=$(mktemp -u /tmp/libnbd-test-copy.XXXXXX)
-cleanup_fn rm -f $pidfile $sock
-
-# Run an nbdkit server that randomly fails.
-nbdkit --exit-with-parent -f -v -P $pidfile -U $sock \
- --filter=error --filter=noextents \
- memory size=5M error-pread-rate=0.5 error-pwrite-rate=0.5 &
-# Wait for the pidfile to appear.
-for i in {1..60}; do
- if test -f $pidfile; then
- break
- fi
- sleep 1
-done
-if ! test -f $pidfile; then
- echo "$0: nbdkit did not start up"
- exit 1
-fi
-
fail=0
+# Failure to get block status should not be fatal, but merely downgrade to
+# reading the entire image as if data
+echo "Testing extents failures on source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \
+ error-extents-rate=1 ] null: || fail=1
+
# Failure to read should be fatal
-$VG nbdcopy -- "nbd+unix:///?socket=$sock" null: && fail=1
+echo "Testing read failures on non-sparse source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \
+ error-pread-rate=0.5 ] null: && fail=1
-# Failure to write should be fatal
-$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ]
"nbd+unix:///?socket=$sock" && fail=1
+# However, reliable block status on a sparse image can avoid the need to read
+echo "Testing read failures on sparse source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error null 5M \
+ error-pread-rate=1 ] null: || fail=1
+
+# Failure to write data should be fatal
+echo "Testing write data failures on arbitrary destination"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \
+ memory 5M error-pwrite-rate=0.5 ] && fail=1
+
+# However, writing zeroes can bypass the need for normal writes
+echo "Testing write data failures from sparse source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \
+ memory 5M error-pwrite-rate=1 ] || fail=1
+
+# Failure to write zeroes should be fatal
+echo "Testing write zero failures on arbitrary destination"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error memory 5M \
+ error-zero-rate=1 ] && fail=1
+
+# However, assuming/learning destination is zero can skip need to write
+echo "Testing write failures on pre-zeroed destination"
+$VG nbdcopy --destination-is-zero -- \
+ [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error memory 5M \
+ error-pwrite-rate=1 error-zero-rate=1 ] || fail=1
+
+# Likewise, when write zero is not advertised, fallback to normal write works
+echo "Testing write zeroes to destination without zero support"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=nozero --filter=error memory 5M \
+ error-zero-rate=1 ] || fail=1
exit $fail
diff --git c/copy/file-ops.c w/copy/file-ops.c
index eb30f924..aaf04ade 100644
--- c/copy/file-ops.c
+++ w/copy/file-ops.c
@@ -587,10 +587,12 @@ file_asynch_read (struct rw *rw,
struct command *command,
nbd_completion_callback cb)
{
+ int dummy = 0;
+
file_synch_read (rw, slice_ptr (command->slice),
command->slice.len, command->offset);
- errno = 0; /* safe, since file_synch_read exits on error */
- cb.callback (cb.user_data, &errno);
+ /* file_synch_read called exit() on error */
+ cb.callback (cb.user_data, &dummy);
}
static void
@@ -598,20 +600,23 @@ file_asynch_write (struct rw *rw,
struct command *command,
nbd_completion_callback cb)
{
+ int dummy = 0;
+
file_synch_write (rw, slice_ptr (command->slice),
command->slice.len, command->offset);
- errno = 0; /* safe, since file_synch_write exits on error */
- cb.callback (cb.user_data, &errno);
+ /* file_synch_write called exit() on error */
+ cb.callback (cb.user_data, &dummy);
}
static bool
file_asynch_zero (struct rw *rw, struct command *command,
nbd_completion_callback cb, bool allocate)
{
+ int dummy = 0;
+
if (!file_synch_zero (rw, command->offset, command->slice.len,
allocate))
return false;
- errno = 0;
- cb.callback (cb.user_data, &errno);
+ cb.callback (cb.user_data, &dummy);
return true;
}
diff --git c/copy/multi-thread-copying.c w/copy/multi-thread-copying.c
index c578c4bc..2aa74d63 100644
--- c/copy/multi-thread-copying.c
+++ w/copy/multi-thread-copying.c
@@ -28,6 +28,7 @@
#include <errno.h>
#include <assert.h>
#include <sys/stat.h>
+#include <inttypes.h>
#include <pthread.h>
@@ -378,8 +379,8 @@ finished_read (void *vp, int *error)
/* XXX - is it worth retrying a failed command? */
if (*error) {
- errno = *error;
- perror("read");
+ fprintf (stderr, "read at offset 0x%" PRIx64 "failed:
%s\n",
+ command->offset, strerror (*error));
exit (EXIT_FAILURE);
}
@@ -400,6 +401,7 @@ finished_read (void *vp, int *error)
bool last_is_hole = false;
uint64_t i;
struct command *newcommand;
+ int dummy = 0;
/* Iterate over whole blocks in the command, starting on a block
* boundary.
@@ -482,8 +484,7 @@ finished_read (void *vp, int *error)
/* Free the original command since it has been split into
* subcommands and the original is no longer needed.
*/
- errno = 0;
- free_command (command, &errno);
+ free_command (command, &dummy);
}
return 1; /* auto-retires the command */
@@ -510,6 +511,7 @@ fill_dst_range_with_zeroes (struct command *command)
{
char *data;
size_t data_size;
+ int dummy = 0;
if (destination_is_zero)
goto free_and_return;
@@ -545,8 +547,7 @@ fill_dst_range_with_zeroes (struct command *command)
free (data);
free_and_return:
- errno = 0;
- free_command (command, &errno);
+ free_command (command, &dummy);
}
static int
@@ -557,8 +558,8 @@ free_command (void *vp, int *error)
/* XXX - is it worth retrying a failed command? */
if (*error) {
- errno = *error;
- perror("write");
+ fprintf (stderr, "write at offset 0x%" PRIx64 "failed:
%s\n",
+ command->offset, strerror (*error));
exit (EXIT_FAILURE);
}
diff --git c/copy/null-ops.c w/copy/null-ops.c
index 924eaf1e..5f1fda50 100644
--- c/copy/null-ops.c
+++ w/copy/null-ops.c
@@ -117,16 +117,18 @@ null_asynch_write (struct rw *rw,
struct command *command,
nbd_completion_callback cb)
{
- errno = 0;
- cb.callback (cb.user_data, &errno);
+ int dummy = 0;
+
+ cb.callback (cb.user_data, &dummy);
}
static bool
null_asynch_zero (struct rw *rw, struct command *command,
nbd_completion_callback cb, bool allocate)
{
- errno = 0;
- cb.callback (cb.user_data, &errno);
+ int dummy = 0;
+
+ cb.callback (cb.user_data, &dummy);
return true;
}
Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 1/5] python: tests: Fix error handling
Like a lot of the C examples, the aio copy test ignores read and write
errors in the completion callback, which can cause silent data
corruption. The failure in the test is not critical, but this is a bad
example that may be copied by developers to a real application.
The test dies with an assertion failure now if completion callback
fails. Tested with the temporary patch of:
| diff --git i/python/t/590-aio-copy.py w/python/t/590-aio-copy.py
| index 861fa6c8..4cd64d83 100644
| --- i/python/t/590-aio-copy.py
| +++ w/python/t/590-aio-copy.py
| @@ -117,7 +117,8 @@ src.set_handle_name("src")
| dst = nbd.NBD()
| dst.set_handle_name("dst")
| src.connect_command(["nbdkit", "-s",
"--exit-with-parent", "-r",
| - "pattern", "size=%d" % disk_size])
| + "--filter=error", "pattern",
"error-pread-rate=1",
| + "size=%d" % disk_size])
| dst.connect_command(["nbdkit", "-s",
"--exit-with-parent",
| "memory", "size=%d" % disk_size])
| asynch_copy(src, dst)
---
python/t/590-aio-copy.py | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/python/t/590-aio-copy.py b/python/t/590-aio-copy.py
index 6cde5934..861fa6c8 100644
--- a/python/t/590-aio-copy.py
+++ b/python/t/590-aio-copy.py
@@ -1,5 +1,5 @@
# libnbd Python bindings
-# Copyright (C) 2010-2019 Red Hat Inc.
+# Copyright (C) 2010-2022 Red Hat Inc.
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -36,6 +36,7 @@ def asynch_copy(src, dst):
# This callback is called when any pread from the source
# has completed.
def read_completed(buf, offset, error):
+ assert not error
global bytes_read
bytes_read += buf.size()
wr = (buf, offset)
@@ -46,6 +47,7 @@ def asynch_copy(src, dst):
# This callback is called when any pwrite to the destination
# has completed.
def write_completed(buf, error):
+ assert not error
global bytes_written
bytes_written += buf.size()
# By returning 1 here we auto-retire the pwrite command.
--
2.34.1
Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 2/5] ocaml: tests: Fix error handling
Like a lot of the C examples, the aio copy test ignores read and write
errors in the completion callback, which can cause silent data
corruption. The failure in the test is not critical, but this is a bad
example that may be copied by developers to a real application.
The test dies with an assertion failure now if completion callback
fails. Tested with the temporary patch of:
| diff --git i/ocaml/tests/test_590_aio_copy.ml
w/ocaml/tests/test_590_aio_copy.ml
| index a0339c8b..332bf31b 100644
| --- i/ocaml/tests/test_590_aio_copy.ml
| +++ w/ocaml/tests/test_590_aio_copy.ml
| @@ -120,7 +120,8 @@ let
| let dst = NBD.create () in
| NBD.set_handle_name dst "dst";
| NBD.connect_command src ["nbdkit"; "-s";
"--exit-with-parent"; "-r";
| - "pattern"; sprintf "size=%d"
disk_size];
| + "--filter=error"; "pattern";
"error-pread-rate=1";
| + sprintf "size=%d" disk_size];
| NBD.connect_command dst ["nbdkit"; "-s";
"--exit-with-parent";
| "memory"; sprintf "size=%d"
disk_size];
| asynch_copy src dst
---
ocaml/tests/test_590_aio_copy.ml | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/ocaml/tests/test_590_aio_copy.ml b/ocaml/tests/test_590_aio_copy.ml
index 11d89256..a0339c8b 100644
--- a/ocaml/tests/test_590_aio_copy.ml
+++ b/ocaml/tests/test_590_aio_copy.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* libnbd OCaml test case
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -45,7 +45,8 @@ let
* next iteration of the loop.
*)
let writes = ref [] in
- let read_completed buf offset _ + let read_completed buf offset err +
assert (!err = 0);
bytes_read := !bytes_read + NBD.Buffer.size buf;
(* Get ready to issue a write command. *)
writes := (buf, offset) :: !writes;
@@ -56,7 +57,8 @@ let
(* This callback is called when any pwrite to the destination
* has completed.
*)
- let write_completed buf _ + let write_completed buf err + assert (!err =
0);
bytes_written := !bytes_written + NBD.Buffer.size buf;
(* By returning 1 here we auto-retire the pwrite command. *)
1
--
2.34.1
Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 3/5] docs: Clarify how callbacks should handle errors
Recent patches have demonstrated confusion on the order in which
callbacks are reached, when it is safe or dangerous to ignore *error,
and what a completion callback should do when auto-retirement is in
use. Add wording to make it more obvious that:
- callbacks are reached in the following order: mid-command callback
(0, 1, or many times, if supplied), completion callback (exactly
once, if supplied), mid-command free (exactly once, if supplied),
completion free (exactly once, if supplied)
- returning -1 from a mid-command callback does does not prevent
future callbacks
- ignoring *error in a mid-command callback is safe
- completion callbacks are reached unconditionally, and must NOT ignore
*error
- if the user chooses to use auto-retirement instead of manual calls to
nbd_aio_command_completed, the completion callback should return 1 even
on error cases to avoid complicating command cleanup
- the contents of buf after nbd_pread and friends is undefined on
error (at present, if the user did not pre-initialize the buffer,
there are some code paths in libnbd that leave it uninitialized)
---
docs/libnbd.pod | 69 +++++++++++++++++++++++++++++++++++++-----------
generator/API.ml | 24 ++++++++++++-----
2 files changed, 71 insertions(+), 22 deletions(-)
diff --git a/docs/libnbd.pod b/docs/libnbd.pod
index eb8038b0..15bdf0a8 100644
--- a/docs/libnbd.pod
+++ b/docs/libnbd.pod
@@ -829,8 +829,12 @@ This can be used to free associated C<user_data>.
For example:
NBD_NULL_COMPLETION,
0);
-will call L<free(3)> on C<my_data> after the last time that the
-S<C<chunk.callback = my_fn>> function is called.
+will call L<free(3)> once on C<my_data> after the point where it is
+known that the S<C<chunk.callback = my_fn>> function can no longer
be
+called, regardless of how many times C<my_fn> was actually called. If
+both a mid-command and completion callback are supplied, the functions
+will be reached in this order: mid-function callbacks, completion
+callback, mid-function free, and finally completion free.
The free function is only accessible in the C API as it is not needed
in garbage collected programming languages.
@@ -858,27 +862,60 @@ same nbd object, as it would cause deadlock.
=head2 Completion callbacks
-All of the low-level commands have a completion callback variant that
-registers a callback function used right before the command is marked
-complete.
+All of the asychronous commands have an optional completion callback
+function that is used right before the command is marked complete,
+after any mid-command callbacks have finished, and before any free
+functions.
When the completion callback returns C<1>, the command is
automatically retired (there is no need to call
-L<nbd_aio_command_completed(3)>); for any other return value, the command
-still needs to be retired.
+L<nbd_aio_command_completed(3)>); for any other return value, the
+command still needs to be manually retired (otherwise, the command
+will tie up resources until L<nbd_close(3)> is eventually reached).
=head2 Callbacks with C<int *error> parameter
Some of the high-level commands (L<nbd_pread_structured(3)>,
-L<nbd_block_status(3)>) involve the use of a callback function invoked by
-the state machine at appropriate points in the server's reply before
-the overall command is complete. These callback functions, along with
-all of the completion callbacks, include a parameter C<error>
-containing the value of any error detected so far; if the callback
-function fails, it should assign back into C<error> and return
C<-1>
-to change the resulting error of the overall command. Assignments
-into C<error> are ignored for any other return value; similarly,
-assigning C<0> into C<error> does not have an effect.
+L<nbd_block_status(3)>) involve the use of a callback function invoked
+by the state machine at appropriate points in the server's reply
+before the overall command is complete. These callback functions,
+along with all of the completion callbacks, include a parameter
+C<error> which is a pointer containing the value of any error detected
+so far. If a callback function fails and wants to change the
+resulting error of the overall command visible later in the API
+sequence, it should assign back into C<error> and return C<-1> in
the
+C API. Assignments into C<error> are ignored for any other return
+value; similarly, assigning C<0> into C<error> does not have an
+effect. In other language bindings, reporting callback errors is
+generally done by raising an exception rather than by return value.
+
+Note that a mid-command callback might never be reached, such as if
+libnbd detects that the command was invalid to send (see
+L<nbd_set_strict_mode(3)>) or if the server reports a failure that
+concludes the command. It is safe for a mid-command callback to
+ignore non-zero C<error>: all the other parameters to the mid-command
+callback will still be valid (corresponding to the current portion of
+the server's reply), and the overall command will still fail (at the
+completion callback or L<nbd_aio_command_completed(3)> for an
+asynchronous command, or as the result of the overall synchronous
+command). Returing C<-1> from a mid-command callback does not prevent
+that callback from being reached again, if the server sends more
+mid-command replies that warrant another use of that callback. A
+mid-command callback may be reached more times than expected if the
+server is non-compliant.
+
+On the other hand, if a completion callback is supplied (only possible
+with asynchronous commands), it will always be reached exactly once,
+and the completion callback must not ignore the value pointed to by
+C<error>. In particular, the content of a buffer passed to
+L<nbd_aio_pread(3)> or L<nbd_aio_pread_structured(3)> is undefined
+if C<*error> is non-zero on entry to the completion callback. It is
+recommended that if you choose to use automatic command retirement
+(where the completion callback returns C<1> to avoid needing to call
+L<nbd_aio_command_completed(3)> later), your completion function
+should return C<1> on all control paths, even when handling errors
+(note that with automatic retirement, assigning into C<error> is
+pointless as there is no later API to see that value).
=head1 COMPILING YOUR PROGRAM
diff --git a/generator/API.ml b/generator/API.ml
index cf2e7543..012016bc 100644
--- a/generator/API.ml
+++ b/generator/API.ml
@@ -1,6 +1,6 @@
(* hey emacs, this is OCaml code: -*- tuareg -*- *)
(* nbd client library in userspace: the API
- * Copyright (C) 2013-2021 Red Hat Inc.
+ * Copyright (C) 2013-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -1829,7 +1829,9 @@ "pread", {
L<nbd_get_block_size(3)>.
The C<flags> parameter must be C<0> for now (it exists for future
NBD
-protocol extensions)."
+protocol extensions).
+
+Note that if this command fails, the contents of C<buf> are
undefined."
^ strict_call_description;
see_also = [Link "aio_pread"; Link "pread_structured";
Link "get_block_size"; Link
"set_strict_mode"];
@@ -1914,7 +1916,9 @@ "pread_structured", {
C<LIBNBD_CMD_FLAG_DF> meaning that the server should not reply with
more than one fragment (if that is supported - some servers cannot do
this, see L<nbd_can_df(3)>). Libnbd does not validate that the server
-actually obeys the flag."
+actually obeys the flag.
+
+Note that if this command fails, the contents of C<buf> are
undefined."
^ strict_call_description;
see_also = [Link "can_df"; Link "pread";
Link "aio_pread_structured"; Link
"get_block_size";
@@ -2155,7 +2159,8 @@ "block_status", {
for contexts known to libnbd, B<E<lt>libnbd.hE<gt>> contains
constants
beginning with C<LIBNBD_STATE_> that may help decipher the values.
On entry to the callback, the C<error> parameter contains the errno
-value of any previously detected error.
+value of any previously detected error, but even if an earlier error
+was detected, the current C<metacontext> and C<entries> are valid.
It is possible for the extent function to be called
more times than you expect (if the server is buggy),
@@ -2454,7 +2459,10 @@ "aio_pread", {
as described in L<libnbd(3)/Completion callbacks>.
Note that you must ensure C<buf> is valid until the command has
-completed. Other parameters behave as documented in
L<nbd_pread(3)>."
+completed. Furthermore, the contents of C<buf> are undefined if the
+C<error> parameter to C<completion_callback> is set, or if
+L<nbd_aio_command_completed(3)> reports failure. Other parameters behave
+as documented in L<nbd_pread(3)>."
^ strict_call_description;
example = Some "examples/aio-connect-read.c";
see_also = [SectionLink "Issuing asynchronous commands";
@@ -2478,7 +2486,11 @@ "aio_pread_structured", {
Or supply the optional C<completion_callback> which will be invoked
as described in L<libnbd(3)/Completion callbacks>.
-Other parameters behave as documented in
L<nbd_pread_structured(3)>."
+Note that you must ensure C<buf> is valid until the command has
+completed. Furthermore, the contents of C<buf> are undefined if the
+C<error> parameter to C<completion_callback> is set, or if
+L<nbd_aio_command_completed(3)> reports failure. Other parameters behave
+as documented in L<nbd_pread_structured(3)>."
^ strict_call_description;
see_also = [SectionLink "Issuing asynchronous commands";
Link "aio_pread"; Link "pread_structured";
--
2.34.1
Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 4/5] copy: Pass in dummy variable rather than &errno to callback
In several places where asynch handlers manually call the provided
nbd_completion_callback, the value of errno is indeterminate (for
example, in file-ops.c:file_asynch_read(), the previous call to
file_synch_read() already triggered exit() on error, but does not
guarantee what is left in errno on success). As the callback should
be paying attention to the value of *error (to be fixed in the next
patch), we are better off ensuring that we pass in a pointer to a
known-zero value; at which point, it is easier to use a dummy variable
on the stack than to mess around with errno and it's magic macro
expansion into a thread-local storage location.
Note that several callsites then check if the callback returned -1,
and if so assume that the callback has caused errno to now have a sane
value to pass on to perror. In theory, the fact that we are no longer
passing in &errno means that if the callback assigns into *error but
did not otherwise affect errno, our perror call would no longer
reflect that value. But in practice, since the callback never
actually returned -1, nor even assigned into *error, the call to
perror is dead code; although I have chosen to defer that additional
cleanup to the next patch.
---
copy/file-ops.c | 17 ++++++++++-------
copy/multi-thread-copying.c | 8 +++++---
copy/null-ops.c | 12 +++++++-----
3 files changed, 22 insertions(+), 15 deletions(-)
diff --git a/copy/file-ops.c b/copy/file-ops.c
index 84704341..e37a5014 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace.
- * Copyright (C) 2020 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -587,10 +587,11 @@ file_asynch_read (struct rw *rw,
struct command *command,
nbd_completion_callback cb)
{
+ int dummy = 0;
+
file_synch_read (rw, slice_ptr (command->slice),
command->slice.len, command->offset);
- errno = 0;
- if (cb.callback (cb.user_data, &errno) == -1) {
+ if (cb.callback (cb.user_data, &dummy) == -1) {
perror (rw->name);
exit (EXIT_FAILURE);
}
@@ -601,10 +602,11 @@ file_asynch_write (struct rw *rw,
struct command *command,
nbd_completion_callback cb)
{
+ int dummy = 0;
+
file_synch_write (rw, slice_ptr (command->slice),
command->slice.len, command->offset);
- errno = 0;
- if (cb.callback (cb.user_data, &errno) == -1) {
+ if (cb.callback (cb.user_data, &dummy) == -1) {
perror (rw->name);
exit (EXIT_FAILURE);
}
@@ -614,10 +616,11 @@ static bool
file_asynch_zero (struct rw *rw, struct command *command,
nbd_completion_callback cb, bool allocate)
{
+ int dummy = 0;
+
if (!file_synch_zero (rw, command->offset, command->slice.len,
allocate))
return false;
- errno = 0;
- if (cb.callback (cb.user_data, &errno) == -1) {
+ if (cb.callback (cb.user_data, &dummy) == -1) {
perror (rw->name);
exit (EXIT_FAILURE);
}
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index b17ca598..815b8a02 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace.
- * Copyright (C) 2020 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -393,6 +393,7 @@ finished_read (void *vp, int *error)
bool last_is_hole = false;
uint64_t i;
struct command *newcommand;
+ int dummy = 0;
/* Iterate over whole blocks in the command, starting on a block
* boundary.
@@ -475,7 +476,7 @@ finished_read (void *vp, int *error)
/* Free the original command since it has been split into
* subcommands and the original is no longer needed.
*/
- free_command (command, &errno);
+ free_command (command, &dummy);
}
return 1; /* auto-retires the command */
@@ -502,6 +503,7 @@ fill_dst_range_with_zeroes (struct command *command)
{
char *data;
size_t data_size;
+ int dummy = 0;
if (destination_is_zero)
goto free_and_return;
@@ -537,7 +539,7 @@ fill_dst_range_with_zeroes (struct command *command)
free (data);
free_and_return:
- free_command (command, &errno);
+ free_command (command, &dummy);
}
static int
diff --git a/copy/null-ops.c b/copy/null-ops.c
index a38666d6..11cd5116 100644
--- a/copy/null-ops.c
+++ b/copy/null-ops.c
@@ -1,5 +1,5 @@
/* NBD client library in userspace.
- * Copyright (C) 2020-2021 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -117,8 +117,9 @@ null_asynch_write (struct rw *rw,
struct command *command,
nbd_completion_callback cb)
{
- errno = 0;
- if (cb.callback (cb.user_data, &errno) == -1) {
+ int dummy = 0;
+
+ if (cb.callback (cb.user_data, &dummy) == -1) {
perror (rw->name);
exit (EXIT_FAILURE);
}
@@ -128,8 +129,9 @@ static bool
null_asynch_zero (struct rw *rw, struct command *command,
nbd_completion_callback cb, bool allocate)
{
- errno = 0;
- if (cb.callback (cb.user_data, &errno) == -1) {
+ int dummy = 0;
+
+ if (cb.callback (cb.user_data, &dummy) == -1) {
perror (rw->name);
exit (EXIT_FAILURE);
}
--
2.34.1
Eric Blake
2022-Feb-03 20:25 UTC
[Libguestfs] [libnbd PATCH v2 5/5] copy: CVE-2022-0485: Fail nbdcopy if NBD read or write fails
nbdcopy has a nasty bug when performing multi-threaded copies using
asynchronous nbd calls - it was blindly treating the completion of an
asynchronous command as successful, rather than checking the *error
parameter. This can result in the silent creation of a corrupted
image in two different ways: when a read fails, we blindly wrote
garbage to the destination; when a write fails, we did not flag that
the destination was not written.
Since nbdcopy already calls exit() on a synchronous read or write
failure to a file, doing the same for an asynchronous op to an NBD
server is the simplest solution. A nicer solution, but more invasive
to code and thus not done here, might be to allow up to N retries of
the transaction (in case the read or write failure was transient), or
even having a mode where as much data is copied as possible (portions
of the copy that failed would be logged on stderr, and nbdcopy would
still fail with a non-zero exit status, but this would copy more than
just stopping at the first error, as can be done with rsync or
ddrescue).
Note that since we rely on auto-retiring and do NOT call
nbd_aio_command_completed, our completion callbacks must always return
1 (if they do not exit() first), even when acting on *error, so as not
leave the command allocated until nbd_close. As such, there is no
sane way to return an error to a manual caller of the callback, and
therefore we can drop dead code that calls perror() and exit() if the
callback "failed". It is also worth documenting the contract on when
we must manually call the callback during the asynch_zero callback, so
that we do not leak or double-free the command; thankfully, all the
existing code paths were correct.
The added testsuite script demonstrates several scenarios, some of
which fail without the rest of this patch in place, and others which
showcase ways in which sparse images can bypass errors.
Once backports are complete, a followup patch on the main branch will
edit docs/libnbd-security.pod with the mailing list announcement of
the stable branch commit ids and release versions that incorporate
this fix.
Reported-by: Nir Soffer <nsoffer at redhat.com>
Fixes: bc896eec4d ("copy: Implement multi-conn, multiple threads, multiple
requests in flight.", v1.5.6)
Fixes: https://bugzilla.redhat.com/2046194
---
TODO | 1 +
copy/Makefile.am | 4 +-
copy/copy-nbd-error.sh | 78 +++++++++++++++++++++++++++++++++++++
copy/file-ops.c | 17 +++-----
copy/multi-thread-copying.c | 15 +++++++
copy/nbdcopy.h | 7 ++--
copy/null-ops.c | 10 +----
7 files changed, 108 insertions(+), 24 deletions(-)
create mode 100755 copy/copy-nbd-error.sh
diff --git a/TODO b/TODO
index da157942..7c9c15e2 100644
--- a/TODO
+++ b/TODO
@@ -33,6 +33,7 @@ nbdcopy:
- Better page cache usage, see nbdkit-file-plugin options
fadvise=sequential cache=none.
- Consider io_uring if there are performance bottlenecks.
+ - Configurable retries in response to read or write failures.
nbdfuse:
- If you write beyond the end of the virtual file, it returns EIO.
diff --git a/copy/Makefile.am b/copy/Makefile.am
index f2100853..e729f86a 100644
--- a/copy/Makefile.am
+++ b/copy/Makefile.am
@@ -1,5 +1,5 @@
# nbd client library in userspace
-# Copyright (C) 2020 Red Hat Inc.
+# Copyright (C) 2020-2022 Red Hat Inc.
#
# This library is free software; you can redistribute it and/or
# modify it under the terms of the GNU Lesser General Public
@@ -33,6 +33,7 @@ EXTRA_DIST = \
copy-nbd-to-small-nbd-error.sh \
copy-nbd-to-sparse-file.sh \
copy-nbd-to-stdout.sh \
+ copy-nbd-error.sh \
copy-progress-bar.sh \
copy-sparse.sh \
copy-sparse-allocated.sh \
@@ -124,6 +125,7 @@ TESTS += \
copy-stdin-to-nbd.sh \
copy-stdin-to-null.sh \
copy-nbd-to-stdout.sh \
+ copy-nbd-error.sh \
copy-progress-bar.sh \
copy-sparse.sh \
copy-sparse-allocated.sh \
diff --git a/copy/copy-nbd-error.sh b/copy/copy-nbd-error.sh
new file mode 100755
index 00000000..89f0a2f1
--- /dev/null
+++ b/copy/copy-nbd-error.sh
@@ -0,0 +1,78 @@
+#!/usr/bin/env bash
+# nbd client library in userspace
+# Copyright (C) 2022 Red Hat Inc.
+#
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of the GNU Lesser General Public
+# License as published by the Free Software Foundation; either
+# version 2 of the License, or (at your option) any later version.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
+
+# Tests several scenarios of handling NBD server errors
+# Serves as a regression test for the CVE-2022-0485 fix.
+
+. ../tests/functions.sh
+
+set -e
+set -x
+
+requires nbdkit --exit-with-parent --version
+
+fail=0
+
+# Failure to get block status should not be fatal, but merely downgrade to
+# reading the entire image as if data
+echo "Testing extents failures on source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \
+ error-extents-rate=1 ] null: || fail=1
+
+# Failure to read should be fatal
+echo "Testing read failures on non-sparse source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error pattern 5M \
+ error-pread-rate=0.5 ] null: && fail=1
+
+# However, reliable block status on a sparse image can avoid the need to read
+echo "Testing read failures on sparse source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v --filter=error null 5M \
+ error-pread-rate=1 ] null: || fail=1
+
+# Failure to write data should be fatal
+echo "Testing write data failures on arbitrary destination"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v pattern 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \
+ memory 5M error-pwrite-rate=0.5 ] && fail=1
+
+# However, writing zeroes can bypass the need for normal writes
+echo "Testing write data failures from sparse source"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error --filter=noextents \
+ memory 5M error-pwrite-rate=1 ] || fail=1
+
+# Failure to write zeroes should be fatal
+echo "Testing write zero failures on arbitrary destination"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error memory 5M \
+ error-zero-rate=1 ] && fail=1
+
+# However, assuming/learning destination is zero can skip need to write
+echo "Testing write failures on pre-zeroed destination"
+$VG nbdcopy --destination-is-zero -- \
+ [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=error memory 5M \
+ error-pwrite-rate=1 error-zero-rate=1 ] || fail=1
+
+# Likewise, when write zero is not advertised, fallback to normal write works
+echo "Testing write zeroes to destination without zero support"
+$VG nbdcopy -- [ nbdkit --exit-with-parent -v null 5M ] \
+ [ nbdkit --exit-with-parent -v --filter=nozero --filter=error memory 5M \
+ error-zero-rate=1 ] || fail=1
+
+exit $fail
diff --git a/copy/file-ops.c b/copy/file-ops.c
index e37a5014..aaf04ade 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -591,10 +591,8 @@ file_asynch_read (struct rw *rw,
file_synch_read (rw, slice_ptr (command->slice),
command->slice.len, command->offset);
- if (cb.callback (cb.user_data, &dummy) == -1) {
- perror (rw->name);
- exit (EXIT_FAILURE);
- }
+ /* file_synch_read called exit() on error */
+ cb.callback (cb.user_data, &dummy);
}
static void
@@ -606,10 +604,8 @@ file_asynch_write (struct rw *rw,
file_synch_write (rw, slice_ptr (command->slice),
command->slice.len, command->offset);
- if (cb.callback (cb.user_data, &dummy) == -1) {
- perror (rw->name);
- exit (EXIT_FAILURE);
- }
+ /* file_synch_write called exit() on error */
+ cb.callback (cb.user_data, &dummy);
}
static bool
@@ -620,10 +616,7 @@ file_asynch_zero (struct rw *rw, struct command *command,
if (!file_synch_zero (rw, command->offset, command->slice.len,
allocate))
return false;
- if (cb.callback (cb.user_data, &dummy) == -1) {
- perror (rw->name);
- exit (EXIT_FAILURE);
- }
+ cb.callback (cb.user_data, &dummy);
return true;
}
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index 815b8a02..2aa74d63 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -28,6 +28,7 @@
#include <errno.h>
#include <assert.h>
#include <sys/stat.h>
+#include <inttypes.h>
#include <pthread.h>
@@ -376,6 +377,13 @@ finished_read (void *vp, int *error)
{
struct command *command = vp;
+ /* XXX - is it worth retrying a failed command? */
+ if (*error) {
+ fprintf (stderr, "read at offset 0x%" PRIx64 "failed:
%s\n",
+ command->offset, strerror (*error));
+ exit (EXIT_FAILURE);
+ }
+
if (allocated || sparse_size == 0) {
/* If sparseness detection (see below) is turned off then we write
* the whole command.
@@ -548,6 +556,13 @@ free_command (void *vp, int *error)
struct command *command = vp;
struct buffer *buffer = command->slice.buffer;
+ /* XXX - is it worth retrying a failed command? */
+ if (*error) {
+ fprintf (stderr, "write at offset 0x%" PRIx64 "failed:
%s\n",
+ command->offset, strerror (*error));
+ exit (EXIT_FAILURE);
+ }
+
if (buffer != NULL) {
if (--buffer->refs == 0) {
free (buffer->data);
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
index e7fe1eab..c070f8d7 100644
--- a/copy/nbdcopy.h
+++ b/copy/nbdcopy.h
@@ -1,5 +1,5 @@
/* NBD client library in userspace.
- * Copyright (C) 2020-2021 Red Hat Inc.
+ * Copyright (C) 2020-2022 Red Hat Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -157,7 +157,8 @@ struct rw_ops {
bool allocate);
/* Asynchronous I/O operations. These start the operation and call
- * 'cb' on completion.
+ * 'cb' on completion. 'cb' will return 1, for auto-retiring
with
+ * asynchronous libnbd calls.
*
* The file_ops versions are actually implemented synchronously, but
* still call 'cb'.
@@ -173,7 +174,7 @@ struct rw_ops {
nbd_completion_callback cb);
/* Asynchronously zero. command->slice.buffer is not used. If not
possible,
- * returns false.
+ * returns false. 'cb' must be called only if returning true.
*/
bool (*asynch_zero) (struct rw *rw, struct command *command,
nbd_completion_callback cb, bool allocate);
diff --git a/copy/null-ops.c b/copy/null-ops.c
index 11cd5116..5f1fda50 100644
--- a/copy/null-ops.c
+++ b/copy/null-ops.c
@@ -119,10 +119,7 @@ null_asynch_write (struct rw *rw,
{
int dummy = 0;
- if (cb.callback (cb.user_data, &dummy) == -1) {
- perror (rw->name);
- exit (EXIT_FAILURE);
- }
+ cb.callback (cb.user_data, &dummy);
}
static bool
@@ -131,10 +128,7 @@ null_asynch_zero (struct rw *rw, struct command *command,
{
int dummy = 0;
- if (cb.callback (cb.user_data, &dummy) == -1) {
- perror (rw->name);
- exit (EXIT_FAILURE);
- }
+ cb.callback (cb.user_data, &dummy);
return true;
}
--
2.34.1