[libvirt] [PATCH 0/9] json: Fix leak/double-free, clean up code and privatize virJSONValue
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
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> --- 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
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@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
Make sure that the 'a' and 'A' modifiers for virJSONValueObjectAddVArgs behave correctly. Signed-off-by: Peter Krempa <pkrempa@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
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@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
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@redhat.com> --- tests/virjsontest.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 47 insertions(+)
ACK Jano
Replace direct access to virJSONValue members by accessor. Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> --- src/qemu/qemu_monitor_json.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK Jano
Use virJSONValueObjectGetArray instead of virJSONValueObjectGet so that it's not necessary to check whether it's an array. Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> --- src/qemu/qemu_agent.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)
ACK Jano
Signed-off-by: Peter Krempa <pkrempa@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
On Fri, Mar 30, 2018 at 12:59:12PM +0200, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@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
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@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
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@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virjson.c | 10 ++++++++++ src/util/virjson.h | 1 + 3 files changed, 12 insertions(+)
ACK Jano
Use the accessors instead. Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> --- src/util/virqemu.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
ACK Jano
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@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
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@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
Enforce usage of accessors by hiding the implementation in the code. Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> --- src/util/virjson.c | 39 +++++++++++++++++++++++++++++++++++++++ src/util/virjson.h | 38 -------------------------------------- 2 files changed, 39 insertions(+), 38 deletions(-)
ACK Jano
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
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 :|
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
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.
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 :|
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa