Re: [PATCH v3 1/4] nbd/server: Prefer heap over stack for parsing client names

[Maxim Levitsky]

On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> As long as we limit NBD names to 256 bytes (the bare minimum permitted
> by the standard), stack-allocation works for parsing a name received
> from the client.  But as mentioned in a comment, we eventually want to
> permit up to the 4k maximum of the NBD standard, which is too large
> for stack allocation; so switch everything in the server to use heap
> allocation.  For now, there is no change in actually supported name
> length.

I am just curios, why is this so?
I know that kernel uses 8K stacks due to historical limitation
of 1:1 physical memory mapping which creates fragmentation,
but in the userspace stacks shouldn't really be limited and grow on demand.
Some gcc security option limits this?

> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  include/block/nbd.h | 10 +++++-----
>  nbd/server.c        | 25 +++++++++++++++----------
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index 316fd705a9e4..c306423dc85c 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -226,11 +226,11 @@ 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. */
> +/*
> + * Maximum size of an export name. The NBD spec requires a minimum of
> + * 256 and recommends that servers support up to 4096; all users use
> + * malloc so we can bump this constant without worry.
> + */
>  #define NBD_MAX_NAME_SIZE 256
> 
>  /* Two types of reply structures */
> diff --git a/nbd/server.c b/nbd/server.c
> index d8d1e6245532..c63b76b22735 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -324,18 +324,20 @@ static int nbd_opt_skip(NBDClient *client, size_t size, 
> Error **errp)
>   *   uint32_t len     (<= NBD_MAX_NAME_SIZE)
>   *   len bytes string (not 0-terminated)
>   *
> - * @name should be enough to store NBD_MAX_NAME_SIZE+1.
> + * On success, @name will be allocated.
>   * If @length is non-null, it will be set to the actual string length.
>   *
>   * Return -errno on I/O error, 0 if option was completely handled by
>   * sending a reply about inconsistent lengths, or 1 on success.
>   */
> -static int nbd_opt_read_name(NBDClient *client, char *name, uint32_t *length,
> +static int nbd_opt_read_name(NBDClient *client, char **name, uint32_t 
> *length,
>                               Error **errp)
>  {
>      int ret;
>      uint32_t len;
> +    g_autofree char *local_name = NULL;
> 
> +    *name = NULL;
>      ret = nbd_opt_read(client, &len, sizeof(len), errp);
>      if (ret <= 0) {
>          return ret;
> @@ -347,15 +349,17 @@ static int nbd_opt_read_name(NBDClient *client, char 
> *name, uint32_t *length,
>                                 "Invalid name length: %" PRIu32, len);
>      }
> 
> -    ret = nbd_opt_read(client, name, len, errp);
> +    local_name = g_malloc(len + 1);
> +    ret = nbd_opt_read(client, local_name, len, errp);
>      if (ret <= 0) {
>          return ret;
>      }
> -    name[len] = '\0';
> +    local_name[len] = '\0';
> 
>      if (length) {
>          *length = len;
>      }
> +    *name = g_steal_pointer(&local_name);
> 
>      return 1;
>  }
> @@ -427,7 +431,7 @@ static void nbd_check_meta_export(NBDClient *client)
>  static int nbd_negotiate_handle_export_name(NBDClient *client, bool 
> no_zeroes,
>                                              Error **errp)
>  {
> -    char name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *name;

That is what patchew complained about I think.

Isn't it wonderful how g_autofree fixes one issue
and introduces another. I mean 'name' isn't really
used here prior to allocation according to plain C,
but due to g_autofree, it can be now on any error
path. Nothing against g_autofree though, just noting this.

>      char buf[NBD_REPLY_EXPORT_NAME_SIZE] = "";
>      size_t len;
>      int ret;
> @@ -441,10 +445,11 @@ static int nbd_negotiate_handle_export_name(NBDClient 
> *client, bool no_zeroes,
>          [10 .. 133]   reserved     (0) [unless no_zeroes]
>       */
>      trace_nbd_negotiate_handle_export_name();
> -    if (client->optlen >= sizeof(name)) {
> +    if (client->optlen > NBD_MAX_NAME_SIZE) {
>          error_setg(errp, "Bad length received");
>          return -EINVAL;
>      }
> +    name = g_malloc(client->optlen + 1);
>      if (nbd_read(client->ioc, name, client->optlen, "export name", errp) < 
> 0) {
>          return -EIO;
>      }
> @@ -533,7 +538,7 @@ static int nbd_reject_length(NBDClient *client, bool 
> fatal, Error **errp)
>  static int nbd_negotiate_handle_info(NBDClient *client, Error **errp)
>  {
>      int rc;
> -    char name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *name = NULL;
>      NBDExport *exp;
>      uint16_t requests;
>      uint16_t request;
> @@ -551,7 +556,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
> Error **errp)
>          2 bytes: N, number of requests (can be 0)
>          N * 2 bytes: N requests
>      */
> -    rc = nbd_opt_read_name(client, name, &namelen, errp);
> +    rc = nbd_opt_read_name(client, &name, &namelen, errp);
>      if (rc <= 0) {
>          return rc;
>      }
> @@ -957,7 +962,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
>                                        NBDExportMetaContexts *meta, Error 
> **errp)
>  {
>      int ret;
> -    char export_name[NBD_MAX_NAME_SIZE + 1];
> +    g_autofree char *export_name = NULL;
>      NBDExportMetaContexts local_meta;
>      uint32_t nb_queries;
>      int i;
> @@ -976,7 +981,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
> 
>      memset(meta, 0, sizeof(*meta));
> 
> -    ret = nbd_opt_read_name(client, export_name, NULL, errp);
> +    ret = nbd_opt_read_name(client, &export_name, NULL, errp);
>      if (ret <= 0) {
>          return ret;
>      }

Looks correct, but I might have missed something.

Reviewed-by: Maxim Levitsky <address@hidden>

Best regards,
        Maxim Levitsky