[libvirt] [PATCH] Resolve Coverity complaints in tests/securityselinuxlabeltest.c
Two complaints of RESOURCE_FREE due to going to cleanup prior to a VIR_FREE(line). Two complaints of FORWARD_NULL due to 'tmp' being accessed after a strchr() without first checking if the return was NULL. While looking at the code it seems that 'line' need only be allocated once as the while loop will keep reading into line until eof causing an unreported leak since line was never VIR_FREE()'d at the bottom of the loop. --- tests/securityselinuxlabeltest.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 5ae4f57..d2ba49b 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -79,6 +79,7 @@ testSELinuxLoadFileList(const char *testname, int ret = -1; char *path = NULL; FILE *fp = NULL; + char *line = NULL; *files = NULL; *nfiles = 0; @@ -93,37 +94,43 @@ testSELinuxLoadFileList(const char *testname, goto cleanup; } + if (VIR_ALLOC_N(line, 1024) < 0) { + virReportOOMError(); + goto cleanup; + } + while (!feof(fp)) { - char *line; - char *file, *context; - if (VIR_ALLOC_N(line, 1024) < 0) { - virReportOOMError(); - goto cleanup; - } + char *file, *context, *tmp; if (!fgets(line, 1024, fp)) { if (!feof(fp)) goto cleanup; break; } - char *tmp = strchr(line, ';'); + tmp = strchr(line, ';'); + if (!tmp) { + virReportError(VIR_ERR_INTERNAL_ERROR, + "unexpected format for line '%s'", + line); + goto cleanup; + } *tmp = '\0'; tmp++; - if (virAsprintf(&file, "%s/securityselinuxlabeldata%s", abs_builddir, line) < 0) { - VIR_FREE(line); + if (virAsprintf(&file, "%s/securityselinuxlabeldata%s", + abs_builddir, line) < 0) { virReportOOMError(); goto cleanup; } if (*tmp != '\0' && *tmp != '\n') { if (VIR_STRDUP(context, tmp) < 0) { - VIR_FREE(line); VIR_FREE(file); goto cleanup; } tmp = strchr(context, '\n'); - *tmp = '\0'; + if (tmp) + *tmp = '\0'; } else { context = NULL; } @@ -142,6 +149,7 @@ testSELinuxLoadFileList(const char *testname, cleanup: VIR_FORCE_FCLOSE(fp); VIR_FREE(path); + VIR_FREE(line); return ret; } -- 1.8.1.4
On Wed, Jul 03, 2013 at 10:05:58AM -0400, John Ferlan wrote:
Two complaints of RESOURCE_FREE due to going to cleanup prior to a VIR_FREE(line). Two complaints of FORWARD_NULL due to 'tmp' being accessed after a strchr() without first checking if the return was NULL.
While looking at the code it seems that 'line' need only be allocated once as the while loop will keep reading into line until eof causing an unreported leak since line was never VIR_FREE()'d at the bottom of the loop. --- tests/securityselinuxlabeltest.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 07/03/2013 11:17 AM, Daniel P. Berrange wrote:
On Wed, Jul 03, 2013 at 10:05:58AM -0400, John Ferlan wrote:
Two complaints of RESOURCE_FREE due to going to cleanup prior to a VIR_FREE(line). Two complaints of FORWARD_NULL due to 'tmp' being accessed after a strchr() without first checking if the return was NULL.
While looking at the code it seems that 'line' need only be allocated once as the while loop will keep reading into line until eof causing an unreported leak since line was never VIR_FREE()'d at the bottom of the loop. --- tests/securityselinuxlabeltest.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)
ACK
Daniel
Pushed - thanks. John
participants (2)
-
Daniel P. Berrange -
John Ferlan