The example uses a buffer pool for all requests, but it did not clear the buffers before they were used. If we failed to handle a read error, this could lead to leaking sensitive data to the destination server. Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; --- examples/copy-libev.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/examples/copy-libev.c b/examples/copy-libev.c index 51ff9fb0..13db898a 100644 --- a/examples/copy-libev.c +++ b/examples/copy-libev.c @@ -634,21 +634,28 @@ main (int argc, char *argv[]) /* Check destination server capabilities. */ dst.can_zero = nbd_can_zero (dst.nbd) > 0; /* Start the copy "loop". When request completes, it starts the * next request, until entire image was copied. */ for (i = 0; i < MAX_REQUESTS; i++) { struct request *r = &requests[i]; r->index = i; - r->data = malloc (REQUEST_SIZE); + + /* + * Clear the buffer before starting the copy, so if we fail to + * handle a read error we will not write uninitilized data to + * the destination server, which may leak sensitive data to + * remote host. + */ + r->data = calloc (1, REQUEST_SIZE); if (r->data == NULL) FAIL ("Cannot allocate buffer: %s", strerror (errno)); start_request(r); } /* Start watching events on src and dst handles. */ ev_io_init (&src.watcher, io_cb, get_fd (&src), get_events (&src)); ev_io_start (loop, &src.watcher); -- 2.34.1

This example failed to check the *error parameter to the completion and extent callbacks. - If the source NBD server failed a read, we wrote stale data from the request buffer to the destination image, corrupting the image. - If the destination NBD server failed a write or zero command, we ignored the error, leaving previous content on the destination image, corrupting the image. - Error in the extents callbacks were ignored. I'm not sure if this was a real problem, but it is a very bad example. In all cases, the copy would end with zero exit code creating a corrupted image. Fixed by failing the copy if read, write, zero commands completed with non-zero *error parameter. If an extent callback completed with non-zero *error, we log the error and abort the callback, leaving the extents array as NULL. If extents completion callback completed with non-zero *error we don't need to check it, since we already handle a NULL extents array by disabling can_extents. Here is an example failure: $ ./copy-libev nbd+unix:///?socket=/tmp/src.sock nbd+unix:///?socket=/tmp/dst.sock copy-libev: r0: read failed: Input/output error $ echo $? 1 Signed-off-by: Nir Soffer <nsoffer(a)redhat.com&gt; --- examples/copy-libev.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/examples/copy-libev.c b/examples/copy-libev.c index 13db898a..ad50b64c 100644 --- a/examples/copy-libev.c +++ b/examples/copy-libev.c @@ -184,20 +184,26 @@ static int extent_callback (void *user_data, const char *metacontext, uint64_t offset, uint32_t *entries, size_t nr_entries, int *error) { struct request *r = user_data; if (strcmp (metacontext, LIBNBD_CONTEXT_BASE_ALLOCATION) != 0) { DEBUG ("Unexpected meta context: %s", metacontext); return 1; } + if (*error) { + DEBUG ("r%zu: extent callback for %s failed: %s", + r->index, LIBNBD_CONTEXT_BASE_ALLOCATION, strerror (*error)); + return 1; + } + /* Libnbd returns uint32_t pair (length, flags) for each extent. */ extents_len = nr_entries / 2; extents = malloc (extents_len * sizeof *extents); if (extents == NULL) FAIL ("Cannot allocated extents: %s", strerror (errno)); /* Copy libnbd entries to extents array. */ for (int i = 0, j = 0; i < extents_len; i++, j=i*2) { extents[i].length = entries[j]; @@ -240,20 +246,26 @@ static void wake_up_requests () static int extents_completed (void *user_data, int *error) { struct request *r = (struct request *)user_data; DEBUG ("r%zu: extents completed time=%.6f", r->index, ev_now (loop) - r->started); extents_in_progress = false; + /* + * If extents failed (*error != 0), the extent callback was not + * called, or called with an error, so we did not allocate new + * extents array. + */ + if (extents == NULL) { DEBUG ("r%zu: received no extents, disabling extents", r->index); src.can_extents = false; } /* Start the request to process recvievd extents. This must be done on the * next loop iteration, to avoid deadlock if we need to start a read. */ start_request_soon (r); @@ -434,20 +446,23 @@ start_read(struct request *r) 0); if (cookie == -1) FAIL ("Cannot start read: %s", nbd_get_error ()); } static int read_completed (void *user_data, int *error) { struct request *r = (struct request *)user_data; + if (*error) + FAIL ("r%zu: read failed: %s", r->index, strerror (*error)); + DEBUG ("r%zu: read completed offset=%" PRIi64 " len=%zu", r->index, r->offset, r->length); if (dst.can_zero && is_zero (r->data, r->length)) start_zero (r); else start_write (r); return 1; } @@ -489,20 +504,24 @@ start_zero(struct request *r) if (cookie == -1) FAIL ("Cannot start zero: %s", nbd_get_error ()); } /* Called when async copy or zero request completed. */ static int request_completed (void *user_data, int *error) { struct request *r = (struct request *)user_data; + if (*error) + FAIL ("r%zu: %s failed: %s", + r->index, request_state (r), strerror (*error)); + written += r->length; DEBUG ("r%zu: %s completed offset=%" PRIi64 " len=%zu, time=%.6f", r->index, request_state (r), r->offset, r->length, ev_now (loop) - r->started); if (written == size) { /* The last write completed. Stop all watchers and break out * from the event loop. */ -- 2.34.1