On Mon, Sep 04, 2023 at 07:15:04PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 29.08.23 20:58, Eric Blake wrote:
> > Widen the length field of NBDRequest to 64-bits, although we can
> > assert that all current uses are still under 32 bits: either because
> > of NBD_MAX_BUFFER_SIZE which is even smaller (and where size_t can
> > still be appropriate, even on 32-bit platforms), or because nothing
> > ever puts us into NBD_MODE_EXTENDED yet (and while future patches will
> > allow larger transactions, the lengths in play here are still capped
> > at 32-bit). There are no semantic changes, other than a typo fix in a
> > couple of error messages.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> > ---
> >
> > v6: fix sign extension bug
> >
> > v5: tweak commit message, adjust a few more spots [Vladimir]
> >
> > v4: split off enum changes to earlier patches [Vladimir]
>
> [..]
>
> > --- a/nbd/server.c
> > +++ b/nbd/server.c
> > @@ -1165,7 +1165,7 @@ static int nbd_negotiate_options(NBDClient *client,
> > Error **errp)
> > client->optlen = length;
> >
> > if (length > NBD_MAX_BUFFER_SIZE) {
> > - error_setg(errp, "len (%" PRIu32" ) is larger than max len
> > (%u)",
> > + error_setg(errp, "len (%" PRIu32 ") is larger than max len
> > (%u)",
> > length, NBD_MAX_BUFFER_SIZE);
> > return -EINVAL;
> > }
> > @@ -1441,7 +1441,7 @@ static int coroutine_fn nbd_receive_request(NBDClient
> > *client, NBDRequest *reque
> > request->type = lduw_be_p(buf + 6);
> > request->cookie = ldq_be_p(buf + 8);
> > request->from = ldq_be_p(buf + 16);
> > - request->len = ldl_be_p(buf + 24);
> > + request->len = (uint32_t)ldl_be_p(buf + 24); /* widen 32 to 64 bits
> > */
>
> should it be "(uint64_t)" ?
No. ldl_be_p() returns an int. Widening int to 64 bits sign-extends;
we have to first make it unsigned (by casting to uint32_t) so that we
then have an unsigned value that widens by zero-extension.
Maybe we should fix ldl_bd_p() and friends to favor unsigned values.
'git grep ldul_be' has zero hits; even though Peter just touched the
docs patch claiming it exists:
https://lists.gnu.org/archive/html/qemu-devel/2023-09/msg00645.html
>
> >
> > trace_nbd_receive_request(magic, request->flags, request->type,
> > request->from, request->len);
> > @@ -1899,7 +1899,7 @@ static int coroutine_fn
> > nbd_co_send_simple_reply(NBDClient *client,
> > NBDRequest *request,
> > uint32_t error,
> > void *data,
> > - size_t len,
> > + uint64_t len,
> > Error **errp)
> > {
> > NBDSimpleReply reply;
> > @@ -1910,6 +1910,7 @@ static int coroutine_fn
> > nbd_co_send_simple_reply(NBDClient *client,
> > };
> >
> > assert(!len || !nbd_err);
> > + assert(len <= NBD_MAX_STRING_SIZE);
>
> NBD_MAX_BUFFER_SIZE ?
No. MAX_STRING_SIZE is 4k, MAX_BUFFER_SIZE is 32M. The smaller size
is the correct bound here (an error message has to be transmitted as a
single string, and the recipient does not expect more than a 4k error
message).
>
> with that fixed:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
