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 <ebl...@redhat.com>
> ---
> 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 <mlevi...@redhat.com>
Best regards,
Maxim Levitsky
