5:59 a.m.
Coverity was not wrong about the usage of 'a'/'A' modifiers for
virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
leak/double-free, and add test to make sure it works as expected.
This series also cleans up direct access to attributes of virJSONValue
and in the end privatizes the implementation so that all users are
forced to use accessors.
Peter Krempa (9):
util: json: Fix freeing of objects appended to virJSONValue
tests: json: Validate that attribute values are properly stolen
qemu: monitor: Use virJSONValueObjectKeysNumber in
qemuMonitorJSONGetCPUModelExpansion
qemu: agent: Avoid unnecessary JSON object type check
json: Replace access to virJSONValue->type by virJSONValueGetType
util: json: Add accessor for geting a VIR_JSON_TYPE_NUMBER as string
util: qemu: Don't access virJSONValue members directly in
virQEMUBuildCommandLineJSONRecurse
qemu: monitor: Don't resist stealing 'actions' in
qemuMonitorJSONTransaction
util: json: Privatize struct _virJSONValue and sub-structs
src/libvirt_private.syms | 1 +
src/qemu/qemu_agent.c | 21 +++++-----------
src/qemu/qemu_block.c | 22 +++++------------
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_driver.c | 4 +--
src/qemu/qemu_monitor.c | 4 +--
src/qemu/qemu_monitor.h | 2 +-
src/qemu/qemu_monitor_json.c | 59 ++++++++++++++------------------------------
src/qemu/qemu_monitor_json.h | 2 +-
src/util/virjson.c | 59 +++++++++++++++++++++++++++++++++++++++++---
src/util/virjson.h | 39 +----------------------------
src/util/virqemu.c | 13 ++++++----
tests/qemublocktest.c | 4 +--
tests/virjsontest.c | 47 +++++++++++++++++++++++++++++++++++
14 files changed, 152 insertions(+), 127 deletions(-)
--
2.16.2
5:59 a.m.
New subject: [libvirt] [PATCH 1/9] 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(a)redhat.com>
---
src/qemu/qemu_agent.c | 7 ++-----
src/qemu/qemu_block.c | 22 ++++++----------------
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_monitor_json.c | 36 ++++++++++--------------------------
src/util/virjson.c | 10 +++++++---
tests/qemublocktest.c | 4 +---
6 files changed, 27 insertions(+), 54 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index 89183c3f76..b99d5b10e3 100644
--- 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 6f81e9d96c..c0cf6a95ad 100644
--- 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 89fd08b642..3d4130d3c0 100644
--- 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 d80c4f18d1..f38d04f663 100644
--- 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 6a02ddf0cc..212c158da0 100644
--- 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 e8fac100dd..99584c759c 100644
--- 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;
--
2.16.2
3:48 p.m.
New subject: [libvirt] [PATCH 1/9] util: json: Fix freeing of objects appended to virJSONValue
On Fri, Mar 30, 2018 at 12:59:08PM +0200, Peter Krempa wrote:
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(a)redhat.com>
---
src/qemu/qemu_agent.c | 7 ++-----
src/qemu/qemu_block.c | 22 ++++++----------------
src/qemu/qemu_command.c | 2 +-
src/qemu/qemu_monitor_json.c | 36 ++++++++++--------------------------
src/util/virjson.c | 10 +++++++---
tests/qemublocktest.c | 4 +---
6 files changed, 27 insertions(+), 54 deletions(-)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 2/9] tests: json: Validate that attribute values are properly stolen
Make sure that the 'a' and 'A' modifiers for virJSONValueObjectAddVArgs
behave correctly.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
tests/virjsontest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
diff --git a/tests/virjsontest.c b/tests/virjsontest.c
index 685df7276e..fe72b84340 100644
--- a/tests/virjsontest.c
+++ b/tests/virjsontest.c
@@ -429,6 +429,51 @@ testJSONEscapeObj(const void *data ATTRIBUTE_UNUSED)
}
+static int
+testJSONObjectFormatSteal(const void *opaque ATTRIBUTE_UNUSED)
+{
+ virJSONValuePtr a1 = NULL;
+ virJSONValuePtr a2 = NULL;
+ virJSONValuePtr t1 = NULL;
+ virJSONValuePtr t2 = NULL;
+ int ret = -1;
+
+ if (!(a1 = virJSONValueNewString("test")) ||
+ !(a2 = virJSONValueNewString("test"))) {
+ VIR_TEST_VERBOSE("Failed to create json object");
+ }
+
+ if (virJSONValueObjectCreate(&t1, "a:t", &a1, "s:f",
NULL, NULL) != -1) {
+ VIR_TEST_VERBOSE("virJSONValueObjectCreate(t1) should have failed\n");
+ goto cleanup;
+ }
+
+ if (a1) {
+ VIR_TEST_VERBOSE("appended object a1 was not consumed\n");
+ goto cleanup;
+ }
+
+ if (virJSONValueObjectCreate(&t2, "s:f", NULL, "a:t",
&a1, NULL) != -1) {
+ VIR_TEST_VERBOSE("virJSONValueObjectCreate(t2) should have failed\n");
+ goto cleanup;
+ }
+
+ if (!a2) {
+ VIR_TEST_VERBOSE("appended object a2 was consumed\n");
+ goto cleanup;
+ }
+
+ ret = 0;
+
+ cleanup:
+ virJSONValueFree(a1);
+ virJSONValueFree(a2);
+ virJSONValueFree(t1);
+ virJSONValueFree(t2);
+ return ret;
+}
+
+
static int
mymain(void)
{
@@ -588,6 +633,8 @@ mymain(void)
NULL, true);
DO_TEST_FULL("create object with nested json in attribute", EscapeObj,
NULL, NULL, true);
+ DO_TEST_FULL("stealing of attributes while creating objects",
+ ObjectFormatSteal, NULL, NULL, true);
#define DO_TEST_DEFLATTEN(name, pass) \
DO_TEST_FULL(name, Deflatten, name, NULL, pass)
--
2.16.2
6:11 a.m.
New subject: [libvirt] [PATCH 2/9] tests: json: Validate that attribute values are properly stolen
On Fri, Mar 30, 2018 at 12:59:09 +0200, Peter Krempa wrote:
Make sure that the 'a' and 'A' modifiers for
virJSONValueObjectAddVArgs
behave correctly.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
If you apply this patch without the fix first you get the following
error in valgrind:
$ ./run valgrind tests/virjsontest
==164888== Memcheck, a memory error detector
==164888== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==164888== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==164888== Command: /home/pipo/libvirt/tests/.libs/virjsontest
==164888==
TEST: virjsontest
........................................ 40
.==164888== Invalid free() / delete / delete[] / realloc()
==164888== at 0x4C2E1AB: free (vg_replace_malloc.c:530)
==164888== by 0x4EBD808: virFree (viralloc.c:582)
==164888== by 0x4EF6ACE: virJSONValueFree (virjson.c:382)
==164888== by 0x4EF6AAC: virJSONValueFree (virjson.c:362)
==164888== by 0x4EF88A1: virJSONValueObjectCreateVArgs (virjson.c:329)
==164888== by 0x4EF894B: virJSONValueObjectCreate (virjson.c:344)
==164888== by 0x10BDFB: testJSONObjectFormatSteal (virjsontest.c:446)
==164888== by 0x10D74E: virTestRun (testutils.c:180)
==164888== by 0x10B929: mymain (virjsontest.c:636)
==164888== by 0x10E92F: virTestMain (testutils.c:1119)
==164888== by 0x10CE58: main (virjsontest.c:656)
==164888== Address 0x1ffefffa70 is on thread 1's stack
==164888== in frame #6, created by testJSONObjectFormatSteal (virjsontest.c:434)
==164888==
!.......... 52 FAIL
4:07 p.m.
New subject: [libvirt] [PATCH 2/9] tests: json: Validate that attribute values are properly stolen
On Fri, Mar 30, 2018 at 12:59:09PM +0200, Peter Krempa wrote:
Make sure that the 'a' and 'A' modifiers for
virJSONValueObjectAddVArgs
behave correctly.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
tests/virjsontest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 47 insertions(+)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 3/9] qemu: monitor: Use virJSONValueObjectKeysNumber in qemuMonitorJSONGetCPUModelExpansion
Replace direct access to virJSONValue members by accessor.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_monitor_json.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index f38d04f663..c94d2ef4b8 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -5443,7 +5443,7 @@ qemuMonitorJSONGetCPUModelExpansion(qemuMonitorPtr mon,
if (VIR_STRDUP(machine_model->name, cpu_name) < 0)
goto cleanup;
- if (VIR_ALLOC_N(machine_model->props, cpu_props->data.object.npairs) < 0)
+ if (VIR_ALLOC_N(machine_model->props, virJSONValueObjectKeysNumber(cpu_props))
< 0)
goto cleanup;
if (virJSONValueObjectForeachKeyValue(cpu_props,
--
2.16.2
4:08 p.m.
New subject: [libvirt] [PATCH 3/9] qemu: monitor: Use virJSONValueObjectKeysNumber in qemuMonitorJSONGetCPUModelExpansion
On Fri, Mar 30, 2018 at 12:59:10PM +0200, Peter Krempa wrote:
Replace direct access to virJSONValue members by accessor.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_monitor_json.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 4/9] qemu: agent: Avoid unnecessary JSON object type check
Use virJSONValueObjectGetArray instead of virJSONValueObjectGet so that
it's not necessary to check whether it's an array.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_agent.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index b99d5b10e3..dfec57d992 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -1502,18 +1502,12 @@ qemuAgentGetVCPUs(qemuAgentPtr mon,
VIR_DOMAIN_QEMU_AGENT_COMMAND_BLOCK) < 0)
goto cleanup;
- if (!(data = virJSONValueObjectGet(reply, "return"))) {
+ if (!(data = virJSONValueObjectGetArray(reply, "return"))) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest-get-vcpus reply was missing return data"));
goto cleanup;
}
- if (data->type != VIR_JSON_TYPE_ARRAY) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("guest-get-vcpus return information was not an
array"));
- goto cleanup;
- }
-
ndata = virJSONValueArraySize(data);
if (VIR_ALLOC_N(*info, ndata) < 0)
--
2.16.2
4:09 p.m.
New subject: [libvirt] [PATCH 4/9] qemu: agent: Avoid unnecessary JSON object type check
On Fri, Mar 30, 2018 at 12:59:11PM +0200, Peter Krempa wrote:
Use virJSONValueObjectGetArray instead of virJSONValueObjectGet so
that
it's not necessary to check whether it's an array.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_agent.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 5/9] json: Replace access to virJSONValue->type by virJSONValueGetType
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_agent.c | 6 +++---
src/qemu/qemu_monitor_json.c | 18 +++++++++---------
src/util/virqemu.c | 5 +++--
3 files changed, 15 insertions(+), 14 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c
index dfec57d992..85af53d194 100644
--- a/src/qemu/qemu_agent.c
+++ b/src/qemu/qemu_agent.c
@@ -336,7 +336,7 @@ qemuAgentIOProcessLine(qemuAgentPtr mon,
goto cleanup;
}
- if (obj->type != VIR_JSON_TYPE_OBJECT) {
+ if (virJSONValueGetType(obj) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Parsed JSON reply '%s' isn't an object"),
line);
goto cleanup;
@@ -1872,7 +1872,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
goto cleanup;
}
- if (data->type != VIR_JSON_TYPE_ARRAY) {
+ if (virJSONValueGetType(data) != VIR_JSON_TYPE_ARRAY) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest-get-fsinfo return information was not "
"an array"));
@@ -1931,7 +1931,7 @@ qemuAgentGetFSInfo(qemuAgentPtr mon, virDomainFSInfoPtr **info,
goto cleanup;
}
- if (entry->type != VIR_JSON_TYPE_ARRAY) {
+ if (virJSONValueGetType(entry) != VIR_JSON_TYPE_ARRAY) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("guest-get-fsinfo 'disk' data was not an
array"));
goto cleanup;
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index c94d2ef4b8..de915eabb4 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -197,7 +197,7 @@ qemuMonitorJSONIOProcessLine(qemuMonitorPtr mon,
if (!(obj = virJSONValueFromString(line)))
goto cleanup;
- if (obj->type != VIR_JSON_TYPE_OBJECT) {
+ if (virJSONValueGetType(obj) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("Parsed JSON reply '%s' isn't an object"),
line);
goto cleanup;
@@ -1978,7 +1978,7 @@ qemuMonitorJSONGetBlockDev(virJSONValuePtr devices,
{
virJSONValuePtr dev = virJSONValueArrayGet(devices, idx);
- if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
+ if (!dev || virJSONValueGetType(dev) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("query-block device entry was not in expected
format"));
return NULL;
@@ -2197,7 +2197,7 @@ qemuMonitorJSONGetAllBlockStatsInfo(qemuMonitorPtr mon,
virJSONValuePtr dev = virJSONValueArrayGet(devices, i);
const char *dev_name;
- if (!dev || dev->type != VIR_JSON_TYPE_OBJECT) {
+ if (!dev || virJSONValueGetType(dev) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("blockstats device entry was not "
"in expected format"));
@@ -3276,7 +3276,7 @@ qemuMonitorJSONGetDumpGuestMemoryCapability(qemuMonitorPtr mon,
for (i = 0; i < virJSONValueArraySize(formats); i++) {
virJSONValuePtr dumpformat = virJSONValueArrayGet(formats, i);
- if (!dumpformat || dumpformat->type != VIR_JSON_TYPE_STRING) {
+ if (!dumpformat || virJSONValueGetType(dumpformat) != VIR_JSON_TYPE_STRING) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing entry in supported dump formats"));
goto cleanup;
@@ -4791,7 +4791,7 @@ qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result,
virJSONValuePtr inserted;
const char *current_dev;
- if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) {
+ if (!temp_dev || virJSONValueGetType(temp_dev) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("block_io_throttle device entry "
"was not in expected format"));
@@ -5261,7 +5261,7 @@ qemuMonitorJSONGetCPUDefinitions(qemuMonitorPtr mon,
for (j = 0; j < len; j++) {
virJSONValuePtr blocker = virJSONValueArrayGet(blockers, j);
- if (blocker->type != VIR_JSON_TYPE_STRING) {
+ if (virJSONValueGetType(blocker) != VIR_JSON_TYPE_STRING) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unexpected value in unavailable-features
"
"array"));
@@ -5304,7 +5304,7 @@ qemuMonitorJSONParseCPUModelProperty(const char *key,
prop = machine_model->props + machine_model->nprops;
- switch ((virJSONType) value->type) {
+ switch ((virJSONType) virJSONValueGetType(value)) {
case VIR_JSON_TYPE_STRING:
if (VIR_STRDUP(prop->value.string, virJSONValueGetString(value)) < 0)
return -1;
@@ -6193,7 +6193,7 @@ qemuMonitorJSONGetMigrationCapabilities(qemuMonitorPtr mon,
virJSONValuePtr cap = virJSONValueArrayGet(caps, i);
const char *name;
- if (!cap || cap->type != VIR_JSON_TYPE_OBJECT) {
+ if (!cap || virJSONValueGetType(cap) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing entry in migration capabilities list"));
goto cleanup;
@@ -6343,7 +6343,7 @@ qemuMonitorJSONGetGICCapabilities(qemuMonitorPtr mon,
bool kernel;
bool emulated;
- if (!cap || cap->type != VIR_JSON_TYPE_OBJECT) {
+ if (!cap || virJSONValueGetType(cap) != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("missing entry in GIC capabilities list"));
goto cleanup;
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index 2e9e65f9ef..e7ea068b94 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -146,16 +146,17 @@ virQEMUBuildCommandLineJSONRecurse(const char *key,
bool nested)
{
struct virQEMUCommandLineJSONIteratorData data = { key, buf, arrayFunc };
+ virJSONType type = virJSONValueGetType(value);
virJSONValuePtr elem;
size_t i;
- if (!key && value->type != VIR_JSON_TYPE_OBJECT) {
+ if (!key && type != VIR_JSON_TYPE_OBJECT) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("only JSON objects can be top level"));
return -1;
}
- switch ((virJSONType) value->type) {
+ switch (type) {
case VIR_JSON_TYPE_STRING:
virBufferAsprintf(buf, "%s=", key);
virQEMUBuildBufferEscapeComma(buf, value->data.string);
--
2.16.2
4:11 p.m.
New subject: [libvirt] [PATCH 5/9] json: Replace access to virJSONValue->type by virJSONValueGetType
On Fri, Mar 30, 2018 at 12:59:12PM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_agent.c | 6 +++---
src/qemu/qemu_monitor_json.c | 18 +++++++++---------
src/util/virqemu.c | 5 +++--
3 files changed, 15 insertions(+), 14 deletions(-)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 6/9] util: json: Add accessor for geting a VIR_JSON_TYPE_NUMBER as string
Sometimes it's desired to get a JSON number as string. Add a helper.
This will help in cases where we'd want to conver the internal type from
string to something else.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virjson.c | 10 ++++++++++
src/util/virjson.h | 1 +
3 files changed, 12 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 03fe3b315f..989411cdca 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2038,6 +2038,7 @@ virJSONValueGetBoolean;
virJSONValueGetNumberDouble;
virJSONValueGetNumberInt;
virJSONValueGetNumberLong;
+virJSONValueGetNumberString;
virJSONValueGetNumberUint;
virJSONValueGetNumberUlong;
virJSONValueGetString;
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 212c158da0..6f2b52257f 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -1043,6 +1043,16 @@ virJSONValueGetString(virJSONValuePtr string)
}
+const char *
+virJSONValueGetNumberString(virJSONValuePtr number)
+{
+ if (number->type != VIR_JSON_TYPE_NUMBER)
+ return NULL;
+
+ return number->data.number;
+}
+
+
int
virJSONValueGetNumberInt(virJSONValuePtr number,
int *value)
diff --git a/src/util/virjson.h b/src/util/virjson.h
index 017a1f20ed..e80d10dea1 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -134,6 +134,7 @@ const char *virJSONValueObjectGetKey(virJSONValuePtr object, unsigned
int n);
virJSONValuePtr virJSONValueObjectGetValue(virJSONValuePtr object, unsigned int n);
const char *virJSONValueGetString(virJSONValuePtr object);
+const char *virJSONValueGetNumberString(virJSONValuePtr number);
int virJSONValueGetNumberInt(virJSONValuePtr object, int *value);
int virJSONValueGetNumberUint(virJSONValuePtr object, unsigned int *value);
int virJSONValueGetNumberLong(virJSONValuePtr object, long long *value);
--
2.16.2
4:13 p.m.
New subject: [libvirt] [PATCH 6/9] util: json: Add accessor for geting a VIR_JSON_TYPE_NUMBER as string
On Fri, Mar 30, 2018 at 12:59:13PM +0200, Peter Krempa wrote:
Sometimes it's desired to get a JSON number as string. Add a
helper.
This will help in cases where we'd want to conver the internal type from
s/conver/convert/
string to something else.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libvirt_private.syms | 1 +
src/util/virjson.c | 10 ++++++++++
src/util/virjson.h | 1 +
3 files changed, 12 insertions(+)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 7/9] util: qemu: Don't access virJSONValue members directly in virQEMUBuildCommandLineJSONRecurse
Use the accessors instead.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virqemu.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c
index e7ea068b94..d6652262fe 100644
--- a/src/util/virqemu.c
+++ b/src/util/virqemu.c
@@ -148,6 +148,7 @@ virQEMUBuildCommandLineJSONRecurse(const char *key,
struct virQEMUCommandLineJSONIteratorData data = { key, buf, arrayFunc };
virJSONType type = virJSONValueGetType(value);
virJSONValuePtr elem;
+ bool tmp;
size_t i;
if (!key && type != VIR_JSON_TYPE_OBJECT) {
@@ -159,16 +160,17 @@ virQEMUBuildCommandLineJSONRecurse(const char *key,
switch (type) {
case VIR_JSON_TYPE_STRING:
virBufferAsprintf(buf, "%s=", key);
- virQEMUBuildBufferEscapeComma(buf, value->data.string);
+ virQEMUBuildBufferEscapeComma(buf, virJSONValueGetString(value));
virBufferAddLit(buf, ",");
break;
case VIR_JSON_TYPE_NUMBER:
- virBufferAsprintf(buf, "%s=%s,", key, value->data.number);
+ virBufferAsprintf(buf, "%s=%s,", key,
virJSONValueGetNumberString(value));
break;
case VIR_JSON_TYPE_BOOLEAN:
- if (value->data.boolean)
+ virJSONValueGetBoolean(value, &tmp);
+ if (tmp)
virBufferAsprintf(buf, "%s=yes,", key);
else
virBufferAsprintf(buf, "%s=no,", key);
--
2.16.2
4:15 p.m.
New subject: [libvirt] [PATCH 7/9] util: qemu: Don't access virJSONValue members directly in virQEMUBuildCommandLineJSONRecurse
You might be overdoing it with the prefixes.
On Fri, Mar 30, 2018 at 12:59:14PM +0200, Peter Krempa wrote:
Use the accessors instead.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virqemu.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 8/9] qemu: monitor: Don't resist stealing 'actions' in qemuMonitorJSONTransaction
Rather than trying to prevent stealing of the 'actions' virJSONValue
into the monitor command replace the code so that it does the same
thing, since 'actions' was actually not really used after calling the
monitor.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor.h | 2 +-
src/qemu/qemu_monitor_json.c | 9 ++-------
src/qemu/qemu_monitor_json.h | 2 +-
5 files changed, 8 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 7bcc4936de..8f1d58ba71 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -14811,7 +14811,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
if (qemuDomainObjEnterMonitorAsync(driver, vm, asyncJob) < 0)
goto cleanup;
- ret = qemuMonitorTransaction(priv->mon, actions);
+ ret = qemuMonitorTransaction(priv->mon, &actions);
if (qemuDomainObjExitMonitor(driver, vm) < 0 || ret < 0) {
ret = -1;
@@ -14855,7 +14855,7 @@ qemuDomainSnapshotCreateDiskActive(virQEMUDriverPtr driver,
}
}
- if (ret == 0 || !actions) {
+ if (ret == 0 || !do_transaction) {
if (virDomainSaveStatus(driver->xmlopt, cfg->stateDir, vm, driver->caps)
< 0 ||
(persist && virDomainSaveConfig(cfg->configDir, driver->caps,
vm->newDef) < 0))
diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c
index e169553b7e..7b647525b3 100644
--- a/src/qemu/qemu_monitor.c
+++ b/src/qemu/qemu_monitor.c
@@ -3386,9 +3386,9 @@ qemuMonitorDriveMirror(qemuMonitorPtr mon,
/* Use the transaction QMP command to run atomic snapshot commands. */
int
-qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
+qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions)
{
- VIR_DEBUG("actions=%p", actions);
+ VIR_DEBUG("actions=%p", *actions);
QEMU_CHECK_MONITOR_JSON(mon);
diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h
index 7a22323504..d04148e568 100644
--- a/src/qemu/qemu_monitor.h
+++ b/src/qemu/qemu_monitor.h
@@ -898,7 +898,7 @@ int qemuMonitorDiskSnapshot(qemuMonitorPtr mon,
const char *file,
const char *format,
bool reuse);
-int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
+int qemuMonitorTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions)
ATTRIBUTE_NONNULL(2);
int qemuMonitorDriveMirror(qemuMonitorPtr mon,
const char *device,
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index de915eabb4..1fd09a3398 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -4125,18 +4125,14 @@ qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
}
int
-qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
+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", &act,
+ "a:actions", actions,
NULL);
if (!cmd)
goto cleanup;
@@ -4151,7 +4147,6 @@ qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr
actions)
cleanup:
virJSONValueFree(cmd);
virJSONValueFree(reply);
- actions->protect = protect;
return ret;
}
diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h
index 846d366b27..045df4919f 100644
--- a/src/qemu/qemu_monitor_json.h
+++ b/src/qemu/qemu_monitor_json.h
@@ -253,7 +253,7 @@ int qemuMonitorJSONDiskSnapshot(qemuMonitorPtr mon,
bool reuse)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(3)
ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5);
-int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr actions)
+int qemuMonitorJSONTransaction(qemuMonitorPtr mon, virJSONValuePtr *actions)
ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
int qemuMonitorJSONDriveMirror(qemuMonitorPtr mon,
const char *device,
--
2.16.2
4:21 p.m.
New subject: [libvirt] [PATCH 8/9] qemu: monitor: Don't resist stealing 'actions' in qemuMonitorJSONTransaction
On Fri, Mar 30, 2018 at 12:59:15PM +0200, Peter Krempa wrote:
Rather than trying to prevent stealing of the 'actions'
virJSONValue
into the monitor command replace the code so that it does the same
thing, since 'actions' was actually not really used after calling the
monitor.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/qemu/qemu_driver.c | 4 ++--
src/qemu/qemu_monitor.c | 4 ++--
src/qemu/qemu_monitor.h | 2 +-
src/qemu/qemu_monitor_json.c | 9 ++-------
src/qemu/qemu_monitor_json.h | 2 +-
5 files changed, 8 insertions(+), 13 deletions(-)
ACK
Jano
5:59 a.m.
New subject: [libvirt] [PATCH 9/9] util: json: Privatize struct _virJSONValue and sub-structs
Enforce usage of accessors by hiding the implementation in the code.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virjson.c | 39 +++++++++++++++++++++++++++++++++++++++
src/util/virjson.h | 38 --------------------------------------
2 files changed, 39 insertions(+), 38 deletions(-)
diff --git a/src/util/virjson.c b/src/util/virjson.c
index 6f2b52257f..3ddefc34ca 100644
--- a/src/util/virjson.c
+++ b/src/util/virjson.c
@@ -51,6 +51,45 @@
VIR_LOG_INIT("util.json");
+typedef struct _virJSONObject virJSONObject;
+typedef virJSONObject *virJSONObjectPtr;
+
+typedef struct _virJSONObjectPair virJSONObjectPair;
+typedef virJSONObjectPair *virJSONObjectPairPtr;
+
+typedef struct _virJSONArray virJSONArray;
+typedef virJSONArray *virJSONArrayPtr;
+
+
+struct _virJSONObjectPair {
+ char *key;
+ virJSONValuePtr value;
+};
+
+struct _virJSONObject {
+ size_t npairs;
+ virJSONObjectPairPtr pairs;
+};
+
+struct _virJSONArray {
+ size_t nvalues;
+ virJSONValuePtr *values;
+};
+
+struct _virJSONValue {
+ int type; /* enum virJSONType */
+ bool protect; /* prevents deletion when embedded in another object */
+
+ union {
+ virJSONObject object;
+ virJSONArray array;
+ char *string;
+ char *number; /* int/float/etc format is context defined so we can't parse it
here :-( */
+ int boolean;
+ } data;
+};
+
+
typedef struct _virJSONParserState virJSONParserState;
typedef virJSONParserState *virJSONParserStatePtr;
struct _virJSONParserState {
diff --git a/src/util/virjson.h b/src/util/virjson.h
index e80d10dea1..f7283dcf97 100644
--- a/src/util/virjson.h
+++ b/src/util/virjson.h
@@ -42,44 +42,6 @@ typedef enum {
typedef struct _virJSONValue virJSONValue;
typedef virJSONValue *virJSONValuePtr;
-typedef struct _virJSONObject virJSONObject;
-typedef virJSONObject *virJSONObjectPtr;
-
-typedef struct _virJSONObjectPair virJSONObjectPair;
-typedef virJSONObjectPair *virJSONObjectPairPtr;
-
-typedef struct _virJSONArray virJSONArray;
-typedef virJSONArray *virJSONArrayPtr;
-
-
-struct _virJSONObjectPair {
- char *key;
- virJSONValuePtr value;
-};
-
-struct _virJSONObject {
- size_t npairs;
- virJSONObjectPairPtr pairs;
-};
-
-struct _virJSONArray {
- size_t nvalues;
- virJSONValuePtr *values;
-};
-
-struct _virJSONValue {
- int type; /* enum virJSONType */
- bool protect; /* prevents deletion when embedded in another object */
-
- union {
- virJSONObject object;
- virJSONArray array;
- char *string;
- char *number; /* int/float/etc format is context defined so we can't parse it
here :-( */
- int boolean;
- } data;
-};
-
void virJSONValueFree(virJSONValuePtr value);
void virJSONValueHashFree(void *opaque, const void *name);
--
2.16.2
4:23 p.m.
New subject: [libvirt] [PATCH 9/9] util: json: Privatize struct _virJSONValue and sub-structs
On Fri, Mar 30, 2018 at 12:59:16PM +0200, Peter Krempa wrote:
Enforce usage of accessors by hiding the implementation in the code.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virjson.c | 39 +++++++++++++++++++++++++++++++++++++++
src/util/virjson.h | 38 --------------------------------------
2 files changed, 39 insertions(+), 38 deletions(-)
ACK
Jano
6:05 a.m.
On Fri, Mar 30, 2018 at 12:59:07 +0200, Peter Krempa wrote:
Coverity was not wrong about the usage of 'a'/'A'
modifiers for
virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
leak/double-free, and add test to make sure it works as expected.
[1] https://www.redhat.com/archives/libvir-list/2017-November/msg00306.html
5:56 a.m.
On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
Coverity was not wrong about the usage of 'a'/'A'
modifiers for
virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
leak/double-free, and add test to make sure it works as expected.
Do you have any idea how to trigger the double-free scenario ? In particular
is it something that a broken / malicious QEMU monitor / guest agent can
cause us todo. If so we'll need to request a CVE assignment for this flaw.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
6:01 a.m.
On Tue, Apr 03, 2018 at 11:56:33 +0100, Daniel Berrange wrote:
On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> Coverity was not wrong about the usage of 'a'/'A' modifiers for
> virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> leak/double-free, and add test to make sure it works as expected.
Do you have any idea how to trigger the double-free scenario ? In particular
is it something that a broken / malicious QEMU monitor / guest agent can
cause us todo. If so we'll need to request a CVE assignment for this flaw.
It can be triggered when we are formatting the virJSONValue using
callers of virJSONValueObjectAddVArgs, so it would require malformed
parameters to a libvirt API and I did not inspec the possibility of
that.
You can see the paths which potentially could hit the double-free in
this patch, since it's modifying all the cases.
All paths where we format anything after an 'a' or 'A' valuea which can
fail after the 'a' or 'A' was successful and then the original value is
freed.
I doubt that it's CVE-material
6:07 a.m.
On Tue, Apr 03, 2018 at 13:01:48 +0200, Peter Krempa wrote:
On Tue, Apr 03, 2018 at 11:56:33 +0100, Daniel Berrange wrote:
> On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> > Coverity was not wrong about the usage of 'a'/'A' modifiers
for
> > virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> > leak/double-free, and add test to make sure it works as expected.
>
> Do you have any idea how to trigger the double-free scenario ? In particular
> is it something that a broken / malicious QEMU monitor / guest agent can
> cause us todo. If so we'll need to request a CVE assignment for this flaw.
It can be triggered when we are formatting the virJSONValue using
callers of virJSONValueObjectAddVArgs, so it would require malformed
parameters to a libvirt API and I did not inspec the possibility of
that.
You can see the paths which potentially could hit the double-free in
this patch, since it's modifying all the cases.
All paths where we format anything after an 'a' or 'A' valuea which can
fail after the 'a' or 'A' was successful and then the original value is
freed.
So the calls which match the above condition are from:
qemuBlockStorageSourceGetNBDProps,
qemuBlockStorageSourceGetRBDProps,
qemuBlockStorageSourceGetSshProps,
qemuMonitorJSONSendKey
All of the above format only optional strings ('S') or integers after
the 'a'/'A' argument, so the double free scenario would happen only on a
out-of-memory condition.
I doubt that it's CVE-material
It's not.
6:11 a.m.
On Tue, Apr 03, 2018 at 01:07:29PM +0200, Peter Krempa wrote:
On Tue, Apr 03, 2018 at 13:01:48 +0200, Peter Krempa wrote:
> On Tue, Apr 03, 2018 at 11:56:33 +0100, Daniel Berrange wrote:
> > On Fri, Mar 30, 2018 at 12:59:07PM +0200, Peter Krempa wrote:
> > > Coverity was not wrong about the usage of 'a'/'A'
modifiers for
> > > virJSONValueObjectAddVArgs as noted in [1]. Fix the possible
> > > leak/double-free, and add test to make sure it works as expected.
> >
> > Do you have any idea how to trigger the double-free scenario ? In particular
> > is it something that a broken / malicious QEMU monitor / guest agent can
> > cause us todo. If so we'll need to request a CVE assignment for this flaw.
>
> It can be triggered when we are formatting the virJSONValue using
> callers of virJSONValueObjectAddVArgs, so it would require malformed
> parameters to a libvirt API and I did not inspec the possibility of
> that.
>
> You can see the paths which potentially could hit the double-free in
> this patch, since it's modifying all the cases.
>
> All paths where we format anything after an 'a' or 'A' valuea which
can
> fail after the 'a' or 'A' was successful and then the original value
is
> freed.
So the calls which match the above condition are from:
qemuBlockStorageSourceGetNBDProps,
qemuBlockStorageSourceGetRBDProps,
qemuBlockStorageSourceGetSshProps,
qemuMonitorJSONSendKey
All of the above format only optional strings ('S') or integers after
the 'a'/'A' argument, so the double free scenario would happen only on a
out-of-memory condition.
> I doubt that it's CVE-material
It's not.
Great, thanks for checking
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
2424
days inactive
2428
days old
24 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa