[Qemu-devel] [PATCH] block: update string sizes for filename, backing_file, exact_filename

[Qemu-devel] [PATCH] block: update string sizes for filename, backing_file, exact_filename

[Jeff Cody]

The string field entries 'filename', 'backing_file', and
'exact_filename' in the BlockDriverState struct are defined as 1024
bytes.

However, most places that use these values accept a maximum of PATH_MAX
bytes.  This patch makes the BlockDriverStruct field string sizes match
the most common usage.

This patch also updates two block drivers that still use 1024-byte sized
arrays for 'backing_file'.

Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/mirror.c            | 2 +-
 block/qapi.c              | 2 +-
 include/block/block_int.h | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 9019d1b..57154eb 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -378,7 +378,7 @@ static void coroutine_fn mirror_run(void *opaque)
     int64_t sector_num, end, sectors_per_chunk, length;
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
-    char backing_filename[1024];
+    char backing_filename[PATH_MAX];
     int ret = 0;
     int n;
 
diff --git a/block/qapi.c b/block/qapi.c
index a6fd6f7..c097238 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
 {
     int64_t size;
     const char *backing_filename;
-    char backing_filename2[1024];
+    char backing_filename2[PATH_MAX];
     BlockDriverInfo bdi;
     int ret;
     Error *err = NULL;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 06a21dd..e264be9 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -339,13 +339,13 @@ struct BlockDriverState {
      * regarding this BDS's context */
     QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
 
-    char filename[1024];
-    char backing_file[1024]; /* if non zero, the image is a diff of
-                                this file image */
+    char filename[PATH_MAX];
+    char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
+                                    this file image */
     char backing_format[16]; /* if non-zero and backing_file exists */
 
     QDict *full_open_options;
-    char exact_filename[1024];
+    char exact_filename[PATH_MAX];
 
     BlockDriverState *backing_hd;
     BlockDriverState *file;
-- 
1.9.3

Re: [Qemu-devel] [PATCH] block: update string sizes for filename, backing_file, exact_filename

[John Snow]

On 01/13/2015 12:03 PM, Jeff Cody wrote:
> The string field entries 'filename', 'backing_file', and
> 'exact_filename' in the BlockDriverState struct are defined as 1024
> bytes.
>
> However, most places that use these values accept a maximum of PATH_MAX
> bytes.  This patch makes the BlockDriverStruct field string sizes match
> the most common usage.
>
> This patch also updates two block drivers that still use 1024-byte sized
> arrays for 'backing_file'.
>
> Signed-off-by: Jeff Cody <jcody@redhat.com>
> ---
>   block/mirror.c            | 2 +-
>   block/qapi.c              | 2 +-
>   include/block/block_int.h | 8 ++++----
>   3 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 9019d1b..57154eb 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -378,7 +378,7 @@ static void coroutine_fn mirror_run(void *opaque)
>       int64_t sector_num, end, sectors_per_chunk, length;
>       uint64_t last_pause_ns;
>       BlockDriverInfo bdi;
> -    char backing_filename[1024];
> +    char backing_filename[PATH_MAX];
>       int ret = 0;
>       int n;
>
> diff --git a/block/qapi.c b/block/qapi.c
> index a6fd6f7..c097238 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
>   {
>       int64_t size;
>       const char *backing_filename;
> -    char backing_filename2[1024];
> +    char backing_filename2[PATH_MAX];
>       BlockDriverInfo bdi;
>       int ret;
>       Error *err = NULL;
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 06a21dd..e264be9 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -339,13 +339,13 @@ struct BlockDriverState {
>        * regarding this BDS's context */
>       QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
>
> -    char filename[1024];
> -    char backing_file[1024]; /* if non zero, the image is a diff of
> -                                this file image */
> +    char filename[PATH_MAX];
> +    char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
> +                                    this file image */
>       char backing_format[16]; /* if non-zero and backing_file exists */
>
>       QDict *full_open_options;
> -    char exact_filename[1024];
> +    char exact_filename[PATH_MAX];
>
>       BlockDriverState *backing_hd;
>       BlockDriverState *file;
>

Is it important that qcow2_open seems to enforce a 1023-char length 
backing_file name?

 From qcow2.c, qcow2_open (currently line ~871):
         if (len > MIN(1023, s->cluster_size - 
header.backing_file_offset)) {

Re: [Qemu-devel] [PATCH] block: update string sizes for filename, backing_file, exact_filename

[Kevin Wolf]

Am 13.01.2015 um 21:49 hat John Snow geschrieben:
> 
> 
> On 01/13/2015 12:03 PM, Jeff Cody wrote:
> >The string field entries 'filename', 'backing_file', and
> >'exact_filename' in the BlockDriverState struct are defined as 1024
> >bytes.
> >
> >However, most places that use these values accept a maximum of PATH_MAX
> >bytes.  This patch makes the BlockDriverStruct field string sizes match
> >the most common usage.
> >
> >This patch also updates two block drivers that still use 1024-byte sized
> >arrays for 'backing_file'.
> >
> >Signed-off-by: Jeff Cody <jcody@redhat.com>
> >---
> >  block/mirror.c            | 2 +-
> >  block/qapi.c              | 2 +-
> >  include/block/block_int.h | 8 ++++----
> >  3 files changed, 6 insertions(+), 6 deletions(-)
> >
> >diff --git a/block/mirror.c b/block/mirror.c
> >index 9019d1b..57154eb 100644
> >--- a/block/mirror.c
> >+++ b/block/mirror.c
> >@@ -378,7 +378,7 @@ static void coroutine_fn mirror_run(void *opaque)
> >      int64_t sector_num, end, sectors_per_chunk, length;
> >      uint64_t last_pause_ns;
> >      BlockDriverInfo bdi;
> >-    char backing_filename[1024];
> >+    char backing_filename[PATH_MAX];
> >      int ret = 0;
> >      int n;
> >
> >diff --git a/block/qapi.c b/block/qapi.c
> >index a6fd6f7..c097238 100644
> >--- a/block/qapi.c
> >+++ b/block/qapi.c
> >@@ -175,7 +175,7 @@ void bdrv_query_image_info(BlockDriverState *bs,
> >  {
> >      int64_t size;
> >      const char *backing_filename;
> >-    char backing_filename2[1024];
> >+    char backing_filename2[PATH_MAX];
> >      BlockDriverInfo bdi;
> >      int ret;
> >      Error *err = NULL;

We shouldn't have had paths on the stack before this patch, and after
increasing the array size, we should have them even less.

Actually, mirror_run() could probably do with a char[2] or even access
bs->backing_file directly without copying things around. It has that
array only so it can check that backing_filename isn't empty.

> >diff --git a/include/block/block_int.h b/include/block/block_int.h
> >index 06a21dd..e264be9 100644
> >--- a/include/block/block_int.h
> >+++ b/include/block/block_int.h
> >@@ -339,13 +339,13 @@ struct BlockDriverState {
> >       * regarding this BDS's context */
> >      QLIST_HEAD(, BdrvAioNotifier) aio_notifiers;
> >
> >-    char filename[1024];
> >-    char backing_file[1024]; /* if non zero, the image is a diff of
> >-                                this file image */
> >+    char filename[PATH_MAX];
> >+    char backing_file[PATH_MAX]; /* if non zero, the image is a diff of
> >+                                    this file image */
> >      char backing_format[16]; /* if non-zero and backing_file exists */
> >
> >      QDict *full_open_options;
> >-    char exact_filename[1024];
> >+    char exact_filename[PATH_MAX];
> >
> >      BlockDriverState *backing_hd;
> >      BlockDriverState *file;
> >
> 
> Is it important that qcow2_open seems to enforce a 1023-char length
> backing_file name?
> 
> From qcow2.c, qcow2_open (currently line ~871):
>         if (len > MIN(1023, s->cluster_size -
> header.backing_file_offset)) {

It is relevant for PATH_MAX < 1023. In such cases we have a buffer
overflow now.

For cases where PATH_MAX > 1023: The limitation has become part of the
file format definition, so we can't remove it (otherwise older qemu
versions wouldn't be able to read the image).

Kevin