[PATCH 0/4] block/vvfat: Fix leaks and out of bounds writes

[PATCH 0/4] block/vvfat: Fix leaks and out of bounds writes

[Philippe Mathieu-Daudé]

The first 3 patches are trivial leak fixes, the last
one is a RFC since I have no clue about this code.

Johannes, you wrote this 18 years ago, do you still
remember? =)

Philippe Mathieu-Daudé (4):
  block/vvfat: Fix leak of BDRVVVFATState::qcow_filename
  block/vvfat: Fix leak of BDRVVVFATState::used_clusters
  block/vvfat: Fix leak of mapping_t::path
  block/vvfat: Avoid out of bounds write in create_long_filename()

 block/vvfat.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

-- 
2.26.3

[PATCH 1/4] block/vvfat: Fix leak of BDRVVVFATState::qcow_filename

[Philippe Mathieu-Daudé]

qcow_filename is allocated in enable_write_target(), called by
vvfat_open(), but never free'd. Free it in vvfat_close().

This fixes (QEMU built with --enable-sanitizers):

  Direct leak of 4096 byte(s) in 1 object(s) allocated from:
      #0 0x55d7a363773f in malloc (/mnt/scratch/qemu/sanitizer/qemu-system-x86_64+0x1dab73f)
      #1 0x7f55c6447958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
      #2 0x55d7a5e123aa in vvfat_open block/vvfat.c:1236:19
      #3 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
      #4 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
      #5 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
      #6 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
      #7 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
      #8 0x55d7a5a65da3 in bdrv_open block.c:3537:12
      #9 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
      #10 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
      #11 0x55d7a5a088e7 in drive_new blockdev.c:994:11
      #12 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
      #13 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
      #14 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
      #15 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
      #16 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
      #17 0x55d7a366f619 in main softmmu/main.c:49:5

Fixes: 8475ea48544 ("block/vvfat: Do not unref qcow on closing backing bdrv")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vvfat.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index 54807f82ca1..5a4a7915220 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3233,6 +3233,7 @@ static void vvfat_close(BlockDriverState *bs)
     array_free(&(s->directory));
     array_free(&(s->mapping));
     g_free(s->cluster_buffer);
