[v3,2/4] bitmap: Enforce maximum bitmap name length
diff mbox series

Message ID 20191114024635.11363-3-eblake@redhat.com
State New
Headers show
Series
  • Better NBD string length handling
Related show

Commit Message

Eric Blake Nov. 14, 2019, 2:46 a.m. UTC
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.

Signed-off-by: Eric Blake <eblake@redhat.com>
---
 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(-)

Comments

Maxim Levitsky Nov. 14, 2019, 10:04 a.m. UTC | #1
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 <eblake@redhat.com>
> ---
>  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 <mlevitsk@redhat.com>

Best regards,
	Maxim Levitsky
Vladimir Sementsov-Ogievskiy Nov. 15, 2019, 3:04 p.m. UTC | #2
14.11.2019 5:46, 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.
> 
> Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Vladimir Sementsov-Ogievskiy Nov. 15, 2019, 3:47 p.m. UTC | #3
15.11.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
> 14.11.2019 5:46, 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.
>>
>> Signed-off-by: Eric Blake <eblake@redhat.com>
> 
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 
> 

One doubt:

Is it good idea to include string larger than 4K into error message
(in next patch too)? I doubt that such message would be
readable, and I think that most possible source of such message is
some kind of memory corruption, so the whole message would be garbage,
which may contain special symbols which may look bad or even break
output.
Eric Blake Nov. 15, 2019, 4:33 p.m. UTC | #4
On 11/15/19 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> 15.11.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
>> 14.11.2019 5:46, 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.
>>>
>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>>
> 
> One doubt:
> 
> Is it good idea to include string larger than 4K into error message
> (in next patch too)? I doubt that such message would be
> readable, and I think that most possible source of such message is
> some kind of memory corruption, so the whole message would be garbage,
> which may contain special symbols which may look bad or even break
> output.

The string was provided by the user. You are correct that it results in 
a lot of output on stderr, but it is no more garbage than what the user 
provided in the first place. If we wanted, we could truncate (list only 
the first 256 or so bytes and then output "..."), but it's such a corner 
case error that I don't think it's worth the effort to worry about it.
Vladimir Sementsov-Ogievskiy Nov. 15, 2019, 5:09 p.m. UTC | #5
15.11.2019 19:33, Eric Blake wrote:
> On 11/15/19 9:47 AM, Vladimir Sementsov-Ogievskiy wrote:
>> 15.11.2019 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>> 14.11.2019 5:46, 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.
>>>>
>>>> Signed-off-by: Eric Blake <eblake@redhat.com>
>>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>
>>>
>>
>> One doubt:
>>
>> Is it good idea to include string larger than 4K into error message
>> (in next patch too)? I doubt that such message would be
>> readable, and I think that most possible source of such message is
>> some kind of memory corruption, so the whole message would be garbage,
>> which may contain special symbols which may look bad or even break
>> output.
> 
> The string was provided by the user. You are correct that it results in a lot of output on stderr, but it is no more garbage than what the user provided in the first place. If we wanted, we could truncate (list only the first 256 or so bytes and then output "..."), but it's such a corner case error that I don't think it's worth the effort to worry about it.
> 

OK

Patch
diff mbox series

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