[PATCH] block/vvfat.c fix leak when failure occurs

[PATCH] block/vvfat.c fix leak when failure occurs

[Daniella Lee]

Function vvfat_open called function enable_write_target and init_directories,
and these functions malloc new memory for BDRVVVFATState::qcow_filename, 
BDRVVVFATState::used_clusters, and BDRVVVFATState::cluster_buff.

When the specified folder does not exist ,it may contains memory leak. 
After init_directories function is executed, the vvfat_open return -EIO, 
and bdrv_open_driver goto label open_failed, 
the program use g_free(bs->opaque) to release BDRVVVFATState struct 
without members mentioned.

command line:
qemu-system-x86_64 -hdb <vdisk qcow file>  -usb -device usb-storage,drive=fat16 
-drive file=fat:rw:fat-type=16:"<path of a host folder does not exist>",
id=fat16,format=raw,if=none

enable_write_target called:
(gdb) bt
#0  enable_write_target (bs=0x555556f9f000, errp=0x7fffffffd780) 
    at ../block/vvfat.c:3114
#1  vvfat_open (bs=0x555556f9f000, options=0x555556fa45d0, 
    flags=155650, errp=0x7fffffffd780) at ../block/vvfat.c:1236
#2  bdrv_open_driver (bs=0x555556f9f000, drv=0x555556c47920 <bdrv_vvfat>, 
    node_name=0x0, options=0x555556fa45d0, open_flags=155650, 
    errp=0x7fffffffd890) at ../block.c:1558
#3  bdrv_open_common (bs=0x555556f9f000, file=0x0, options=0x555556fa45d0,
    errp=0x7fffffffd890) at ../block.c:1852
#4  bdrv_open_inherit (filename=0x555556f73310 "fat:rw:<dirNone>", 
    reference=0x0, options=0x555556fa45d0, flags=40962, parent=0x555556f98cd0,
    child_class=0x555556b1d6a0 <child_of_bds>, child_role=19, 
    errp=0x7fffffffda90) at ../block.c:3779
#5  bdrv_open_child_bs (filename=0x555556f73310 "fat:rw:<dirNone>", 
    options=0x555556f9cfc0, bdref_key=0x555556239bb8 "file", 
    parent=0x555556f98cd0, child_class=0x555556b1d6a0 <child_of_bds>, 
    child_role=19, allow_none=true, errp=0x7fffffffda90) at ../block.c:3419
#6  bdrv_open_inherit (filename=0x555556f73310 "fat:rw:<dirNone>", 
    reference=0x0, options=0x555556f9cfc0, flags=8194, parent=0x0,
    child_class=0x0, child_role=0, errp=0x555556c98c40 <error_fatal>) 
    at ../block.c:3726
#7  bdrv_open (filename=0x555556f73310 "fat:rw:<dirNone>", reference=0x0,
    options=0x555556f757b0, flags=0, errp=0x555556c98c40 <error_fatal>) 
    at ../block.c:3872
#8  blk_new_open (filename=0x555556f73310 "fat:rw:<dirNone>", reference=0x0,
    options=0x555556f757b0, flags=0, errp=0x555556c98c40 <error_fatal>)
    at ../block/block-backend.c:436
#9  blockdev_init (file=0x555556f73310 "fat:rw:<dirNone>", 
    bs_opts=0x555556f757b0, errp=0x555556c98c40 <error_fatal>) 
    at ../blockdev.c:608
#10 drive_new (all_opts=0x555556d2b700, block_default_type=IF_IDE, 
    errp=0x555556c98c40 <error_fatal>) at ../blockdev.c:992
......

Signed-off-by: Daniella Lee <daniellalee111@gmail.com>
---
 block/vvfat.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/vvfat.c b/block/vvfat.c
index 05e78e3c27..454a74c5d5 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1280,7 +1280,22 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
     qemu_co_mutex_init(&s->lock);
 
     ret = 0;
+
+    qemu_opts_del(opts);
+    return ret;
 fail:
+    if(s->qcow_filename) {
+        g_free(s->qcow_filename);
+        s->qcow_filename = NULL;
+    }
+    if(s->cluster_buffer) {
+        g_free(s->cluster_buffer);
+        s->cluster_buffer = NULL;
+    } 
+    if(s->used_clusters) {
+        g_free(s->used_clusters);
+        s->used_clusters = NULL;
+    }
     qemu_opts_del(opts);
     return ret;
 }
