Re: [Qemu-devel] [PATCH v3 29/44] nbd: Avoid magic number for NBD max name size

[Alex Bligh]

On 23 Apr 2016, at 00:40, Eric Blake <address@hidden> wrote:

> Declare a constant and use that when determining if an export
> name fits within the constraints we are willing to support.
> 
> Note that upstream NBD recently documented that clients MUST
> support export names of 256 bytes (not including trailing NUL),
> and SHOULD support names up to 4096 bytes.  4096 is a bit big
> (we would lose benefits of stack-allocation of a name array),
> and we already have other limits in place (for example, qcow2
> snapshot names are clamped around 1024).  So for now, just
> stick to the required minimum.
> 
> Signed-off-by: Eric Blake <address@hidden>
Reviewed-by: Alex Bligh <address@hidden>
> 
> ---
> v3: enlarge the limit, and document choice of new value
> ---
> include/block/nbd.h | 6 ++++++
> nbd/client.c        | 2 +-
> nbd/server.c        | 4 ++--
> 3 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 3e2d76b..2c753cc 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -76,6 +76,12 @@ enum {
> 
> /* Maximum size of a single READ/WRITE data buffer */
> #define NBD_MAX_BUFFER_SIZE (32 * 1024 * 1024)
> +/* Maximum size of an export name. The NBD spec requires 256 and
> + * suggests that servers support up to 4096, but we stick to only the
> + * required size so that we can stack-allocate the names, and because
> + * going larger would require an audit of more code to make sure we
> + * aren't overflowing some other buffer. */
> +#define NBD_MAX_NAME_SIZE 256
> 
> ssize_t nbd_wr_syncv(QIOChannel *ioc,
>                      struct iovec *iov,
> diff --git a/nbd/client.c b/nbd/client.c
> index 4659df3..b700100 100644
> --- a/nbd/client.c
> +++ b/nbd/client.c
> @@ -210,7 +210,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, 
> Error **errp)
>             error_setg(errp, "incorrect option name length");
>             return -1;
>         }
> -        if (namelen > 255) {
> +        if (namelen > NBD_MAX_NAME_SIZE) {
>             error_setg(errp, "export name length too long %" PRIu32, namelen);
>             return -1;
>         }
> diff --git a/nbd/server.c b/nbd/server.c
> index fa05a73..a20bf8a 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -296,13 +296,13 @@ static int nbd_negotiate_handle_list(NBDClient *client, 
> uint32_t length)
> static int nbd_negotiate_handle_export_name(NBDClient *client, uint32_t 
> length)
> {
>     int rc = -EINVAL;
> -    char name[256];
> +    char name[NBD_MAX_NAME_SIZE + 1];
> 
>     /* Client sends:
>         [20 ..  xx]   export name (length bytes)
>      */
>     TRACE("Checking length");
> -    if (length > 255) {
> +    if (length >= sizeof(name)) {
>         LOG("Bad length received");
>         goto fail;
>     }
> -- 
> 2.5.5
> 
> 

-- 
Alex Bligh