[libvirt] [PATCH 0/2] Fix a couple of Coverity found issues from recent patches
The virsysinfo changes caused Coverity to "take another look" at the code and it found some issues. Each is described in the patch commit. John Ferlan (2): util: Resource some resource leaks in virsysinfo code util: Avoid possible NULL dereference in virSysinfoParsePPCProcessor src/util/virsysinfo.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) -- 2.9.3
Calls to virFileReadAll after a VIR_ALLOC that return NULL all show a memory leak since 'ret' isn't virSysinfoDefFree'd and normal path "return ret" doesn't free outbuf. Reported by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virsysinfo.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 7b54bda..14c17a8 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -270,7 +270,7 @@ virSysinfoReadPPC(void) if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); - return NULL; + goto no_memory; } ret->nprocessor = 0; @@ -281,10 +281,12 @@ virSysinfoReadPPC(void) if (virSysinfoParsePPCSystem(outbuf, &ret->system) < 0) goto no_memory; + VIR_FREE(outbuf); return ret; no_memory: VIR_FREE(outbuf); + virSysinfoDefFree(ret); return NULL; } @@ -402,7 +404,7 @@ virSysinfoReadARM(void) if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); - return NULL; + goto no_memory; } ret->nprocessor = 0; @@ -413,10 +415,12 @@ virSysinfoReadARM(void) if (virSysinfoParseARMSystem(outbuf, &ret->system) < 0) goto no_memory; + VIR_FREE(outbuf); return ret; no_memory: VIR_FREE(outbuf); + virSysinfoDefFree(ret); return NULL; } @@ -539,7 +543,7 @@ virSysinfoReadS390(void) if (virFileReadAll(CPUINFO, CPUINFO_FILE_LEN, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), CPUINFO); - return NULL; + goto no_memory; } ret->nprocessor = 0; @@ -554,12 +558,13 @@ virSysinfoReadS390(void) if (virFileReadAll(SYSINFO, 8192, &outbuf) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to open %s"), SYSINFO); - return NULL; + goto no_memory; } if (virSysinfoParseS390System(outbuf, &ret->system) < 0) goto no_memory; + VIR_FREE(outbuf); return ret; no_memory: -- 2.9.3
Found by Coverity. Because there's an "if ((cur = strstr(base, "revision")) != NULL) {" followed by a "base = cur" coverity notes that 'base' could then be NULL causing the return to the top of the "while ((tmp_base = strstr(base, "processor")) != NULL) {" to have strstr deref a NULL 'base' pointer because the setting of base at the bottom of the loop is unconditional. Alter the code to set "base = cur" after processing each key. That will "ensure" that base doesn't get set to NULL if both "cpu" and "revision" do no follow a "processor". While a /proc/cpuinfo file that has a "processor" key but with neither a "cpu" nor a "revision" doesn't seem feasible, the code is written as if it could happen, so we have to account for it. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virsysinfo.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/util/virsysinfo.c b/src/util/virsysinfo.c index 14c17a8..8d3377c 100644 --- a/src/util/virsysinfo.c +++ b/src/util/virsysinfo.c @@ -231,6 +231,7 @@ virSysinfoParsePPCProcessor(const char *base, virSysinfoDefPtr ret) if (eol && VIR_STRNDUP(processor->processor_socket_destination, cur, eol - cur) < 0) return -1; + base = cur; if ((cur = strstr(base, "cpu")) != NULL) { cur = strchr(cur, ':') + 1; @@ -239,6 +240,7 @@ virSysinfoParsePPCProcessor(const char *base, virSysinfoDefPtr ret) if (eol && VIR_STRNDUP(processor->processor_type, cur, eol - cur) < 0) return -1; + base = cur; } if ((cur = strstr(base, "revision")) != NULL) { @@ -248,9 +250,9 @@ virSysinfoParsePPCProcessor(const char *base, virSysinfoDefPtr ret) if (eol && VIR_STRNDUP(processor->processor_version, cur, eol - cur) < 0) return -1; + base = cur; } - base = cur; } return 0; -- 2.9.3
participants (2)
-
John Ferlan -
Peter Krempa