Re: [PATCH v6 04/17] nbd: Prepare for 64-bit request effect lengths

[Eric Blake]

On Tue, Sep 05, 2023 at 05:41:33PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > @@ -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).
> > 
> 
> I miss something.. Why it's an error message? It's may be a simple reply for 
> CMD_READ, where len is request->len

Oh; I was confusing this with the length of an error message; but when
an error message is sent over the wire, it's done with
nbd_co_send_chunk_error().  And a quick audit only shows two callers
of nbd_co_send_simple_reply(): nbd_send_generic_reply() which only
uses it with data/len NULL/0; and nbd_do_cmd_read() does indeed use up
to MAX_BUFFER_SIZE when structured mode was not negotiated.  Our unit
tests aren't covering interoperability with clients that don't request
structured replies, so I wasn't seeing the problem with just 'make
check' or any of the iotests; but libnbd's testsuite does do interop
testing, and there I was able to trip this assertion.  I'll fix the
assertion.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org