-- 
2.17.1

Re: [PATCH] block/vvfat.c fix leak when failure occurs

[Hanna Reitz]

On 16.11.21 13:57, Daniella Lee wrote:
> Function vvfat_open called function enable_write_target and init_directories,
> and these functions malloc new memory for BDRVVVFATState::qcow_filename,
> BDRVVVFATState::used_clusters, and BDRVVVFATState::cluster_buff.
>
> When the specified folder does not exist ,it may contains memory leak.
> After init_directories function is executed, the vvfat_open return -EIO,
> and bdrv_open_driver goto label open_failed,
> the program use g_free(bs->opaque) to release BDRVVVFATState struct
> without members mentioned.
>
> command line:
> qemu-system-x86_64 -hdb <vdisk qcow file>  -usb -device usb-storage,drive=fat16
> -drive file=fat:rw:fat-type=16:"<path of a host folder does not exist>",
> id=fat16,format=raw,if=none
>
> enable_write_target called:
> (gdb) bt
> #0  enable_write_target (bs=0x555556f9f000, errp=0x7fffffffd780)
>      at ../block/vvfat.c:3114
> #1  vvfat_open (bs=0x555556f9f000, options=0x555556fa45d0,
>      flags=155650, errp=0x7fffffffd780) at ../block/vvfat.c:1236
> #2  bdrv_open_driver (bs=0x555556f9f000, drv=0x555556c47920 <bdrv_vvfat>,
>      node_name=0x0, options=0x555556fa45d0, open_flags=155650,
>      errp=0x7fffffffd890) at ../block.c:1558
> #3  bdrv_open_common (bs=0x555556f9f000, file=0x0, options=0x555556fa45d0,
>      errp=0x7fffffffd890) at ../block.c:1852
> #4  bdrv_open_inherit (filename=0x555556f73310 "fat:rw:<dirNone>",
>      reference=0x0, options=0x555556fa45d0, flags=40962, parent=0x555556f98cd0,
>      child_class=0x555556b1d6a0 <child_of_bds>, child_role=19,
>      errp=0x7fffffffda90) at ../block.c:3779
> #5  bdrv_open_child_bs (filename=0x555556f73310 "fat:rw:<dirNone>",
>      options=0x555556f9cfc0, bdref_key=0x555556239bb8 "file",
>      parent=0x555556f98cd0, child_class=0x555556b1d6a0 <child_of_bds>,
>      child_role=19, allow_none=true, errp=0x7fffffffda90) at ../block.c:3419
> #6  bdrv_open_inherit (filename=0x555556f73310 "fat:rw:<dirNone>",
>      reference=0x0, options=0x555556f9cfc0, flags=8194, parent=0x0,
>      child_class=0x0, child_role=0, errp=0x555556c98c40 <error_fatal>)
>      at ../block.c:3726
> #7  bdrv_open (filename=0x555556f73310 "fat:rw:<dirNone>", reference=0x0,
>      options=0x555556f757b0, flags=0, errp=0x555556c98c40 <error_fatal>)
>      at ../block.c:3872
> #8  blk_new_open (filename=0x555556f73310 "fat:rw:<dirNone>", reference=0x0,
>      options=0x555556f757b0, flags=0, errp=0x555556c98c40 <error_fatal>)
>      at ../block/block-backend.c:436
> #9  blockdev_init (file=0x555556f73310 "fat:rw:<dirNone>",
>      bs_opts=0x555556f757b0, errp=0x555556c98c40 <error_fatal>)
>      at ../blockdev.c:608
> #10 drive_new (all_opts=0x555556d2b700, block_default_type=IF_IDE,
>      errp=0x555556c98c40 <error_fatal>) at ../blockdev.c:992
> ......
>
> Signed-off-by: Daniella Lee <daniellalee111@gmail.com>
> ---
>   block/vvfat.c | 15 +++++++++++++++
>   1 file changed, 15 insertions(+)

Hi,

Thanks for your patch!  Yes, that makes sense.

I believe there are some issues that should be addressed, though:

> diff --git a/block/vvfat.c b/block/vvfat.c
> index 05e78e3c27..454a74c5d5 100644
> --- a/block/vvfat.c
> +++ b/block/vvfat.c
> @@ -1280,7 +1280,22 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
>       qemu_co_mutex_init(&s->lock);
>   
>       ret = 0;
> +
> +    qemu_opts_del(opts);
> +    return ret;

