[libvirt] [PATCH 0/9] Various patches as a result of Coverity
Been piling up a few Coverity changes, the later patches aren't anything critical, but the first few are newer and it seems more likely to be hit in common paths. In particular, the first 2 are from recent virconf changes. John Ferlan (9): util: Fix incorrect VIR_FREE in virConfGetValueStringList tools: Fix comparison in virLoginShellGetShellArgv tests: Need to check return of virGetLastError conf: Need to check for glisten before accessing vsh: Properly initialize res esx: Replace strtok_r usage with virStringSplitCount conf: Add ignore_value to virDomainDeviceInfoIterate calls for Clear helpers openvz: Remove need for strtok_r in openvzGenerateContainerVethName openvz: Remove strtok_r calls from openvzReadNetworkConf src/conf/domain_conf.c | 12 ++++++--- src/esx/esx_vi.c | 61 ++++++++++++++++++++-------------------------- src/openvz/openvz_conf.c | 33 +++++++++++++++---------- src/openvz/openvz_driver.c | 16 ++++++++---- src/util/virconf.c | 2 +- tests/qemuhelptest.c | 4 ++- tools/virt-login-shell.c | 6 +++-- tools/vsh.c | 4 +-- 8 files changed, 76 insertions(+), 62 deletions(-) -- 2.5.5
Since we VIR_ALLOC_N to *values, the VIR_FREE should be done likewise Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virconf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virconf.c b/src/util/virconf.c index a41f896..ee54072 100644 --- a/src/util/virconf.c +++ b/src/util/virconf.c @@ -1016,7 +1016,7 @@ int virConfGetValueStringList(virConfPtr conf, return -1; if (cval->str && VIR_STRDUP((*values)[0], cval->str) < 0) { - VIR_FREE(values); + VIR_FREE(*values); return -1; } break; -- 2.5.5
Commit id '740e4d70' altered the logic to fetch the sysconf values and added a new virConfGetValueStringList which returns -1 on failure, 0 if missing, and 1 if the value was present. However, the caller only checked !shargv which caught Coverity's attention since the following VIR_ALLOC_N(*shargv, 2) would be a NULL ptr deref Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virt-login-shell.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index 632d493..a2b32ac 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -99,10 +99,12 @@ static int virLoginShellGetShellArgv(virConfPtr conf, char ***shargv, size_t *shargvlen) { - if (virConfGetValueStringList(conf, "shell", true, shargv) < 0) + int rv; + + if ((rv = virConfGetValueStringList(conf, "shell", true, shargv)) < 0) return -1; - if (!shargv) { + if (rv == 0) { if (VIR_ALLOC_N(*shargv, 2) < 0) return -1; if (VIR_STRDUP((*shargv)[0], "/bin/sh") < 0) { -- 2.5.5
Cannot assume virGetLastError returns non-NULL value - modify the code to fetch err and check if err && err->code Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemuhelptest.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/qemuhelptest.c b/tests/qemuhelptest.c index 8aac997..7c8b841 100644 --- a/tests/qemuhelptest.c +++ b/tests/qemuhelptest.c @@ -60,7 +60,9 @@ static int testHelpStrParsing(const void *data) if (virQEMUCapsParseHelpStr("QEMU", help, flags, &version, &is_kvm, &kvm_version, false, NULL) == -1) { - if (info->error && virGetLastError()->code == info->error) + virErrorPtr err = virGetLastError(); + + if (info->error && err && err->code == info->error) ret = 0; goto cleanup; } -- 2.5.5
When formatting the graphics data for TYPE_SPICE, check if the glisten is NULL before blindly referencing Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 82d9d1d..87a9a8d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -22150,6 +22150,12 @@ virDomainGraphicsDefFormat(virBufferPtr buf, break; case VIR_DOMAIN_GRAPHICS_TYPE_SPICE: + if (!glisten) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing listen element for spice graphics")); + return -1; + } + switch (glisten->type) { case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_NETWORK: -- 2.5.5
The 'res' variable was only being initialized to NULL in the if (!state) path; however, that path never used res and evenutally res is assigned one of two results based on a pair of if then else if conditions. If for some reason neither of those paths was taken and the (!state) path wasn't take, then 'res' would be indeterminate. Found by Coverity, probably a false positive based on code paths, but better safe than sorry for the future. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/vsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/vsh.c b/tools/vsh.c index 68f7785..9ac4f21 100644 --- a/tools/vsh.c +++ b/tools/vsh.c @@ -2636,7 +2636,8 @@ vshReadlineParse(const char *text, int state) vshCommandToken tk; static const vshCmdDef *cmd; const vshCmdOptDef *opt; - char *tkdata, *optstr, *const_tkdata, *res; + char *tkdata, *optstr, *const_tkdata; + char *res = NULL; static char *ctext, *sanitized_text; static uint64_t const_opts_need_arg, const_opts_required, const_opts_seen; uint64_t opts_need_arg, opts_required, opts_seen; @@ -2656,7 +2657,6 @@ vshReadlineParse(const char *text, int state) tkdata = NULL; sanitized_text = NULL; optstr = NULL; - res = NULL; /* Sanitize/de-quote the autocomplete text */ tk = sanitizer.getNextArg(NULL, &sanitizer, &sanitized_text, false); -- 2.5.5
On Mon, Jul 18, 2016 at 03:06:56PM -0400, John Ferlan wrote:
The 'res' variable was only being initialized to NULL in the if (!state) path; however, that path never used res and evenutally res is assigned one of two results based on a pair of if then else if conditions. If for some reason neither of those paths was taken and the (!state) path wasn't take, then 'res' would be indeterminate.
Found by Coverity, probably a false positive based on code paths, but better safe than sorry for the future.
Yes, it seems the function could be made more understandable, but this patch is an improvement. ACK to patches 1 to 5. Jan
Rather than use strtok_r using an input "path" variable which may or may not be NULL and thus alter the results, use the virStringSplitCount API in order to parse the path. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/esx/esx_vi.c | 61 ++++++++++++++++++++++++-------------------------------- 1 file changed, 26 insertions(+), 35 deletions(-) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index a28ac7b..f151dc4 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1179,22 +1179,17 @@ esxVI_Context_LookupManagedObjects(esxVI_Context *ctx) int esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) { + char **tmp = NULL; + size_t idx, ntmp = 0; int result = -1; - char *tmp = NULL; - char *saveptr = NULL; - char *previousItem = NULL; - char *item = NULL; virBuffer buffer = VIR_BUFFER_INITIALIZER; esxVI_ManagedObjectReference *root = NULL; esxVI_Folder *folder = NULL; - if (VIR_STRDUP(tmp, path) < 0) + if (!(tmp = virStringSplitCount(path, "/", 0, &ntmp))) goto cleanup; - /* Lookup Datacenter */ - item = strtok_r(tmp, "/", &saveptr); - - if (!item) { + if (ntmp == 0) { virReportError(VIR_ERR_INVALID_ARG, _("Path '%s' does not specify a datacenter"), path); goto cleanup; @@ -1202,11 +1197,12 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = ctx->service->rootFolder; - while (!ctx->datacenter && item) { + for (idx = 0; idx < ntmp && !ctx->datacenter; ++idx) { + esxVI_Folder_Free(&folder); - /* Try to lookup item as a folder */ - if (esxVI_LookupFolder(ctx, item, root, NULL, &folder, + /* Try to lookup entry as a folder */ + if (esxVI_LookupFolder(ctx, tmp[idx], root, NULL, &folder, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -1219,8 +1215,9 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = folder->_reference; folder->_reference = NULL; } else { - /* Try to lookup item as a datacenter */ - if (esxVI_LookupDatacenter(ctx, item, root, NULL, &ctx->datacenter, + /* Try to lookup entry as a datacenter */ + if (esxVI_LookupDatacenter(ctx, tmp[idx], root, NULL, + &ctx->datacenter, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -1230,10 +1227,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) if (virBufferUse(&buffer) > 0) virBufferAddChar(&buffer, '/'); - virBufferAdd(&buffer, item, -1); - - previousItem = item; - item = strtok_r(NULL, "/", &saveptr); + virBufferAdd(&buffer, tmp[idx], -1); } if (!ctx->datacenter) { @@ -1248,9 +1242,10 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) ctx->datacenterPath = virBufferContentAndReset(&buffer); /* Lookup (Cluster)ComputeResource */ - if (!item) { + if (!tmp[idx]) { virReportError(VIR_ERR_INVALID_ARG, - _("Path '%s' does not specify a compute resource"), path); + _("Path '%s' does not specify a compute resource"), + path); goto cleanup; } @@ -1259,11 +1254,11 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = ctx->datacenter->hostFolder; - while (!ctx->computeResource && item) { + for (; idx < ntmp && !ctx->computeResource; ++idx) { esxVI_Folder_Free(&folder); - /* Try to lookup item as a folder */ - if (esxVI_LookupFolder(ctx, item, root, NULL, &folder, + /* Try to lookup entry as a folder */ + if (esxVI_LookupFolder(ctx, tmp[idx], root, NULL, &folder, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; } @@ -1276,8 +1271,8 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) root = folder->_reference; folder->_reference = NULL; } else { - /* Try to lookup item as a compute resource */ - if (esxVI_LookupComputeResource(ctx, item, root, NULL, + /* Try to lookup entry as a compute resource */ + if (esxVI_LookupComputeResource(ctx, tmp[idx], root, NULL, &ctx->computeResource, esxVI_Occurrence_OptionalItem) < 0) { goto cleanup; @@ -1288,10 +1283,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) if (virBufferUse(&buffer) > 0) virBufferAddChar(&buffer, '/'); - virBufferAdd(&buffer, item, -1); - - previousItem = item; - item = strtok_r(NULL, "/", &saveptr); + virBufferAdd(&buffer, tmp[idx], -1); } if (!ctx->computeResource) { @@ -1315,24 +1307,23 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) /* Lookup HostSystem */ if (STREQ(ctx->computeResource->_reference->type, "ClusterComputeResource")) { - if (!item) { + if (!tmp[idx]) { virReportError(VIR_ERR_INVALID_ARG, _("Path '%s' does not specify a host system"), path); goto cleanup; } /* The path specified a cluster, it has to specify a host system too */ - previousItem = item; - item = strtok_r(NULL, "/", &saveptr); + idx++; } - if (item) { + if (tmp[idx]) { virReportError(VIR_ERR_INVALID_ARG, _("Path '%s' ends with an excess item"), path); goto cleanup; } - if (VIR_STRDUP(ctx->hostSystemName, previousItem) < 0) + if (VIR_STRDUP(ctx->hostSystemName, tmp[idx - 1]) < 0) goto cleanup; if (esxVI_LookupHostSystem(ctx, ctx->hostSystemName, @@ -1359,7 +1350,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) esxVI_ManagedObjectReference_Free(&root); } - VIR_FREE(tmp); + virStringFreeListCount(tmp, ntmp); esxVI_Folder_Free(&folder); return result; -- 2.5.5
Since we don't necessarily care about the status return, just add an ignore_value to the calls for virDomainDefClear* calls Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 87a9a8d..3cf1f59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4736,17 +4736,17 @@ virDomainDefValidate(virDomainDefPtr def, void virDomainDefClearPCIAddresses(virDomainDefPtr def) { - virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); + ignore_value(virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL)); } void virDomainDefClearCCWAddresses(virDomainDefPtr def) { - virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearCCWAddress, NULL); + ignore_value(virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearCCWAddress, NULL)); } void virDomainDefClearDeviceAliases(virDomainDefPtr def) { - virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); + ignore_value(virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL)); } -- 2.5.5
On Mon, Jul 18, 2016 at 03:06:58PM -0400, John Ferlan wrote:
Since we don't necessarily care about the status return, just add an ignore_value to the calls for virDomainDefClear* calls
Found by Coverity
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 87a9a8d..3cf1f59 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -4736,17 +4736,17 @@ virDomainDefValidate(virDomainDefPtr def,
void virDomainDefClearPCIAddresses(virDomainDefPtr def) { - virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL); + ignore_value(virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearPCIAddress, NULL)); }
void virDomainDefClearCCWAddresses(virDomainDefPtr def) { - virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearCCWAddress, NULL); + ignore_value(virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearCCWAddress, NULL)); }
These two are unused since commit fb06350, they can be just removed.
void virDomainDefClearDeviceAliases(virDomainDefPtr def) { - virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL); + ignore_value(virDomainDeviceInfoIterate(def, virDomainDeviceInfoClearAlias, NULL)); }
The only caller is qemuProcessStop and I think it can also be deleted. For persistent domains, the domain definition (including aliases) gets freed a few screens later when we replace it with newDef. For transient domains, we free the definition along with the virDomainObj a few moments later, but nothing that cares about aliases (neither building qemu command line nor talking to it via the monitor) should be done in the meantime. Jan
Rather than use strtok_i to parse the VethName, use the virStringSplitCount helper. The Coverity checker would "mark" that it's possible that the results of the strtok_r may not be what's expected if openvzReadVPSConfigParam returned a NULL (although logically it shouldn't, but Coverity got lost). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_driver.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/openvz/openvz_driver.c b/src/openvz/openvz_driver.c index f9a89cf..22dc333 100644 --- a/src/openvz/openvz_driver.c +++ b/src/openvz/openvz_driver.c @@ -798,20 +798,25 @@ openvzGenerateContainerVethName(int veid) { char *temp = NULL; char *name = NULL; + char **tmp = NULL; + size_t i; + size_t ntmp = 0; /* try to get line "^NETIF=..." from config */ if (openvzReadVPSConfigParam(veid, "NETIF", &temp) <= 0) { ignore_value(VIR_STRDUP(name, "eth0")); } else { - char *saveptr = NULL; - char *s; int max = 0; + if (!(tmp = virStringSplitCount(temp, ";", 0, &ntmp))) + goto cleanup; + /* get maximum interface number (actually, it is the last one) */ - for (s = strtok_r(temp, ";", &saveptr); s; s = strtok_r(NULL, ";", &saveptr)) { + for (i = 0; i < ntmp; i++) { int x; - if (sscanf(s, "ifname=eth%d", &x) != 1) return NULL; + if (sscanf(tmp[i], "ifname=eth%d", &x) != 1) + goto cleanup; if (x > max) max = x; } @@ -819,8 +824,9 @@ openvzGenerateContainerVethName(int veid) ignore_value(virAsprintf(&name, "eth%d", max + 1)); } + cleanup: VIR_FREE(temp); - + virStringFreeListCount(tmp, ntmp); return name; } -- 2.5.5
Rather than rely on strtok_r calls, use virStringSplitCount in order to process the conf variable. Coverity complains because it doesn't know that the returned temp variable in openvzReadVPSConfigParam should be non-NULL. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/openvz/openvz_conf.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index 99ce95c..6562fcc 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -200,7 +200,9 @@ openvzReadNetworkConf(virDomainDefPtr def, int ret; virDomainNetDefPtr net = NULL; char *temp = NULL; - char *token, *saveptr = NULL; + char **tmp = NULL; + size_t ntmp = 0; + size_t i; /*parse routing network configuration* * Sample from config: @@ -214,20 +216,23 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, " ", &saveptr); - while (token != NULL) { + if (!(tmp = virStringSplitCount(temp, " ", 0, &ntmp))) + goto error; + + for (i = 0; i < ntmp; i++) { if (VIR_ALLOC(net) < 0) goto error; net->type = VIR_DOMAIN_NET_TYPE_ETHERNET; - if (virDomainNetAppendIPAddress(net, token, AF_UNSPEC, 0) < 0) + if (virDomainNetAppendIPAddress(net, tmp[i], AF_UNSPEC, 0) < 0) goto error; if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) goto error; - - token = strtok_r(NULL, " ", &saveptr); } + virStringFreeListCount(tmp, ntmp); + ntmp = 0; + tmp = NULL; } /*parse bridge devices*/ @@ -242,15 +247,17 @@ openvzReadNetworkConf(virDomainDefPtr def, veid); goto error; } else if (ret > 0) { - token = strtok_r(temp, ";", &saveptr); - while (token != NULL) { - /*add new device to list*/ + if (!(tmp = virStringSplitCount(temp, " ", 0, &ntmp))) + goto error; + + for (i = 0; i < ntmp; i++) { + /* add new device to list */ if (VIR_ALLOC(net) < 0) goto error; net->type = VIR_DOMAIN_NET_TYPE_BRIDGE; - char *p = token; + char *p = tmp[i]; char cpy_temp[32]; int len; @@ -313,21 +320,21 @@ openvzReadNetworkConf(virDomainDefPtr def, } } p = ++next; - } while (p < token + strlen(token)); + } while (p < tmp[i] + strlen(tmp[i])); if (VIR_APPEND_ELEMENT_COPY(def->nets, def->nnets, net) < 0) goto error; - - token = strtok_r(NULL, ";", &saveptr); } } VIR_FREE(temp); + virStringFreeListCount(tmp, ntmp); return 0; error: VIR_FREE(temp); + virStringFreeListCount(tmp, ntmp); virDomainNetDefFree(net); return -1; } -- 2.5.5
participants (2)
-
John Ferlan -
Ján Tomko