From 73e064ccf09d908febc83761addcc6e76feabf78 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Thu, 6 Feb 2020 11:38:32 -0600 Subject: [PATCH 1/4] nbd: Fix regression with multiple meta contexts Detected by a hang in the libnbd testsuite. If a client requests multiple meta contexts (both base:allocation and qemu:dirty-bitmap:x) at the same time, our attempt to silence a false-positive warning about a potential uninitialized variable introduced botched logic: we were short-circuiting the second context, and never sending the NBD_REPLY_FLAG_DONE. Combining two 'if' into one 'if/else' in bdf200a55 was wrong (I'm a bit embarrassed that such a change was my initial suggestion after the v1 patch, then I did not review the v2 patch that actually got committed). Revert that, and instead silence the false positive warning by replacing 'return ret' with 'return 0' (the value it always has at that point in the code, even though it eluded the deduction abilities of the robot that reported the false positive). Fixes: bdf200a5535 Signed-off-by: Eric Blake Message-Id: <20200206173832.130004-1-eblake@redhat.com> Reviewed-by: Laurent Vivier --- nbd/server.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index 87fcd2e7bf..11a31094ff 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2384,15 +2384,23 @@ static coroutine_fn int nbd_handle_request(NBDClient *client, !client->export_meta.bitmap, NBD_META_ID_BASE_ALLOCATION, errp); - } else { /* client->export_meta.bitmap */ + if (ret < 0) { + return ret; + } + } + + if (client->export_meta.bitmap) { ret = nbd_co_send_bitmap(client, request->handle, client->exp->export_bitmap, request->from, request->len, dont_fragment, true, NBD_META_ID_DIRTY_BITMAP, errp); + if (ret < 0) { + return ret; + } } - return ret; + return 0; } else { return nbd_send_generic_reply(client, request->handle, -EINVAL, "CMD_BLOCK_STATUS not negotiated", From 2485f22fe9211b2e80970f83199b320eee211319 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Tue, 11 Feb 2020 20:31:01 -0600 Subject: [PATCH 2/4] nbd-client: Support leading / in NBD URI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The NBD URI specification [1] states that only one leading slash at the beginning of the URI path component is stripped, not all such slashes. This becomes important to a patch I just proposed to nbdkit [2], which would allow the exportname to select a file embedded within an ext2 image: ext2fs demands an absolute pathname beginning with '/', and because qemu was inadvertantly stripping it, my nbdkit patch had to work around the behavior. [1] https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md [2] https://www.redhat.com/archives/libguestfs/2020-February/msg00109.html Note that the qemu bug only affects handling of URIs such as nbd://host:port//abs/path (where '/abs/path' should be the export name); it is still possible to use --image-opts and pass the desired export name with a leading slash directly through JSON even without this patch. Signed-off-by: Eric Blake Message-Id: <20200212023101.1162686-1-eblake@redhat.com> Reviewed-by: Ján Tomko --- block/nbd.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index 6d3b22f844..f69e61e68a 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1528,8 +1528,10 @@ static int nbd_parse_uri(const char *filename, QDict *options) goto out; } - p = uri->path ? uri->path : "/"; - p += strspn(p, "/"); + p = uri->path ? uri->path : ""; + if (p[0] == '/') { + p++; + } if (p[0]) { qdict_put_str(options, "export", p); } From 7f493662be4045146a8f45119d8834c9088a0ad6 Mon Sep 17 00:00:00 2001 From: Pan Nengyuan Date: Thu, 5 Dec 2019 11:45:27 +0800 Subject: [PATCH 3/4] block/nbd: extract the common cleanup code The BDRVNBDState cleanup code is common in two places, add nbd_clear_bdrvstate() function to do these cleanups. Suggested-by: Stefano Garzarella Signed-off-by: Pan Nengyuan Reviewed-by: Vladimir Sementsov-Ogievskiy Message-Id: <1575517528-44312-2-git-send-email-pannengyuan@huawei.com> Reviewed-by: Eric Blake [eblake: fix compilation error and commit message] Signed-off-by: Eric Blake --- block/nbd.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/block/nbd.c b/block/nbd.c index f69e61e68a..ed0f93ab27 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -95,6 +95,19 @@ typedef struct BDRVNBDState { static int nbd_client_connect(BlockDriverState *bs, Error **errp); +static void nbd_clear_bdrvstate(BDRVNBDState *s) +{ + object_unref(OBJECT(s->tlscreds)); + qapi_free_SocketAddress(s->saddr); + s->saddr = NULL; + g_free(s->export); + s->export = NULL; + g_free(s->tlscredsid); + s->tlscredsid = NULL; + g_free(s->x_dirty_bitmap); + s->x_dirty_bitmap = NULL; +} + static void nbd_channel_error(BDRVNBDState *s, int ret) { if (ret == -EIO) { @@ -1879,11 +1892,7 @@ static int nbd_process_options(BlockDriverState *bs, QDict *options, error: if (ret < 0) { - object_unref(OBJECT(s->tlscreds)); - qapi_free_SocketAddress(s->saddr); - g_free(s->export); - g_free(s->tlscredsid); - g_free(s->x_dirty_bitmap); + nbd_clear_bdrvstate(s); } qemu_opts_del(opts); return ret; @@ -1962,12 +1971,7 @@ static void nbd_close(BlockDriverState *bs) BDRVNBDState *s = bs->opaque; nbd_client_close(bs); - - object_unref(OBJECT(s->tlscreds)); - qapi_free_SocketAddress(s->saddr); - g_free(s->export); - g_free(s->tlscredsid); - g_free(s->x_dirty_bitmap); + nbd_clear_bdrvstate(s); } static int64_t nbd_getlength(BlockDriverState *bs) From 8198cf5ef0ef98118b4176970d1cd998d93ec849 Mon Sep 17 00:00:00 2001 From: Pan Nengyuan Date: Thu, 5 Dec 2019 11:45:28 +0800 Subject: [PATCH 4/4] block/nbd: fix memory leak in nbd_open() In currently implementation there will be a memory leak when nbd_client_connect() returns error status. Here is an easy way to reproduce: 1. run qemu-iotests as follow and check the result with asan: ./check -raw 143 Following is the asan output backtrack: Direct leak of 40 byte(s) in 1 object(s) allocated from: #0 0x7f629688a560 in calloc (/usr/lib64/libasan.so.3+0xc7560) #1 0x7f6295e7e015 in g_malloc0 (/usr/lib64/libglib-2.0.so.0+0x50015) #2 0x56281dab4642 in qobject_input_start_struct /mnt/sdb/qemu-4.2.0-rc0/qapi/qobject-input-visitor.c:295 #3 0x56281dab1a04 in visit_start_struct /mnt/sdb/qemu-4.2.0-rc0/qapi/qapi-visit-core.c:49 #4 0x56281dad1827 in visit_type_SocketAddress qapi/qapi-visit-sockets.c:386 #5 0x56281da8062f in nbd_config /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1716 #6 0x56281da8062f in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1829 #7 0x56281da8062f in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873 Direct leak of 15 byte(s) in 1 object(s) allocated from: #0 0x7f629688a3a0 in malloc (/usr/lib64/libasan.so.3+0xc73a0) #1 0x7f6295e7dfbd in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4ffbd) #2 0x7f6295e96ace in g_strdup (/usr/lib64/libglib-2.0.so.0+0x68ace) #3 0x56281da804ac in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1834 #4 0x56281da804ac in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873 Indirect leak of 24 byte(s) in 1 object(s) allocated from: #0 0x7f629688a3a0 in malloc (/usr/lib64/libasan.so.3+0xc73a0) #1 0x7f6295e7dfbd in g_malloc (/usr/lib64/libglib-2.0.so.0+0x4ffbd) #2 0x7f6295e96ace in g_strdup (/usr/lib64/libglib-2.0.so.0+0x68ace) #3 0x56281dab41a3 in qobject_input_type_str_keyval /mnt/sdb/qemu-4.2.0-rc0/qapi/qobject-input-visitor.c:536 #4 0x56281dab2ee9 in visit_type_str /mnt/sdb/qemu-4.2.0-rc0/qapi/qapi-visit-core.c:297 #5 0x56281dad0fa1 in visit_type_UnixSocketAddress_members qapi/qapi-visit-sockets.c:141 #6 0x56281dad17b6 in visit_type_SocketAddress_members qapi/qapi-visit-sockets.c:366 #7 0x56281dad186a in visit_type_SocketAddress qapi/qapi-visit-sockets.c:393 #8 0x56281da8062f in nbd_config /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1716 #9 0x56281da8062f in nbd_process_options /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1829 #10 0x56281da8062f in nbd_open /mnt/sdb/qemu-4.2.0-rc0/block/nbd.c:1873 Fixes: 8f071c9db506e03ab Reported-by: Euler Robot Signed-off-by: Pan Nengyuan Reviewed-by: Vladimir Sementsov-Ogievskiy Cc: qemu-stable Cc: Vladimir Sementsov-Ogievskiy Message-Id: <1575517528-44312-3-git-send-email-pannengyuan@huawei.com> Reviewed-by: Stefano Garzarella Signed-off-by: Eric Blake --- block/nbd.c | 1 + 1 file changed, 1 insertion(+) diff --git a/block/nbd.c b/block/nbd.c index ed0f93ab27..976be76647 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1915,6 +1915,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags, ret = nbd_client_connect(bs, errp); if (ret < 0) { + nbd_clear_bdrvstate(s); return ret; } /* successfully connected */