util: json: Fix freeing of objects appended to virJSONValue

It was not possible to determine whether virJSONValueObjectAddVArgs and
the functions using it would consume a virJSONValue or not when used
with the 'a' or 'A' modifier depending on when the loop failed.

Fix this by passing in a pointer to the pointer so that it can be
cleared once it's successfully consumed and the callers don't have to
second-guess leaving a chance of leaking or double freeing the value
depending on the ordering.

Fix all callers to pass a double pointer too.

Signed-off-by: Peter Krempa <pkrempa@redhat.com>

diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 89183c3f762617c7cb925f8bde013be4ad48a04d..b99d5b10e331387853925faeeb39243a50e88c8a 100644 (file)
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1336,14 +1336,13 @@ int qemuAgentFSFreeze(qemuAgentPtr mon, const char **mountpoints,
             return -1;
 
         cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze-list",
-                                   "a:mountpoints", arg, NULL);
+                                   "a:mountpoints", &arg, NULL);
     } else {
         cmd = qemuAgentMakeCommand("guest-fsfreeze-freeze", NULL);
     }
 
     if (!cmd)
         goto cleanup;
-    arg = NULL;
 
     if (qemuAgentCommand(mon, cmd, &reply, true,
                          VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
@@ -1611,12 +1610,10 @@ qemuAgentSetVCPUsCommand(qemuAgentPtr mon,
     }
 
     if (!(cmd = qemuAgentMakeCommand("guest-set-vcpus",
-                                     "a:vcpus", cpus,
+                                     "a:vcpus", &cpus,
                                      NULL)))
         goto cleanup;
 
-    cpus = NULL;
-
     if (qemuAgentCommand(mon, cmd, &reply, true,
                          VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
         goto cleanup;

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
index 6f81e9d96cb9745e57b5cdaf76a77d29098a4d25..c0cf6a95ad8b72be84754ddf05e93c9a5c0c18c7 100644 (file)
--- a/src/qemu/qemu_block.c
+++ b/src/qemu/qemu_block.c
@@ -674,11 +674,9 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src)
                                  "s:driver", "gluster",
                                  "s:volume", src->volume,
                                  "s:path", src->path,
-                                 "a:server", servers, NULL) < 0)
+                                 "a:server", &servers, NULL) < 0)
         goto cleanup;
 
-    servers = NULL;
-
     if (src->debug &&
         virJSONValueObjectAdd(props, "u:debug", src->debugLevel, NULL) < 0)
         goto cleanup;
@@ -719,7 +717,7 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src)
                                  "s:driver", protocol,
                                  "S:tls-creds", src->tlsAlias,
                                  "s:vdisk-id", src->path,
-                                 "a:server", server, NULL) < 0)
+                                 "a:server", &server, NULL) < 0)
         virJSONValueFree(server);
 
     return ret;
@@ -867,14 +865,12 @@ qemuBlockStorageSourceGetNBDProps(virStorageSourcePtr src)
 
     if (virJSONValueObjectCreate(&ret,
                                  "s:driver", "nbd",
-                                 "a:server", serverprops,
+                                 "a:server", &serverprops,
                                  "S:export", src->path,
                                  "S:tls-creds", src->tlsAlias,
                                  NULL) < 0)
         goto cleanup;
 
-    serverprops = NULL;
-
  cleanup:
     virJSONValueFree(serverprops);
     return ret;
@@ -902,13 +898,11 @@ qemuBlockStorageSourceGetRBDProps(virStorageSourcePtr src)
                                  "s:image", src->path,
                                  "S:snapshot", src->snapshot,
                                  "S:conf", src->configFile,
-                                 "A:server", servers,
+                                 "A:server", &servers,
                                  "S:user", username,
                                  NULL) < 0)
         goto cleanup;
 
-    servers = NULL;
-
  cleanup:
     virJSONValueFree(servers);
     return ret;
