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

[Eric Blake]

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:
 int
 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,
                                      NULL);
     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);

 cleanup:
-    /* We do NOT want to free actions when recursively freeing cmd.  */
-    protect = actions->protect;
-    actions->protect = true;
     virJSONValueFree(cmd);
-    actions->protect = protect;
     virJSONValueFree(reply);
+    actions->protect = protect;
     return ret;
 }



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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 620 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20120406/859affc1/attachment-0001.sig>