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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 17/10/22 21: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);
>       }

Nitpicking (pre-existing) the assertions could be moved before
the assignations.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>