@@ -935,13 +929,11 @@ qemuBlockStorageSourceGetSheepdogProps(virStorageSourcePtr src)
     /* libvirt does not support the 'snap-id' and 'tag' properties */
     if (virJSONValueObjectCreate(&ret,
                                  "s:driver", "sheepdog",
-                                 "a:server", serverprops,
+                                 "a:server", &serverprops,
                                  "s:vdi", src->path,
                                  NULL) < 0)
         goto cleanup;
 
-    serverprops = NULL;
-
  cleanup:
     virJSONValueFree(serverprops);
     return ret;
@@ -971,13 +963,11 @@ qemuBlockStorageSourceGetSshProps(virStorageSourcePtr src)
     if (virJSONValueObjectCreate(&ret,
                                  "s:driver", "ssh",
                                  "s:path", src->path,
-                                 "a:server", serverprops,
+                                 "a:server", &serverprops,
                                  "S:user", username,
                                  NULL) < 0)
         goto cleanup;
 
-    serverprops = NULL;
-
  cleanup:
     virJSONValueFree(serverprops);
     return ret;

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 682d714419deae4ab4d895c5f6161907e1a0f551..9d27c3edad7842f12a8fcfe1f20a3b17a24f54c1 100644 (file)
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1505,7 +1505,7 @@ qemuDiskSourceGetProps(virStorageSourcePtr src)
     if (!(props = qemuBlockStorageSourceGetBackendProps(src)))
         return NULL;
 
-    if (virJSONValueObjectCreate(&ret, "a:file", props, NULL) < 0) {
+    if (virJSONValueObjectCreate(&ret, "a:file", &props, NULL) < 0) {
         virJSONValueFree(props);
         return NULL;
     }

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index d80c4f18d124eb71a2ca5edac45e615b8a832f59..f38d04f663dd1f22fcd40b3825a2cda410c638f3 100644 (file)
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -3950,14 +3950,11 @@ int qemuMonitorJSONAddObject(qemuMonitorPtr mon,
     cmd = qemuMonitorJSONMakeCommand("object-add",
                                      "s:qom-type", type,
                                      "s:id", objalias,
-                                     "A:props", props,
+                                     "A:props", &props,
                                      NULL);
     if (!cmd)
         goto cleanup;
 
-    /* @props is part of @cmd now. Avoid double free */
-    props = NULL;
-
     if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
         goto cleanup;
 
@@ -4133,12 +4130,13 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
     int ret = -1;
     virJSONValuePtr cmd;
     virJSONValuePtr reply = NULL;
+    virJSONValuePtr act = actions;
     bool protect = actions->protect;
 
     /* We do NOT want to free actions when recursively freeing cmd.  */
     actions->protect = true;
     cmd = qemuMonitorJSONMakeCommand("transaction",
-                                     "a:actions", actions,
+                                     "a:actions", &act,
                                      NULL);
     if (!cmd)
         goto cleanup;
@@ -4425,15 +4423,12 @@ int qemuMonitorJSONSendKey(qemuMonitorPtr mon,
     }
 
     cmd = qemuMonitorJSONMakeCommand("send-key",
-                                     "a:keys", keys,
+                                     "a:keys", &keys,
                                      "p:hold-time", holdtime,
                                      NULL);
     if (!cmd)
         goto cleanup;
 
-    /* @keys is part of @cmd now. Avoid double free */
-    keys = NULL;
-
     if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
         goto cleanup;
 
@@ -5390,15 +5385,10 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
 
     if (!(cmd = qemuMonitorJSONMakeCommand("query-cpu-model-expansion",
                                            "s:type", typeStr,
-                                           "a:model", model,
+                                           "a:model", &model,
                                            NULL)))
         goto cleanup;
 
-    /* model will be freed when cmd is freed. we set model
-     * to NULL to avoid double freeing.
-     */
-    model = NULL;
-
     if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
         goto cleanup;
 
@@ -6263,13 +6253,11 @@ qemuMonitorJSONSetMigrationCapability(qemuMonitorPtr mon,
     cap = NULL;
 
     cmd = qemuMonitorJSONMakeCommand("migrate-set-capabilities",
-                                     "a:capabilities", caps,
+                                     "a:capabilities", &caps,
                                      NULL);
     if (!cmd)
         goto cleanup;
 
-    caps = NULL;
-
     if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
         goto cleanup;
 
@@ -6410,7 +6398,7 @@ qemuMonitorJSONBuildInetSocketAddress(const char *host,
         return NULL;
 
     if (virJSONValueObjectCreate(&addr, "s:type", "inet",
-                                        "a:data", data, NULL) < 0) {
+                                        "a:data", &data, NULL) < 0) {
         virJSONValueFree(data);
         return NULL;
     }
@@ -6428,7 +6416,7 @@ qemuMonitorJSONBuildUnixSocketAddress(const char *path)
         return NULL;
 
     if (virJSONValueObjectCreate(&addr, "s:type", "unix",
-                                        "a:data", data, NULL) < 0) {
+                                        "a:data", &data, NULL) < 0) {
         virJSONValueFree(data);
         return NULL;
     }
@@ -6454,13 +6442,10 @@ qemuMonitorJSONNBDServerStart(qemuMonitorPtr mon,
         return ret;
 
     if (!(cmd = qemuMonitorJSONMakeCommand("nbd-server-start",
-                                           "a:addr", addr,
+                                           "a:addr", &addr,
                                            NULL)))
         goto cleanup;
 
-    /* From now on, @addr is part of @cmd */
-    addr = NULL;
-
     if (qemuMonitorJSONCommand(mon, cmd, &reply) < 0)
         goto cleanup;
 
@@ -6763,10 +6748,9 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
 
     if (!(ret = qemuMonitorJSONMakeCommand("chardev-add",
                                            "s:id", chrID,
-                                           "a:backend", backend,
+                                           "a:backend", &backend,
                                            NULL)))
         goto cleanup;
-    backend = NULL;
 
  cleanup:
     VIR_FREE(tlsalias);

diff --git a/src/util/virjson.c b/src/util/virjson.c
index 6a02ddf0cc0b4ae3c11ac7a1192ecbe07de4a162..212c158da04ea17b1369e1faf409aea3a3e9a462 100644 (file)
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -109,8 +109,11 @@ virJSONValueGetType(const virJSONValue *value)
  * d: double precision floating point number
  * n: json null value
  *
+ * The following two cases take a pointer to a pointer to a virJSONValuePtr. The
+ * pointer is cleared when the virJSONValuePtr is stolen into the object.
  * a: json object, must be non-NULL
  * A: json object, omitted if NULL
+ *
  * m: a bitmap represented as a JSON array, must be non-NULL
  * M: a bitmap represented as a JSON array, omitted if NULL
  *
@@ -241,9 +244,9 @@ virJSONValueObjectAddVArgs(virJSONValuePtr obj,
 
         case 'A':
         case 'a': {
-            virJSONValuePtr val = va_arg(args, virJSONValuePtr);
+            virJSONValuePtr *val = va_arg(args, virJSONValuePtr *);
 
-            if (!val) {
+            if (!(*val)) {
                 if (type == 'A')
                     continue;
 
@@ -253,7 +256,8 @@ virJSONValueObjectAddVArgs(virJSONValuePtr obj,
                 goto cleanup;
             }
 
-            rc = virJSONValueObjectAppend(obj, key, val);
+            if ((rc = virJSONValueObjectAppend(obj, key, *val)) == 0)
+                *val = NULL;
         }   break;
 
         case 'M':

diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c
index e8fac100dd604750346983e17cd46143775dfb18..99584c759c95f252261e3debf85fc326d8cd8918 100644 (file)
--- a/tests/qemublocktest.c
+++ b/tests/qemublocktest.c
@@ -67,11 +67,9 @@ testBackingXMLjsonXML(const void *args)
         goto cleanup;
     }
 
-    if (virJSONValueObjectCreate(&wrapper, "a:file", backendprops, NULL) < 0)
+    if (virJSONValueObjectCreate(&wrapper, "a:file", &backendprops, NULL) < 0)
         goto cleanup;
 
-    backendprops = NULL;
-
     if (!(propsstr = virJSONValueToString(wrapper, false)))
         goto cleanup;