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 <vsement...@yandex-team.ru>
Thanks for the careful review.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
_______________________________________________
Libguestfs mailing list
Libguestfs@redhat.com
https://listman.redhat.com/mailman/listinfo/libguestfs