+    g_free(s->qcow_filename);
 
     if (s->qcow) {
         migrate_del_blocker(s->migration_blocker);
-- 
2.26.3

[PATCH 2/4] block/vvfat: Fix leak of BDRVVVFATState::used_clusters

[Philippe Mathieu-Daudé]

used_clusters is allocated in enable_write_target(), called by
vvfat_open(), but never free'd.
Allocate it using GLib API, and free it in vvfat_close().

This fixes (QEMU built with --enable-sanitizers):

  Direct leak of 64508 byte(s) in 1 object(s) allocated from:
      #0 0x55d7a36378f7 in calloc (qemu-system-x86_64+0x1dab8f7)
      #1 0x55d7a5e14246 in enable_write_target block/vvfat.c:3145:24
      #2 0x55d7a5e123aa in vvfat_open block/vvfat.c:1236:19
      #3 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
      #4 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
      #5 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
      #6 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
      #7 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
      #8 0x55d7a5a65da3 in bdrv_open block.c:3537:12
      #9 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
      #10 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
      #11 0x55d7a5a088e7 in drive_new blockdev.c:994:11
      #12 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
      #13 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
      #14 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
      #15 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
      #16 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
      #17 0x55d7a366f619 in main softmmu/main.c:49:5

Fixes: a046433a161 ("Major overhaul of the virtual FAT driver for read/write support")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vvfat.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 5a4a7915220..2cc21787600 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3142,7 +3142,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
     int size = sector2cluster(s, s->sector_count);
     QDict *options;
 
-    s->used_clusters = calloc(size, 1);
+    s->used_clusters = g_malloc0(size);
 
     array_init(&(s->commits), sizeof(commit_t));
 
@@ -3233,6 +3233,7 @@ static void vvfat_close(BlockDriverState *bs)
     array_free(&(s->directory));
     array_free(&(s->mapping));
     g_free(s->cluster_buffer);
+    g_free(s->used_clusters);
     g_free(s->qcow_filename);
 
     if (s->qcow) {
-- 
2.26.3

[PATCH 3/4] block/vvfat: Fix leak of mapping_t::path

[Philippe Mathieu-Daudé]

read_directory() keeps pointers to alloc'ed data in path ...:

 743 static int read_directory(BDRVVVFATState* s, int mapping_index)
 744 {
 ...
 792         buffer = g_malloc(length);
 ...
 828         /* create mapping for this file */
 829         if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
 830             s->current_mapping = array_get_next(&(s->mapping));
 ...
 847             s->current_mapping->path=buffer;

... but these pointers are never free'd. Free them in vvfat_close(),
to fix (QEMU built with --enable-sanitizers):

  Direct leak of 148 byte(s) in 6 object(s) allocated from:
    #0 0x55d7a363773f in malloc (qemu-system-x86_64+0x1dab73f)
    #1 0x7f55c6447958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
    #2 0x55d7a5e17679 in init_directories block/vvfat.c:962:16
    #3 0x55d7a5e1251e in vvfat_open block/vvfat.c:1255:9
    #4 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
    #5 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
    #6 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
    #7 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
    #8 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
    #9 0x55d7a5a65da3 in bdrv_open block.c:3537:12
    #10 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
    #11 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
    #12 0x55d7a5a088e7 in drive_new blockdev.c:994:11
    #13 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
    #14 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
    #15 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
    #16 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
    #17 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
    #18 0x55d7a366f619 in main softmmu/main.c:49:5

Fixes: a046433a161 ("Major overhaul of the virtual FAT driver for read/write support")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vvfat.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index 2cc21787600..c193a816646 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -3228,6 +3228,11 @@ static void vvfat_close(BlockDriverState *bs)
 {
     BDRVVVFATState *s = bs->opaque;
 
+    for (unsigned j = 0; j < s->mapping.next; j++) {
+        mapping_t *mapping = array_get(&(s->mapping), j);
+
+        g_free(mapping->path);
+    }
     vvfat_close_current_file(s);
     array_free(&(s->fat));
     array_free(&(s->directory));
-- 
2.26.3

[PATCH 4/4] block/vvfat: Avoid out of bounds write in create_long_filename()

[Philippe Mathieu-Daudé]

The direntry_t::name holds 11 bytes:

  typedef struct direntry_t {
      uint8_t name[8 + 3];
      ...

However create_long_filename() writes up to 31 bytes into it:

 421     for(i=0;i<26*number_of_entries;i++) {
 422         int offset=(i%26);
 423         if(offset<10) offset=1+offset;
 424         else if(offset<22) offset=14+offset-10;
 425         else offset=28+offset-22;
 426         entry=array_get(&(s->directory),s->directory.next-1-(i/26));
 427         if (i >= 2 * length + 2) {
 428             entry->name[offset] = 0xff;
 429         } else if (i % 2 == 0) {
 430             entry->name[offset] = longname[i / 2] & 0xff;
 431         } else {
 432             entry->name[offset] = longname[i / 2] >> 8;
 433         }
 434     }

For example, if i=25, offset=28+25-22=31

Then in lines 428, 430 and 432 the entry->name[] array is written beside
its 11 bytes, as reported by Clang sanitizer:

  block/vvfat.c:430:13: runtime error: index 14 out of bounds for type 'uint8_t [11]'
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior block/vvfat.c:430:13 in
  block/vvfat.c:432:13: runtime error: index 15 out of bounds for type 'uint8_t [11]'
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior block/vvfat.c:432:13 in
  block/vvfat.c:428:13: runtime error: index 18 out of bounds for type 'uint8_t [11]'
  SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior block/vvfat.c:428:13 in

As I have no idea about what this code does, simply skip the writes if
out of range, since it is not worst than what we have currently (and
my tests using vvfat work identically).

Fixes: de167e416fa ("Virtual VFAT support (Johannes Schindelin)")
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 block/vvfat.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index c193a816646..c7162e77d68 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -423,6 +423,9 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
         if(offset<10) offset=1+offset;
         else if(offset<22) offset=14+offset-10;
         else offset=28+offset-22;
+        if (offset >= ARRAY_SIZE(entry->name)) {
+            continue;
+        }
         entry=array_get(&(s->directory),s->directory.next-1-(i/26));
         if (i >= 2 * length + 2) {
             entry->name[offset] = 0xff;
-- 
2.26.3

Re: [PATCH 1/4] block/vvfat: Fix leak of BDRVVVFATState::qcow_filename

[Stefano Garzarella]

On Fri, Apr 30, 2021 at 06:25:16PM +0200, Philippe Mathieu-Daudé wrote:
>qcow_filename is allocated in enable_write_target(), called by
>vvfat_open(), but never free'd. Free it in vvfat_close().
>
>This fixes (QEMU built with --enable-sanitizers):
>
>  Direct leak of 4096 byte(s) in 1 object(s) allocated from:
>      #0 0x55d7a363773f in malloc (/mnt/scratch/qemu/sanitizer/qemu-system-x86_64+0x1dab73f)
>      #1 0x7f55c6447958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
>      #2 0x55d7a5e123aa in vvfat_open block/vvfat.c:1236:19
>      #3 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
>      #4 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
>      #5 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
>      #6 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
>      #7 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
>      #8 0x55d7a5a65da3 in bdrv_open block.c:3537:12
>      #9 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
>      #10 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
>      #11 0x55d7a5a088e7 in drive_new blockdev.c:994:11
>      #12 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
>      #13 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
>      #14 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
>      #15 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
>      #16 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
>      #17 0x55d7a366f619 in main softmmu/main.c:49:5
>
>Fixes: 8475ea48544 ("block/vvfat: Do not unref qcow on closing backing bdrv")
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> block/vvfat.c | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/block/vvfat.c b/block/vvfat.c
>index 54807f82ca1..5a4a7915220 100644
>--- a/block/vvfat.c
>+++ b/block/vvfat.c
>@@ -3233,6 +3233,7 @@ static void vvfat_close(BlockDriverState *bs)
>     array_free(&(s->directory));
>     array_free(&(s->mapping));
>     g_free(s->cluster_buffer);
>+    g_free(s->qcow_filename);
>
>     if (s->qcow) {
>         migrate_del_blocker(s->migration_blocker);
>-- 
>2.26.3
>
>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH 2/4] block/vvfat: Fix leak of BDRVVVFATState::used_clusters

[Stefano Garzarella]

On Fri, Apr 30, 2021 at 06:25:17PM +0200, Philippe Mathieu-Daudé wrote:
>used_clusters is allocated in enable_write_target(), called by
>vvfat_open(), but never free'd.
>Allocate it using GLib API, and free it in vvfat_close().
>
>This fixes (QEMU built with --enable-sanitizers):
>
>  Direct leak of 64508 byte(s) in 1 object(s) allocated from:
>      #0 0x55d7a36378f7 in calloc (qemu-system-x86_64+0x1dab8f7)
>      #1 0x55d7a5e14246 in enable_write_target block/vvfat.c:3145:24
>      #2 0x55d7a5e123aa in vvfat_open block/vvfat.c:1236:19
>      #3 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
>      #4 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
>      #5 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
>      #6 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
>      #7 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
>      #8 0x55d7a5a65da3 in bdrv_open block.c:3537:12
>      #9 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
>      #10 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
>      #11 0x55d7a5a088e7 in drive_new blockdev.c:994:11
>      #12 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
>      #13 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
>      #14 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
>      #15 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
>      #16 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
>      #17 0x55d7a366f619 in main softmmu/main.c:49:5
>
>Fixes: a046433a161 ("Major overhaul of the virtual FAT driver for read/write support")
>Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>---
> block/vvfat.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
>diff --git a/block/vvfat.c b/block/vvfat.c
>index 5a4a7915220..2cc21787600 100644
>--- a/block/vvfat.c
>+++ b/block/vvfat.c
>@@ -3142,7 +3142,7 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
>     int size = sector2cluster(s, s->sector_count);
>     QDict *options;
>
>-    s->used_clusters = calloc(size, 1);
>+    s->used_clusters = g_malloc0(size);
>
>     array_init(&(s->commits), sizeof(commit_t));
>
>@@ -3233,6 +3233,7 @@ static void vvfat_close(BlockDriverState *bs)
>     array_free(&(s->directory));
>     array_free(&(s->mapping));
>     g_free(s->cluster_buffer);
>+    g_free(s->used_clusters);
>     g_free(s->qcow_filename);
>
>     if (s->qcow) {
>-- 
>2.26.3
>
>

In vvfat_open() we set to NULL the other pointers allocated through GLib 
API (e.g. s->qcow_filename), but IIUC the BDRVVVFATState (bs->opaque) is 
allocated through g_malloc0() in bdrv_open_driver(), so those 
initializations should be superfluous and in this case we can avoid 
setting s->used_clusters to NULL.

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH 0/4] block/vvfat: Fix leaks and out of bounds writes

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

Hi Philippe,

On Fri, 30 Apr 2021, Philippe Mathieu-Daudé wrote:

> The first 3 patches are trivial leak fixes, the last
> one is a RFC since I have no clue about this code.
>
> Johannes, you wrote this 18 years ago, do you still
> remember? =)

I do remember writing them, but I remember almost nothing of the details.
The patch series looks good to me (but keep in mind that I am not active
in the QEMU project, and that I never used that driver except for a little
playing around).

Ciao,
Johannes

>
> Philippe Mathieu-Daudé (4):
>   block/vvfat: Fix leak of BDRVVVFATState::qcow_filename
>   block/vvfat: Fix leak of BDRVVVFATState::used_clusters
>   block/vvfat: Fix leak of mapping_t::path
>   block/vvfat: Avoid out of bounds write in create_long_filename()
>
>  block/vvfat.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> --
> 2.26.3
>
>
>

Re: [PATCH 4/4] block/vvfat: Avoid out of bounds write in create_long_filename()

[Eric Blake]

On 4/30/21 11:25 AM, Philippe Mathieu-Daudé wrote:
> The direntry_t::name holds 11 bytes:
> 
>   typedef struct direntry_t {
>       uint8_t name[8 + 3];
>       ...

struct direntry_t is poorly laid out.  A quick google search finds:

https://www.ntfs.com/fat-filenames.htm

which shows that a long file name is indeed split over multiple fields
of struct direntry_t.  In addition to the direntry_t for the 8+3 short
name, there are additional direntry_t reserved for each 13 bytes of the
long name (well, 26 bytes of UTF-16, as those 13 bytes are expanded into
Unicode).

> 
> However create_long_filename() writes up to 31 bytes into it:
> 
>  421     for(i=0;i<26*number_of_entries;i++) {
>  422         int offset=(i%26);
>  423         if(offset<10) offset=1+offset;
>  424         else if(offset<22) offset=14+offset-10;
>  425         else offset=28+offset-22;
>  426         entry=array_get(&(s->directory),s->directory.next-1-(i/26));
>  427         if (i >= 2 * length + 2) {
>  428             entry->name[offset] = 0xff;
>  429         } else if (i % 2 == 0) {
>  430             entry->name[offset] = longname[i / 2] & 0xff;
>  431         } else {
>  432             entry->name[offset] = longname[i / 2] >> 8;
>  433         }
>  434     }

The code is probably correct for which bytes it is writing, but wrong
for calling it entry->name[offset]; what we probably want is:

typedef struct direntry_t {
  union {
    struct {
      uint8_t name[8 + 3];
      ...
      uint32_t size;
    };
    char raw[32];
  };
} QEMU_PACKED direntry_t;

where we then access through entry->raw[offset] instead of
entry->name[offset] (I know that anonymous union/structs are not
standard C, but I think that both gcc and clang support them, so that we
don't have to refactor the rest of the code to go through additional
names introduced by those modifications to direntry_t).

> 
> For example, if i=25, offset=28+25-22=31
> 
> Then in lines 428, 430 and 432 the entry->name[] array is written beside
> its 11 bytes, as reported by Clang sanitizer:
> 
>   block/vvfat.c:430:13: runtime error: index 14 out of bounds for type 'uint8_t [11]'
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior block/vvfat.c:430:13 in
>   block/vvfat.c:432:13: runtime error: index 15 out of bounds for type 'uint8_t [11]'
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior block/vvfat.c:432:13 in
>   block/vvfat.c:428:13: runtime error: index 18 out of bounds for type 'uint8_t [11]'
>   SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior block/vvfat.c:428:13 in
> 
> As I have no idea about what this code does, simply skip the writes if
> out of range, since it is not worst than what we have currently (and
> my tests using vvfat work identically).
> 
> Fixes: de167e416fa ("Virtual VFAT support (Johannes Schindelin)")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

NACK.  This violates what FAT expects for long names.  Our code is not
pretty, but we should clean it up correctly rather than breaking it.

> ---
>  block/vvfat.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/block/vvfat.c b/block/vvfat.c
> index c193a816646..c7162e77d68 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -423,6 +423,9 @@ static direntry_t *create_long_filename(BDRVVVFATState *s, const char *filename)
>          if(offset<10) offset=1+offset;
>          else if(offset<22) offset=14+offset-10;
>          else offset=28+offset-22;
> +        if (offset >= ARRAY_SIZE(entry->name)) {
> +            continue;
> +        }
>          entry=array_get(&(s->directory),s->directory.next-1-(i/26));
>          if (i >= 2 * length + 2) {
>              entry->name[offset] = 0xff;
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/4] block/vvfat: Fix leak of mapping_t::path

[Max Reitz]

On 30.04.21 18:25, Philippe Mathieu-Daudé wrote:
> read_directory() keeps pointers to alloc'ed data in path ...:
> 
>   743 static int read_directory(BDRVVVFATState* s, int mapping_index)
>   744 {
>   ...
>   792         buffer = g_malloc(length);
>   ...
>   828         /* create mapping for this file */
>   829         if(!is_dot && !is_dotdot && (S_ISDIR(st.st_mode) || st.st_size)) {
>   830             s->current_mapping = array_get_next(&(s->mapping));
>   ...
>   847             s->current_mapping->path=buffer;
> 
> ... but these pointers are never free'd.

“Never” is not quite right, they are freed in some places: Namely in 
remove_mapping(), and in handle_renames_and_mkdirs().

Speaking of the latter, which does some commit_t handling – should those 
paths also be freed?  (i.e. the ones in commit_t, in s->commits)

> Free them in vvfat_close(),
> to fix (QEMU built with --enable-sanitizers):
> 
>    Direct leak of 148 byte(s) in 6 object(s) allocated from:
>      #0 0x55d7a363773f in malloc (qemu-system-x86_64+0x1dab73f)
>      #1 0x7f55c6447958 in g_malloc (/lib64/libglib-2.0.so.0+0x58958)
>      #2 0x55d7a5e17679 in init_directories block/vvfat.c:962:16
>      #3 0x55d7a5e1251e in vvfat_open block/vvfat.c:1255:9
>      #4 0x55d7a5a5363f in bdrv_open_driver block.c:1526:15
>      #5 0x55d7a5a9d369 in bdrv_open_common block.c:1802:11
>      #6 0x55d7a5a609f1 in bdrv_open_inherit block.c:3444:11
>      #7 0x55d7a5a65411 in bdrv_open_child_bs block.c:3079:10
>      #8 0x55d7a5a60079 in bdrv_open_inherit block.c:3391:19
>      #9 0x55d7a5a65da3 in bdrv_open block.c:3537:12
>      #10 0x55d7a5b33f6a in blk_new_open block/block-backend.c:421:10
>      #11 0x55d7a5a0a33e in blockdev_init blockdev.c:610:15
>      #12 0x55d7a5a088e7 in drive_new blockdev.c:994:11
>      #13 0x55d7a51b10c4 in drive_init_func softmmu/vl.c:636:12
>      #14 0x55d7a620e148 in qemu_opts_foreach util/qemu-option.c:1167:14
>      #15 0x55d7a51b0e20 in configure_blockdev softmmu/vl.c:695:9
>      #16 0x55d7a51a70b5 in qemu_create_early_backends softmmu/vl.c:1895:5
>      #17 0x55d7a519bf87 in qemu_init softmmu/vl.c:3551:5
>      #18 0x55d7a366f619 in main softmmu/main.c:49:5
> 
> Fixes: a046433a161 ("Major overhaul of the virtual FAT driver for read/write support")
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>   block/vvfat.c | 5 +++++
>   1 file changed, 5 insertions(+)

First:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Second:

In commit_mappings(), there is this line:

next_mapping->path = mapping->path;

That looks strange.  Should that be a g_strdup(), or am I missing the 
place where only one of them can really exist?

But, well, all of the vvfat code is just strange to me.

> diff --git a/block/vvfat.c b/block/vvfat.c
> index 2cc21787600..c193a816646 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -3228,6 +3228,11 @@ static void vvfat_close(BlockDriverState *bs)
>   {
>       BDRVVVFATState *s = bs->opaque;
>   
> +    for (unsigned j = 0; j < s->mapping.next; j++) {
> +        mapping_t *mapping = array_get(&(s->mapping), j);
> +
> +        g_free(mapping->path);
> +    }
>       vvfat_close_current_file(s);
>       array_free(&(s->fat));
>       array_free(&(s->directory));
>