On Thu, Oct 05, 2023 at 05:33:26PM +0300, Vladimir Sementsov-Ogievskiy wrote:
+static int +nbd_co_block_status_payload_read(NBDClient *client, NBDRequest *request, + Error **errp) +{ + int payload_len = request->len;
payload_len should be uint64_t
+ g_autofree char *buf = NULL; + size_t count, i, nr_bitmaps; + uint32_t id; +
otherwise, we may do something unexpected here, when reqeuest->len is too big for int:
+ if (payload_len > NBD_MAX_BUFFER_SIZE) { + error_setg(errp, "len (%" PRIu64 ") is larger than max len (%u)", + request->len, NBD_MAX_BUFFER_SIZE); + return -EINVAL; + }
Oh, it looks like I introduced that same type mismatch in commit 8db7e2d6 as well, although it appears to have a latent effect until this series enables the ability for request->length to actually exceed 32 bits. I'll reply on 1/12 with another squash I'm making there.
+ + assert(client->contexts.exp == client->exp); + nr_bitmaps = client->exp->nr_export_bitmaps; + request->contexts = g_new0(NBDMetaContexts, 1); + request->contexts->exp = client->exp; + + if (payload_len % sizeof(uint32_t) || + payload_len < sizeof(NBDBlockStatusPayload) || + payload_len > (sizeof(NBDBlockStatusPayload) + + sizeof(id) * client->contexts.count)) { + goto skip; + }
[..]
* connection right away, -EAGAIN to indicate we were interrupted and the @@ -2505,7 +2593,18 @@ static int coroutine_fn nbd_co_receive_request(NBDRequestData *req, break;
case NBD_CMD_BLOCK_STATUS: - request->contexts = &client->contexts; + if (extended_with_payload) { + ret = nbd_co_block_status_payload_read(client, request, errp); + if (ret < 0) { + return ret; + } + /* payload now consumed */ + check_length = extended_with_payload = false;
why set extended_with_payload to false? it's a bit misleading. And you don't do this for WRITE request.
Indeed; it doesn't make any different to later in the function. Will drop.
+ payload_len = 0; + valid_flags |= NBD_CMD_FLAG_PAYLOAD_LEN; + } else { + request->contexts = &client->contexts; + } valid_flags |= NBD_CMD_FLAG_REQ_ONE; break;
[..]
with payload_len changed to uint64_t, your squash-in applied and s/>/>=/ fixed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
Thanks for the careful review. -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org