qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Hanna Reitz
Subject: Re: [PATCH] block/vvfat.c fix leak when failure occurs
Date: Thu, 18 Nov 2021 15:51:04 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.3.0

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





reply via email to

[Prev in Thread] Current Thread [Next in Thread]