Optional: I’d drop the `ret = 0;` line and just `return 0;` here.

>   fail:
> +    if(s->qcow_filename) {

Our coding style requires a space between `if` and the opening parenthesis.

> +        g_free(s->qcow_filename);

`g_free()` checks whether the parameter given to it is `NULL`, and if 
so, performs a no-op.  So checking whether `s->qcow_filename != NULL` 
before calling `g_free()` is not necessary.

We have a script under scripts/checkpatch.pl that takes patch files as 
input and checks whether they conform to our coding style.  It’s really 
helpful, for example in these two cases it does report the issues.

> +        s->qcow_filename = NULL;
> +    }
> +    if(s->cluster_buffer) {
> +        g_free(s->cluster_buffer);
> +        s->cluster_buffer = NULL;
> +    }
> +    if(s->used_clusters) {
> +        g_free(s->used_clusters);

`s->used_clusters` is allocated with `calloc()`, so it can’t be freed 
with `g_free()`.  But you’re right, it should be `g_free()`-able, so the 
fix is to have `enable_write_target()` allocate it with `g_malloc0(size)`.

(And this made me notice that we free neither `s->used_clusters` nor 
`s->qcow_filename` in vvfat_close()...  Oops.)

> +        s->used_clusters = NULL;
> +    }
>       qemu_opts_del(opts);
>       return ret;
>   }

Finally, `enable_write_target()` frees `s->qcow_filename` on error.  
That seems unnecessary now, though not wrong.  (It’s just weird that it 
frees that one, but not `s->used_clusters`...)

Hanna

Re: [PATCH] block/vvfat.c fix leak when failure occurs

[Hanna Reitz]

On 18.11.21 10:33, Daniella Lee wrote:
> Thanks for your reply and your suggestion is useful.
> This is my first submission, and I will pay attention to these issues 
> in the future.
> There are many other places you mentioned need to be modified,
> do I need to resubmit the patch, or you want to modify them with other 
> codes?

Hi,

Yes, you should send a v2 that addresses the issues.

As for the suggestions that concern places outside of `vvfat_open()`:  
Most of them need not be your concern if you don’t want them to be, but 
we definitely need to have `s->used_clusters` be allocated with 
`g_malloc0()`, or we can’t free it with `g_free()`.  (We could free it 
with `free()`, but that would be a suboptimal solution...)  So that 
allocation line in `enable_write_target()` should be fixed in v2.  The 
best way to do that would be to do it in an own patch (i.e. patch 1 
changes that allocation, and patch 2 is this patch), but I wouldn’t mind 
it too much if you do both changes in a single patch.

Regarding the other suggestions for other places: It would be nice to 
drop the clean-up path in `enable_write_target()`, but isn’t necessary.  
If you want to do it, you can do it as part of this patch, or on top in 
another patch, but you can also choose just to ignore that bit.  (And 
maybe then I’ll send a patch later.)

The fact that we’re leaking two of these buffers in `vvfat_close()` is 
definitely unrelated to this patch, so that’s also nothing you have to 
care about if you don’t want to.

I hope that made it clear...?  Don’t hesitate to ask more if it didn’t 
(or for any other questions you might have).

Hanna

