Re: [PATCH v3 2/4] bitmap: Enforce maximum bitmap name length

[Maxim Levitsky]

On Wed, 2019-11-13 at 20:46 -0600, Eric Blake wrote:
> We document that for qcow2 persistent bitmaps, the name cannot exceed
> 1023 bytes.  It is inconsistent if transient bitmaps do not have to
> abide by the same limit, and it is unlikely that any existing client
> even cares about using bitmap names this long.  It's time to codify
> that ALL bitmaps managed by qemu (whether persistent in qcow2 or not)
> have a documented maximum length.

Strange a bit that 1023 was choosen, I guess implementation
uses a 1024 zero terminated string for storing the names
in memory.

> 
> Signed-off-by: Eric Blake <address@hidden>
> ---
>  qapi/block-core.json         |  2 +-
>  include/block/dirty-bitmap.h |  2 ++
>  block/dirty-bitmap.c         | 12 +++++++++---
>  block/qcow2-bitmap.c         |  2 ++
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index aa97ee264112..0cf68fea1450 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2042,7 +2042,7 @@
>  #
>  # @node: name of device/node which the bitmap is tracking
>  #
> -# @name: name of the dirty bitmap
> +# @name: name of the dirty bitmap (must be less than 1024 bytes)
>  #
>  # @granularity: the bitmap granularity, default is 64k for
>  #               block-dirty-bitmap-add
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 958e7474fb51..e2b20ecab9a3 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h
> @@ -14,6 +14,8 @@ typedef enum BitmapCheckFlags {
>                               BDRV_BITMAP_INCONSISTENT)
>  #define BDRV_BITMAP_ALLOW_RO (BDRV_BITMAP_BUSY | BDRV_BITMAP_INCONSISTENT)
> 
> +#define BDRV_BITMAP_MAX_NAME_SIZE 1023
> +
>  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
>                                            uint32_t granularity,
>                                            const char *name,
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 4bbb251b2c9c..7039e8252009 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -104,9 +104,15 @@ BdrvDirtyBitmap 
> *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> 
>      assert(is_power_of_2(granularity) && granularity >= BDRV_SECTOR_SIZE);
> 
> -    if (name && bdrv_find_dirty_bitmap(bs, name)) {
> -        error_setg(errp, "Bitmap already exists: %s", name);
> -        return NULL;
> +    if (name) {
> +        if (bdrv_find_dirty_bitmap(bs, name)) {
> +            error_setg(errp, "Bitmap already exists: %s", name);
> +            return NULL;
> +        }
> +        if (strlen(name) > BDRV_BITMAP_MAX_NAME_SIZE) {
> +            error_setg(errp, "Bitmap name too long: %s", name);
> +            return NULL;
> +        }
>      }
>      bitmap_size = bdrv_getlength(bs);
>      if (bitmap_size < 0) {
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index ef9ef628a0d0..809bbc5d20c8 100644
> --- a/block/qcow2-bitmap.c
> +++ b/block/qcow2-bitmap.c
> @@ -42,6 +42,8 @@
>  #define BME_MIN_GRANULARITY_BITS 9
>  #define BME_MAX_NAME_SIZE 1023
> 
> +QEMU_BUILD_BUG_ON(BME_MAX_NAME_SIZE != BDRV_BITMAP_MAX_NAME_SIZE);
> +
>  #if BME_MAX_TABLE_SIZE * 8ULL > INT_MAX
>  #error In the code bitmap table physical size assumed to fit into int
>  #endif


Reviewed-by: Maxim Levitsky <address@hidden>

Best regards,
        Maxim Levitsky