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

Re: [libvirt] [PATCHv3] snapshot: fix memory leak on error

On 04/05/2012 10:56 PM, Daniel Veillard wrote:
> On Thu, Apr 05, 2012 at 10:24:49PM -0600, Eric Blake wrote:
>> Leak introduced in commit 0436d32.  If we allocate an actions array,
>> but fail early enough to never consume it with the qemu monitor
>> transaction call, we leaked memory.
>> But our semantics of making the transaction command free the caller's
>> memory is awkward; avoiding the memory leak requires making every
>> intermediate function in the call chain check for error.  It is much
>> easier to fix things so that the function that allocates also frees,
>> while the call chain leaves the caller's data intact.  To do that,
>> I had to hack our JSON data structure to make it easy to protect a
>> portion of an arbitrary JSON tree from being freed.

>   ACK, sounds better to me than piggy-backing on an enum value :-)

Thanks.  But I found one more (unlikely) issue:
qemuMonitorJSONMakeCommand can also trigger a recursive free on a
partial construction when hitting OOM.  So I'm pushing with this
additional modification:

diff --git i/src/qemu/qemu_monitor_json.c w/src/qemu/qemu_monitor_json.c
index 8eeb307..d09d779 100644
--- i/src/qemu/qemu_monitor_json.c
+++ w/src/qemu/qemu_monitor_json.c
@@ -3160,16 +3160,18 @@ cleanup:
 qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
-    int ret;
+    int ret = -1;
     virJSONValuePtr cmd;
     virJSONValuePtr reply = NULL;
-    bool protect;
+    bool protect = actions->protect;

+    /* We do NOT want to free actions when recursively freeing cmd.  */
+    actions->protect = true;
     cmd = qemuMonitorJSONMakeCommand("transaction",
                                      "a:actions", actions,
     if (!cmd)
-        return -1;
+        goto cleanup;

     if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
         goto cleanup;
@@ -3177,12 +3179,9 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon,
virJSONValuePtr actions)
     ret = qemuMonitorJSONCheckError(cmd, reply);

-    /* We do NOT want to free actions when recursively freeing cmd.  */
-    protect = actions->protect;
-    actions->protect = true;
-    actions->protect = protect;
+    actions->protect = protect;
     return ret;

Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

Attachment: signature.asc
Description: OpenPGP digital signature

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