> On Thu, Nov 18, 2021 at 4:34 PM Hanna Reitz <hreitz@redhat.com> wrote:
>
>     On 16.11.21 13:57, Daniella Lee wrote:
>     > Function vvfat_open called function enable_write_target and
>     init_directories,
>     > and these functions malloc new memory for
>     BDRVVVFATState::qcow_filename,
>     > BDRVVVFATState::used_clusters, and BDRVVVFATState::cluster_buff.
>     >
>     > When the specified folder does not exist ,it may contains memory
>     leak.
>     > After init_directories function is executed, the vvfat_open
>     return -EIO,
>     > and bdrv_open_driver goto label open_failed,
>     > the program use g_free(bs->opaque) to release BDRVVVFATState struct
>     > without members mentioned.
>     >
>     > command line:
>     > qemu-system-x86_64 -hdb <vdisk qcow file>  -usb -device
>     usb-storage,drive=fat16
>     > -drive file=fat:rw:fat-type=16:"<path of a host folder does not
>     exist>",
>     > id=fat16,format=raw,if=none
>     >
>     > enable_write_target called:
>     > (gdb) bt
>     > #0  enable_write_target (bs=0x555556f9f000, errp=0x7fffffffd780)
>     >      at ../block/vvfat.c:3114
>     > #1  vvfat_open (bs=0x555556f9f000, options=0x555556fa45d0,
>     >      flags=155650, errp=0x7fffffffd780) at ../block/vvfat.c:1236
>     > #2  bdrv_open_driver (bs=0x555556f9f000, drv=0x555556c47920
>     <bdrv_vvfat>,
>     >      node_name=0x0, options=0x555556fa45d0, open_flags=155650,
>     >      errp=0x7fffffffd890) at ../block.c:1558
>     > #3  bdrv_open_common (bs=0x555556f9f000, file=0x0,
>     options=0x555556fa45d0,
>     >      errp=0x7fffffffd890) at ../block.c:1852
>     > #4  bdrv_open_inherit (filename=0x555556f73310 "fat:rw:<dirNone>",
>     >      reference=0x0, options=0x555556fa45d0, flags=40962,
>     parent=0x555556f98cd0,
>     >      child_class=0x555556b1d6a0 <child_of_bds>, child_role=19,
>     >      errp=0x7fffffffda90) at ../block.c:3779
>     > #5  bdrv_open_child_bs (filename=0x555556f73310 "fat:rw:<dirNone>",
>     >      options=0x555556f9cfc0, bdref_key=0x555556239bb8 "file",
>     >      parent=0x555556f98cd0, child_class=0x555556b1d6a0
>     <child_of_bds>,
>     >      child_role=19, allow_none=true, errp=0x7fffffffda90) at
>     ../block.c:3419
>     > #6  bdrv_open_inherit (filename=0x555556f73310 "fat:rw:<dirNone>",
>     >      reference=0x0, options=0x555556f9cfc0, flags=8194, parent=0x0,
>     >      child_class=0x0, child_role=0, errp=0x555556c98c40
>     <error_fatal>)
>     >      at ../block.c:3726
>     > #7  bdrv_open (filename=0x555556f73310 "fat:rw:<dirNone>",
>     reference=0x0,
>     >      options=0x555556f757b0, flags=0, errp=0x555556c98c40
>     <error_fatal>)
>     >      at ../block.c:3872
>     > #8  blk_new_open (filename=0x555556f73310 "fat:rw:<dirNone>",
>     reference=0x0,
>     >      options=0x555556f757b0, flags=0, errp=0x555556c98c40
>     <error_fatal>)
>     >      at ../block/block-backend.c:436
>     > #9  blockdev_init (file=0x555556f73310 "fat:rw:<dirNone>",
>     >      bs_opts=0x555556f757b0, errp=0x555556c98c40 <error_fatal>)
>     >      at ../blockdev.c:608
>     > #10 drive_new (all_opts=0x555556d2b700, block_default_type=IF_IDE,
>     >      errp=0x555556c98c40 <error_fatal>) at ../blockdev.c:992
>     > ......
>     >
>     > Signed-off-by: Daniella Lee <daniellalee111@gmail.com>
>     > ---
>     >   block/vvfat.c | 15 +++++++++++++++
>     >   1 file changed, 15 insertions(+)
>
>     Hi,
>
>     Thanks for your patch!  Yes, that makes sense.
>
>     I believe there are some issues that should be addressed, though:
>
>     > diff --git a/block/vvfat.c b/block/vvfat.c
>     > index 05e78e3c27..454a74c5d5 100644
>     > --- a/block/vvfat.c
>     > +++ b/block/vvfat.c
>     > @@ -1280,7 +1280,22 @@ static int vvfat_open(BlockDriverState
>     *bs, QDict *options, int flags,
>     >       qemu_co_mutex_init(&s->lock);
>     >
>     >       ret = 0;
>     > +
>     > +    qemu_opts_del(opts);
>     > +    return ret;
>
>     Optional: I’d drop the `ret = 0;` line and just `return 0;` here.
>
>     >   fail:
>     > +    if(s->qcow_filename) {
>
>     Our coding style requires a space between `if` and the opening
>     parenthesis.
>
>     > +        g_free(s->qcow_filename);
>
>     `g_free()` checks whether the parameter given to it is `NULL`, and if
>     so, performs a no-op.  So checking whether `s->qcow_filename != NULL`
>     before calling `g_free()` is not necessary.
>
>     We have a script under scripts/checkpatch.pl
>     <http://checkpatch.pl> that takes patch files as
>     input and checks whether they conform to our coding style. It’s
>     really
>     helpful, for example in these two cases it does report the issues.
>
>     > +        s->qcow_filename = NULL;
>     > +    }
>     > +    if(s->cluster_buffer) {
>     > +        g_free(s->cluster_buffer);
>     > +        s->cluster_buffer = NULL;
>     > +    }
>     > +    if(s->used_clusters) {
>     > +        g_free(s->used_clusters);
>
>     `s->used_clusters` is allocated with `calloc()`, so it can’t be freed
>     with `g_free()`.  But you’re right, it should be `g_free()`-able,
>     so the
>     fix is to have `enable_write_target()` allocate it with
>     `g_malloc0(size)`.
>
>     (And this made me notice that we free neither `s->used_clusters` nor
>     `s->qcow_filename` in vvfat_close()...  Oops.)
>
>     > +        s->used_clusters = NULL;
>     > +    }
>     >       qemu_opts_del(opts);
>     >       return ret;
>     >   }
>
>     Finally, `enable_write_target()` frees `s->qcow_filename` on error.
>     That seems unnecessary now, though not wrong.  (It’s just weird
>     that it
>     frees that one, but not `s->used_clusters`...)
>
>     Hanna
>

