Re: [PATCH v2] nbd/client: Use smarter assert

[Eric Blake]

On Mon, Oct 24, 2022 at 02:59:48PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> On 10/17/22 22:12, Eric Blake wrote:
> > Assigning strlen() to a uint32_t and then asserting that it isn't too
> > large doesn't catch the case of an input string 4G in length.
> > Thankfully, the incoming strings can never be that large: if the
> > export name or query is reflecting a string the client got from the
> > server, we already guarantee that we dropped the NBD connection if the
> > server sent more than 32M in a single reply to our NBD_OPT_* request;
> > if the export name is coming from qemu, nbd_receive_negotiate()
> > asserted that strlen(info->name) <= NBD_MAX_STRING_SIZE; and
> > similarly, a query string via x->dirty_bitmap coming from the user was
> > bounds-checked in either qemu-nbd or by the limitations of QMP.
> > Still, it doesn't hurt to be more explicit in how we write our
> > assertions to not have to analyze whether inadvertent wraparound is
> > possible.
> > 
> > Fixes: 93676c88 ("nbd: Don't send oversize strings", v4.2.0)
> > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Eric Blake <eblake@redhat.com>
> > ---
> > 
> > v2: update subject line and commit message to reflect file being
> > touched; adjust a second nearby assertion with the same issue
> > 
> >   nbd/client.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/nbd/client.c b/nbd/client.c
> > index 30d5383cb1..90a6b7b38b 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -658,11 +658,11 @@ static int nbd_send_meta_query(QIOChannel *ioc, 
> > uint32_t opt,
> >       char *p;
> > 
> >       data_len = sizeof(export_len) + export_len + sizeof(queries);
> > -    assert(export_len <= NBD_MAX_STRING_SIZE);
> > +    assert(strlen(export) <= NBD_MAX_STRING_SIZE);
> >       if (query) {
> >           query_len = strlen(query);
> >           data_len += sizeof(query_len) + query_len;
> > -        assert(query_len <= NBD_MAX_STRING_SIZE);
> > +        assert(strlen(query) <= NBD_MAX_STRING_SIZE);
> >       } else {
> >           assert(opt == NBD_OPT_LIST_META_CONTEXT);
> >       }
> 
> I'm a bit late, and this should work as is.
> 
> Still, for me it's a bit strange: you point to the fact that we probably 
> overflow uint32_t variable. But we keep this fact hidden in the code. So, 
> everyone who read should guess "aha, this extra strlen here is because the 
> previous one may overflow the variable)".
> 
> Could we use strnlen() instead of strlen()? That would be also more effective.

Good idea.  As in:

assert(strnlen(query, NBD_MAX_STRING_SIZE + 1) <= NBD_MAX_STRING_SIZE);

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org