From 1413560966eb07732b56dc2b83215462533656e5 Mon Sep 17 00:00:00 2001
From: Eric Blake <eblake@redhat.com>
Date: Thu, 5 Apr 2012 13:15:03 -0600
Subject: [PATCH] snapshot: fix memory leak on error

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.

* src/util/json.h (virJSONType): Name the enum.
(_virJSONValue): New field.
* src/util/json.c (virJSONValueFree): Use it to protect a portion
of an array.
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONTransaction): Avoid
freeing caller's data.
* src/qemu/qemu_driver.c (qemuDomainSnapshotCreateDiskActive):
Free actions array on failure.
---
 src/qemu/qemu_driver.c       |  1 +
 src/qemu/qemu_monitor_json.c | 14 +++++++-------
 src/util/json.c              |  9 ++++++---
 src/util/json.h              |  9 +++++----
 4 files changed, 19 insertions(+), 14 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 0880f51115..d9e35be3ef 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -10124,6 +10124,7 @@ qemuDomainSnapshotCreateDiskActive(virConnectPtr conn,
     if (actions) {
         if (ret == 0)
             ret = qemuMonitorTransaction(priv->mon, actions);
+        virJSONValueFree(actions);
         if (ret < 0) {
             /* Transaction failed; undo the changes to vm.  */
             bool need_unlink = !(flags & VIR_DOMAIN_SNAPSHOT_CREATE_REUSE_EXT);
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 7093e1d6d2..d09d7793ff 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3157,22 +3157,21 @@ cleanup:
     return ret;
 }
 
-/* Note that this call frees actions regardless of whether the call
- * succeeds.  */
 int
 qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
 {
-    int ret;
+    int ret = -1;
     virJSONValuePtr cmd;
     virJSONValuePtr reply = NULL;
+    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) {
-        virJSONValueFree(actions);
-        return -1;
-    }
+    if (!cmd)
+        goto cleanup;
 
     if ((ret = qemuMonitorJSONCommand(mon, cmd, &reply)) < 0)
         goto cleanup;
@@ -3182,6 +3181,7 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
 cleanup:
     virJSONValueFree(cmd);
     virJSONValueFree(reply);
+    actions->protect = protect;
     return ret;
 }
 
diff --git a/src/util/json.c b/src/util/json.c
index a85f580029..3258c3f386 100644
--- a/src/util/json.c
+++ b/src/util/json.c
@@ -1,7 +1,7 @@
 /*
  * json.c: JSON object parsing/formatting
  *
- * Copyright (C) 2009-2010 Red Hat, Inc.
+ * Copyright (C) 2009-2010, 2012 Red Hat, Inc.
  * Copyright (C) 2009 Daniel P. Berrange
  *
  * This library is free software; you can redistribute it and/or
@@ -67,10 +67,10 @@ struct _virJSONParser {
 void virJSONValueFree(virJSONValuePtr value)
 {
     int i;
-    if (!value)
+    if (!value || value->protect)
         return;
 
-    switch (value->type) {
+    switch ((virJSONType) value->type) {
     case VIR_JSON_TYPE_OBJECT:
         for (i = 0 ; i < value->data.object.npairs; i++) {
             VIR_FREE(value->data.object.pairs[i].key);
@@ -89,6 +89,9 @@ void virJSONValueFree(virJSONValuePtr value)
     case VIR_JSON_TYPE_NUMBER:
         VIR_FREE(value->data.number);
         break;
+    case VIR_JSON_TYPE_BOOLEAN:
+    case VIR_JSON_TYPE_NULL:
+        break;
     }
 
     VIR_FREE(value);
diff --git a/src/util/json.h b/src/util/json.h
index 457265496c..686a8fb4b2 100644
--- a/src/util/json.h
+++ b/src/util/json.h
@@ -1,8 +1,8 @@
 /*
  * json.h: JSON object parsing/formatting
  *
+ * Copyright (C) 2009, 2012 Red Hat, Inc.
  * Copyright (C) 2009 Daniel P. Berrange
- * Copyright (C) 2009 Red Hat, Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Lesser General Public
@@ -27,14 +27,14 @@
 # include "internal.h"
 
 
-enum {
+typedef enum {
     VIR_JSON_TYPE_OBJECT,
     VIR_JSON_TYPE_ARRAY,
     VIR_JSON_TYPE_STRING,
     VIR_JSON_TYPE_NUMBER,
     VIR_JSON_TYPE_BOOLEAN,
     VIR_JSON_TYPE_NULL,
-};
+} virJSONType;
 
 typedef struct _virJSONValue virJSONValue;
 typedef virJSONValue *virJSONValuePtr;
@@ -65,7 +65,8 @@ struct _virJSONArray {
 };
 
 struct _virJSONValue {
-    int type;
+    int type; /* enum virJSONType */
+    bool protect; /* prevents deletion when embedded in another object */
 
     union {
         virJSONObject object;