[PATCH] block vvfat.c fix leak when failure occurs

[Daniella Lee]

Based on your suggestions. I made a new patch which contians:
1.format detection
2.replace calloc with g_malloc0 in enable_write_target function
3.use g_free without null pointer detection in vvfat_open function
4.delete line "ret = 0", use return ret directly in vvfat_open function


Signed-off-by: Daniella Lee <daniellalee111@gmail.com>
---
 block/vvfat.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/block/vvfat.c b/block/vvfat.c
index 05e78e3c27..5dacc6cfac 100644
--- a/block/vvfat.c
+++ b/block/vvfat.c
@@ -1279,8 +1279,18 @@ static int vvfat_open(BlockDriverState *bs, QDict *options, int flags,
 
     qemu_co_mutex_init(&s->lock);
 
-    ret = 0;
+    qemu_opts_del(opts);
+
+    return 0;
+
 fail:
+    g_free(s->qcow_filename);
+    s->qcow_filename = NULL;
+    g_free(s->cluster_buffer);
+    s->cluster_buffer = NULL;
+    g_free(s->used_clusters);
+    s->used_clusters = NULL;
+
     qemu_opts_del(opts);
     return ret;
 }
@@ -3118,7 +3128,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));
 
@@ -3166,8 +3176,6 @@ static int enable_write_target(BlockDriverState *bs, Error **errp)
     return 0;
 
 err:
-    g_free(s->qcow_filename);
-    s->qcow_filename = NULL;
     return ret;
 }
 
-- 
2.17.1

Re: [PATCH] block vvfat.c fix leak when failure occurs

[Hanna Reitz]

On 19.11.21 12:25, Daniella Lee wrote:
> Based on your suggestions. I made a new patch which contians:
> 1.format detection
> 2.replace calloc with g_malloc0 in enable_write_target function
> 3.use g_free without null pointer detection in vvfat_open function
> 4.delete line "ret = 0", use return ret directly in vvfat_open function
>
>
> Signed-off-by: Daniella Lee <daniellalee111@gmail.com>
> ---
>   block/vvfat.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)

Thanks, looks good to me!

Two remarks for the next time: When you send a new version of a patch, 
you should mark it as “v2” (and then “v3” and so on) in the subject, 
e.g. “[PATCH v2]”.  This can be done e.g. with `git format-patch -v2`.

Second, for a new iteration, you should generally keep the commit 
message from the previous one and not replace it with a change log. 
Having a change log is very nice, don’t get me wrong, but it shouldn’t 
be part of the commit message.  Once you’ve formatted the patch with 
`git format-patch`, you can edit the file and put the change log below 
the “---” line, e.g. like it was done here: 
https://lists.nongnu.org/archive/html/qemu-block/2021-09/msg00453.html

No need for you to respin this patch, though, I’ve just replaced the 
commit message with the one from v1 and applied it to my block branch:

https://gitlab.com/hreitz/qemu/-/commits/block/

Thanks again,

Hanna