Commit id '43f2ccdc' called virDomainDiskSourceDefFormatInternal rather than formatting the the disk source inline. However, it did not handle the case where the helper failed. Over time the helper has been renamed to virDomainDiskSourceFormat. Similar to other consumers, if virDomainDiskSourceFormat fails, then the formatting could be off, so it's better to fail than to continue on with some possibly bad data. Alter the function and the caller to check status and jump to error in that case. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/conf/snapshot_conf.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index d7b086242b..787c3d0feb 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -662,7 +662,7 @@ virDomainSnapshotAlignDisks(virDomainSnapshotDefPtr def, return ret; } -static void +static int virDomainSnapshotDiskDefFormat(virBufferPtr buf, virDomainSnapshotDiskDefPtr disk, virDomainXMLOptionPtr xmlopt) @@ -670,7 +670,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, int type = disk->src->type; if (!disk->name) - return; + return 0; virBufferEscapeString(buf, "<disk name='%s'", disk->name); if (disk->snapshot > 0) @@ -679,7 +679,7 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (!disk->src->path && disk->src->format == 0) { virBufferAddLit(buf, "/>\n"); - return; + return 0; } virBufferAsprintf(buf, " type='%s'>\n", virStorageTypeToString(type)); @@ -688,10 +688,12 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (disk->src->format > 0) virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src->format)); - virDomainDiskSourceFormat(buf, disk->src, 0, 0, xmlopt); + if (virDomainDiskSourceFormat(buf, disk->src, 0, 0, xmlopt) < 0) + return -1; virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</disk>\n"); + return 0; } @@ -741,8 +743,10 @@ virDomainSnapshotDefFormat(const char *domain_uuid, if (def->ndisks) { virBufferAddLit(&buf, "<disks>\n"); virBufferAdjustIndent(&buf, 2); - for (i = 0; i < def->ndisks; i++) - virDomainSnapshotDiskDefFormat(&buf, &def->disks[i], xmlopt); + for (i = 0; i < def->ndisks; i++) { + if (virDomainSnapshotDiskDefFormat(&buf, &def->disks[i], xmlopt) < 0) + goto error; + } virBufferAdjustIndent(&buf, -2); virBufferAddLit(&buf, "</disks>\n"); } -- 2.13.6

Move to within the #if since the #else portion ends with a goto and that raised concern by Coverity. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/remote/remote_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index d3b588c374..12f7d47388 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -928,6 +928,7 @@ doRemoteOpen(virConnectPtr conn, if (!priv->tls) goto failed; priv->is_secure = 1; + ATTRIBUTE_FALLTHROUGH; #else (void)tls_priority; (void)sanity; @@ -937,7 +938,6 @@ doRemoteOpen(virConnectPtr conn, goto failed; #endif - ATTRIBUTE_FALLTHROUGH; case trans_tcp: priv->client = virNetClientNewTCP(priv->hostname, port, AF_UNSPEC); if (!priv->client) -- 2.13.6

If @log is not fopen'd then, going to cleanup and calling fclose will make for an unhappy caller. So just fail immediately instead since there's nothing to clean up. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/commandhelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/commandhelper.c b/tests/commandhelper.c index 1da2834aa4..bf91550ede 100644 --- a/tests/commandhelper.c +++ b/tests/commandhelper.c @@ -67,7 +67,7 @@ int main(int argc, char **argv) { int ret = EXIT_FAILURE; if (!log) - goto cleanup; + return ret; for (i = 1; i < argc; i++) fprintf(log, "ARG:%s\n", argv[i]); -- 2.13.6

If @client hasn't been opened, then don't call virNetServerClientClose since that'll cause certain failure. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/virnetserverclienttest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/virnetserverclienttest.c b/tests/virnetserverclienttest.c index 398b46928c..aa3f0bcf9b 100644 --- a/tests/virnetserverclienttest.c +++ b/tests/virnetserverclienttest.c @@ -152,7 +152,8 @@ static int testIdentity(const void *opaque ATTRIBUTE_UNUSED) ret = 0; cleanup: virObjectUnref(sock); - virNetServerClientClose(client); + if (!client) + virNetServerClientClose(client); virObjectUnref(client); virObjectUnref(ident); VIR_FORCE_CLOSE(sv[0]); -- 2.13.6

If virJSONValueObjectGetArray fails to find "members" in @localroot, then using @rootmembers in subsequent calls which assume that it was successful will not go well. So add a check that it was successfully fetched and an error if not. Similarly virJSONValueArraySize returns an 'ssize_t', so the callers should use the right type and they should check the return value against -1 and print an error if something is wrong. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- tests/testutilsqemuschema.c | 40 +++++++++++++++++++++++++++++----------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/tests/testutilsqemuschema.c b/tests/testutilsqemuschema.c index 21f5d119e8..79c526ac7c 100644 --- a/tests/testutilsqemuschema.c +++ b/tests/testutilsqemuschema.c @@ -97,14 +97,19 @@ struct testQEMUSchemaValidateObjectMemberData { static virJSONValuePtr testQEMUSchemaStealObjectMemberByName(const char *name, - virJSONValuePtr members) + virJSONValuePtr members, + virBufferPtr debug) { virJSONValuePtr member; virJSONValuePtr ret = NULL; - size_t n; + ssize_t n; size_t i; - n = virJSONValueArraySize(members); + if ((n = virJSONValueArraySize(members)) < 0) { + virBufferAddLit(debug, "ERROR: members size < 0"); + return NULL; + } + for (i = 0; i < n; i++) { member = virJSONValueArrayGet(members, i); @@ -132,7 +137,7 @@ testQEMUSchemaValidateObjectMember(const char *key, virBufferStrcat(data->debug, key, ": ", NULL); /* lookup 'member' entry for key */ - if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers))) { + if (!(keymember = testQEMUSchemaStealObjectMemberByName(key, data->rootmembers, data->debug))) { virBufferAddLit(data->debug, "ERROR: attribute not in schema"); goto cleanup; } @@ -188,7 +193,7 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, virHashTablePtr schema, virBufferPtr debug) { - size_t n; + ssize_t n; size_t i; virJSONValuePtr variants = NULL; virJSONValuePtr variant; @@ -203,7 +208,11 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, return -2; } - n = virJSONValueArraySize(variants); + if ((n = virJSONValueArraySize(variants)) < 0) { + virBufferAddLit(debug, "ERROR: 'variants' array size < 0"); + return -2; + } + for (i = 0; i < n; i++) { variant = virJSONValueArrayGet(variants, i); @@ -307,7 +316,10 @@ testQEMUSchemaValidateObject(virJSONValuePtr obj, /* validate members */ - data.rootmembers = virJSONValueObjectGetArray(localroot, "members"); + if (!(data.rootmembers = virJSONValueObjectGetArray(localroot, "members"))) { + virBufferAddLit(debug, "ERROR: missing attribute 'members'\n"); + goto cleanup; + } if (virJSONValueObjectForeachKeyValue(obj, testQEMUSchemaValidateObjectMember, &data) < 0) @@ -342,7 +354,7 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj, const char *objstr; virJSONValuePtr values = NULL; virJSONValuePtr value; - size_t n; + ssize_t n; size_t i; if (virJSONValueGetType(obj) != VIR_JSON_TYPE_STRING) { @@ -358,7 +370,10 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj, return -2; } - n = virJSONValueArraySize(values); + if ((n = virJSONValueArraySize(values)) < 0) { + virBufferAddLit(debug, "ERROR: enum values array size < 0"); + return -2; + } for (i = 0; i < n; i++) { value = virJSONValueArrayGet(values, i); @@ -423,7 +438,7 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj, { virJSONValuePtr members; virJSONValuePtr member; - size_t n; + ssize_t n; size_t i; const char *membertype; virJSONValuePtr memberschema; @@ -439,7 +454,10 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj, virBufferAdjustIndent(debug, 3); indent = virBufferGetIndent(debug, false); - n = virJSONValueArraySize(members); + if ((n = virJSONValueArraySize(members)) < 0) { + virBufferAddLit(debug, "ERROR: alternate schema 'members' array size < 0"); + return -2; + } for (i = 0; i < n; i++) { membertype = NULL; -- 2.13.6

If virJSONValueArraySize(caps) <= 0, then we will still need to virJSONValueFree(caps) because qemuMonitorSetMigrationCapabilities won't consume it. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_migration_params.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/qemu/qemu_migration_params.c b/src/qemu/qemu_migration_params.c index 4f3b239637..3bbe50a8ed 100644 --- a/src/qemu/qemu_migration_params.c +++ b/src/qemu/qemu_migration_params.c @@ -771,6 +771,7 @@ qemuMigrationParamsApply(virQEMUDriverPtr driver, migParams->params[xbzrle].set = true; virJSONValueFree(params); + virJSONValueFree(caps); return ret; } -- 2.13.6

If the "return" is not in the reply, then the subsequent virJSONValueGetType will not be happy. Found by Coverity Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_monitor_json.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 410fa178b2..681c0575c3 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -423,7 +423,12 @@ qemuMonitorJSONCheckReply(virJSONValuePtr cmd, if (qemuMonitorJSONCheckError(cmd, reply) < 0) return -1; - data = virJSONValueObjectGet(reply, "return"); + if (!(data = virJSONValueObjectGet(reply, "return"))) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing 'return' for returned data")); + return -1; + } + if (virJSONValueGetType(data) != type) { char *cmdstr = virJSONValueToString(cmd, false); char *retstr = virJSONValueToString(data, false); -- 2.13.6