[libvirt] [PATCH 0/7] Addressing some Coverity complaints...
My current pile is 48 patches - most of those are known false positives or have been posted before and either rejected or ignored. In any case, the following is a list of more recent adjustments from my coverity tree. Seems we've largely moved away from resource leaks and into return value checking issues. Some good, some not so good. John Ferlan (7): conf: Add error checking to virDomainSnapshotDiskDefFormat remote: Fix usage of ATTRIBUTE_FALLTHROUGH tests: Return failure if log not fopen'd tests: Don't call virNetServerClientClose without valid client tests: Add checks for possible errors qemu: Fix possible memory leak in migration param processing qemu: Check for missing 'return' in qemuMonitorJSONCheckReply src/conf/snapshot_conf.c | 16 ++++++++++------ src/qemu/qemu_migration_params.c | 1 + src/qemu/qemu_monitor_json.c | 7 ++++++- src/remote/remote_driver.c | 2 +- tests/commandhelper.c | 2 +- tests/testutilsqemuschema.c | 40 +++++++++++++++++++++++++++++----------- tests/virnetserverclienttest.c | 3 ++- 7 files changed, 50 insertions(+), 21 deletions(-) -- 2.13.6
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:12 -0400, John Ferlan wrote:
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@redhat.com> --- src/conf/snapshot_conf.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
ACK
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:13 -0400, John Ferlan wrote:
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@redhat.com> --- src/remote/remote_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK, although the code is correct as it is, it will just never fall through, so this is again a false positive.
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:14 -0400, John Ferlan wrote:
If @log is not fopen'd then, going to cleanup and calling fclose will make for an unhappy caller. So just fail immediately instead
s/caller/callee/
since there's nothing to clean up.
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/commandhelper.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:15 -0400, John Ferlan wrote:
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@redhat.com> --- tests/virnetserverclienttest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:16 -0400, John Ferlan wrote:
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@redhat.com> --- 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;
All of this bove should be fixed by checking the return value of virJSONValueObjectGetArray in testQEMUSchemaValidateObject where data.rootmembers. This will make sure that the callee gets an array and thus the array size function will not return -1;
}
[...]
@@ -203,7 +208,11 @@ testQEMUSchemaValidateObjectMergeVariant(virJSONValuePtr root, return -2; }
- n = virJSONValueArraySize(variants);
'variants' was retrieved via virJSONValueObjectStealArray thus this will not return -1 as it was type-checked.
+ 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)
[...]
@@ -358,7 +370,10 @@ testQEMUSchemaValidateEnum(virJSONValuePtr obj, return -2; }
- n = virJSONValueArraySize(values);
'values' was retrieved via virJSONValueObjectGetArray thus this will not return -1 as it was type-checked.
+ 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);
[...]
@@ -439,7 +454,10 @@ testQEMUSchemaValidateAlternate(virJSONValuePtr obj, virBufferAdjustIndent(debug, 3); indent = virBufferGetIndent(debug, false);
- n = virJSONValueArraySize(members);
'members' was retrieved via virJSONValueObjectGetArray thus this will not return -1 as it was type-checked.
+ if ((n = virJSONValueArraySize(members)) < 0) { + virBufferAddLit(debug, "ERROR: alternate schema 'members' array size < 0"); + return -2; + } for (i = 0; i < n; i++) { membertype = NULL;
Also all of the error messages are misleading. The schema can't have an array with < 0 elements, since that is not possible in JSON. The problem always will be that the field is either missing or of incorrect type. Rather than adding pointless checks it might be worth just to modify virJSONValueArraySize so that it does not do type checking and offload that to the caller.
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:17 -0400, John Ferlan wrote:
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@redhat.com> --- src/qemu/qemu_migration_params.c | 1 + 1 file changed, 1 insertion(+)
ACK
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@redhat.com> --- 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
On Tue, Apr 17, 2018 at 12:22:18 -0400, John Ferlan wrote:
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@redhat.com> --- 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)
This checks that the reply indeed has a key named 'return'.
return -1;
- data = virJSONValueObjectGet(reply, "return");
So this is not possible to fail.
+ 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
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
John Ferlan -
Peter Krempa