[Top][All Lists]

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

Re: [Qemu-devel] [PATCH 18/34] qcow2: Fix memory leak in qcow2_update_op

From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 18/34] qcow2: Fix memory leak in qcow2_update_options() error path
Date: Wed, 13 May 2015 14:04:21 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0

On 13.05.2015 14:02, Kevin Wolf wrote:
Am 13.05.2015 um 13:52 hat Max Reitz geschrieben:
On 08.05.2015 19:21, Kevin Wolf wrote:
Signed-off-by: Kevin Wolf <address@hidden>
  block/qcow2.c | 12 ++++++++++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index abe22f3..84d6e0f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -546,8 +546,8 @@ static int qcow2_update_options(BlockDriverState *bs, QDict 
      const char *opt_overlap_check, *opt_overlap_check_template;
      int overlap_check_template = 0;
      uint64_t l2_cache_size, refcount_cache_size;
-    Qcow2Cache* l2_table_cache;
-    Qcow2Cache* refcount_block_cache;
+    Qcow2Cache* l2_table_cache = NULL;
+    Qcow2Cache* refcount_block_cache = NULL;
      bool use_lazy_refcounts;
      int i;
      Error *local_err = NULL;
@@ -670,6 +670,14 @@ static int qcow2_update_options(BlockDriverState *bs, 
QDict *options,
      ret = 0;
+    if (ret < 0) {
+        if (l2_table_cache) {
+            qcow2_cache_destroy(bs, l2_table_cache);
+        }
+        if (refcount_block_cache) {
+            qcow2_cache_destroy(bs, refcount_block_cache);
+        }
+    }
      opts = NULL;
Why don't you squash this into patch 17 (I guess there is a reason,
but I fail to see it)?
It's a preexisting bug and its fix is unrelated to any of the
refactoring or preparation for reopen. So I think it deserves its own
commit, and if it doesn't, it's not clear to me why it should belong to
patch 17 of all patches.

Because patch 17 is introducing the {l2_table,refcount_block}_cache as local variables.

Also, I think it might make sense to either set
{l2_table,refcount_block}_cache to NULL once they have been moved to
s->{l2_table,refcount_block}_cache, or, even better, exchange them
with their respective field in *s. Thus, you could drop the "if (ret
< 0)", I think I'd find the code easier to read, and with the
transation, we'd even be safe if the options had been set before and
are now to be updated.
The if (ret < 0) disappears shortly after this patch when this is moved
into the abort part of a transaction.

Right, I just saw this. :-)


reply via email to

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