[libvirt] [PATCH 0/3] Resolve some Coverity issues
Resolve some issues seen by my daily Coverity run. Interesting that they didn't show up on the internal Jenkins Coverity scanner, but did show up on my daily run. Perhaps because I have 7.0.3 and the Jenkins has 7.0.0... I also see 7.5.0 is available - I'll give that a go too... John Ferlan (3): commandtest: Resolve Coverity RESOURCE_LEAK virnetsocket: Resolve Coverity RESOURCE_LEAK xenconfig: Resolve Coverity RESOURCE_LEAK src/rpc/virnetsocket.c | 8 ++++++-- src/xenconfig/xen_common.c | 2 ++ tests/commandtest.c | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) -- 1.9.3
Since '62f263a73' - Coverity complains if the !pidfile path is taken, then newfd1 would be leaked. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/commandtest.c b/tests/commandtest.c index b3287fa..5f52a00 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1081,6 +1081,7 @@ static int test24(const void *unused ATTRIBUTE_UNUSED) unlink(pidfile); VIR_FREE(pidfile); virCommandFree(cmd); + VIR_FORCE_CLOSE(newfd1); /* coverity[double_close] */ VIR_FORCE_CLOSE(newfd2); VIR_FORCE_CLOSE(newfd3); -- 1.9.3
On 22.08.2014 17:28, John Ferlan wrote:
Since '62f263a73' - Coverity complains if the !pidfile path is taken, then newfd1 would be leaked.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/tests/commandtest.c b/tests/commandtest.c index b3287fa..5f52a00 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -1081,6 +1081,7 @@ static int test24(const void *unused ATTRIBUTE_UNUSED) unlink(pidfile); VIR_FREE(pidfile); virCommandFree(cmd); + VIR_FORCE_CLOSE(newfd1); /* coverity[double_close] */ VIR_FORCE_CLOSE(newfd2); VIR_FORCE_CLOSE(newfd3);
ACK Michal
On 08/22/2014 05:28 PM, John Ferlan wrote:
Since '62f263a73' - Coverity complains if the !pidfile path is taken, then newfd1 would be leaked.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/commandtest.c | 1 + 1 file changed, 1 insertion(+)
ACK Jan
Since '1b807f92d' - Coverity complains that in the error paths of both virFork() and virProcessWait() that the 'passfd' will not be closed Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetsocket.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index f913365..ce908fa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path, * behaviour on sockets according to POSIX, so it doesn't * work outside Linux. */ - if ((pid = virFork()) < 0) + if ((pid = virFork()) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + } if (pid == 0) { umask(0077); @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path, _exit(EXIT_SUCCESS); } - if (virProcessWait(pid, &status, false) < 0) + if (virProcessWait(pid, &status, false) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + } if (status != EXIT_SUCCESS) { /* -- 1.9.3
On 22.08.2014 17:28, John Ferlan wrote:
Since '1b807f92d' - Coverity complains that in the error paths of both virFork() and virProcessWait() that the 'passfd' will not be closed
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetsocket.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index f913365..ce908fa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path, * behaviour on sockets according to POSIX, so it doesn't * work outside Linux. */ - if ((pid = virFork()) < 0) + if ((pid = virFork()) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (pid == 0) { umask(0077); @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path, _exit(EXIT_SUCCESS); }
- if (virProcessWait(pid, &status, false) < 0) + if (virProcessWait(pid, &status, false) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (status != EXIT_SUCCESS) { /*
Unfortunately not enough context is shown to express myself, so I'll paste interesting knobs from the function: if (listen(passfd, 0) < 0) { virReportSystemError(errno, "%s", _("Failed to listen on socket that's about " "to be passed to the daemon")); goto error; } if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; } if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; } localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { virReportSystemError(errno, "%s", _("Unable to get local socket name")); goto error; } if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) goto error; return 0; error: VIR_FREE(buf); VIR_FORCE_CLOSE(fd); if (spawnDaemon) unlink(path); return -1; } Here, if any of listen() or connect() fail, the control continues on the error label and the @passfd is leaked again. virNetSocketForkDaemon() is different - it closes passfd on failure. So I suggest moving VIR_FORCE_CLOSE() under the error label. Michal
On 08/22/2014 11:49 AM, Michal Privoznik wrote:
On 22.08.2014 17:28, John Ferlan wrote:
Since '1b807f92d' - Coverity complains that in the error paths of both virFork() and virProcessWait() that the 'passfd' will not be closed
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetsocket.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index f913365..ce908fa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path, * behaviour on sockets according to POSIX, so it doesn't * work outside Linux. */ - if ((pid = virFork()) < 0) + if ((pid = virFork()) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (pid == 0) { umask(0077); @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path, _exit(EXIT_SUCCESS); }
- if (virProcessWait(pid, &status, false) < 0) + if (virProcessWait(pid, &status, false) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (status != EXIT_SUCCESS) { /*
Unfortunately not enough context is shown to express myself, so I'll paste interesting knobs from the function:
if (listen(passfd, 0) < 0) { virReportSystemError(errno, "%s", _("Failed to listen on socket that's about " "to be passed to the daemon")); goto error; }
if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; }
if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; }
localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { virReportSystemError(errno, "%s", _("Unable to get local socket name")); goto error; }
if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) goto error;
return 0;
error: VIR_FREE(buf); VIR_FORCE_CLOSE(fd); if (spawnDaemon) unlink(path); return -1; }
Here, if any of listen() or connect() fail, the control continues on the error label and the @passfd is leaked again. virNetSocketForkDaemon() is different - it closes passfd on failure. So I suggest moving VIR_FORCE_CLOSE() under the error label.
Yeah - Coverity only complained about the two paths - so I was hesitant to put the VIR_FORCE_CLOSE(passfd) in the error: path... In any case, putting it the error: path does resolve the issue as well as making sure to initialize passfd = -1 (initializing fd = -1 wasn't necessary since there's currently no way to error unless it's set) John
On 08/22/2014 12:42 PM, John Ferlan wrote:
On 08/22/2014 11:49 AM, Michal Privoznik wrote:
On 22.08.2014 17:28, John Ferlan wrote:
Since '1b807f92d' - Coverity complains that in the error paths of both virFork() and virProcessWait() that the 'passfd' will not be closed
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetsocket.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index f913365..ce908fa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path, * behaviour on sockets according to POSIX, so it doesn't * work outside Linux. */ - if ((pid = virFork()) < 0) + if ((pid = virFork()) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (pid == 0) { umask(0077); @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path, _exit(EXIT_SUCCESS); }
- if (virProcessWait(pid, &status, false) < 0) + if (virProcessWait(pid, &status, false) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (status != EXIT_SUCCESS) { /*
Unfortunately not enough context is shown to express myself, so I'll paste interesting knobs from the function:
if (listen(passfd, 0) < 0) { virReportSystemError(errno, "%s", _("Failed to listen on socket that's about " "to be passed to the daemon")); goto error; }
if (connect(fd, &remoteAddr.data.sa, remoteAddr.len) < 0) { virReportSystemError(errno, _("Failed to connect socket to '%s'"), path); goto error; }
if (virNetSocketForkDaemon(binary, passfd) < 0) goto error; }
localAddr.len = sizeof(localAddr.data); if (getsockname(fd, &localAddr.data.sa, &localAddr.len) < 0) { virReportSystemError(errno, "%s", _("Unable to get local socket name")); goto error; }
if (!(*retsock = virNetSocketNew(&localAddr, &remoteAddr, true, fd, -1, 0))) goto error;
return 0;
error: VIR_FREE(buf);
While implementing the change to add passfd here - I noticed that buf is never used in this context? Strange. There's an initialization to NULL and a VIR_FREE(buf);, but nothing else uses it, so I removed it. John
VIR_FORCE_CLOSE(fd); if (spawnDaemon) unlink(path); return -1; }
Here, if any of listen() or connect() fail, the control continues on the error label and the @passfd is leaked again. virNetSocketForkDaemon() is different - it closes passfd on failure. So I suggest moving VIR_FORCE_CLOSE() under the error label.
Yeah - Coverity only complained about the two paths - so I was hesitant to put the VIR_FORCE_CLOSE(passfd) in the error: path...
In any case, putting it the error: path does resolve the issue as well as making sure to initialize passfd = -1 (initializing fd = -1 wasn't necessary since there's currently no way to error unless it's set)
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On 08/22/2014 05:28 PM, John Ferlan wrote:
Since '1b807f92d' - Coverity complains that in the error paths of both virFork() and virProcessWait() that the 'passfd' will not be closed
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/rpc/virnetsocket.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index f913365..ce908fa 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -593,8 +593,10 @@ int virNetSocketNewConnectUNIX(const char *path, * behaviour on sockets according to POSIX, so it doesn't * work outside Linux. */ - if ((pid = virFork()) < 0) + if ((pid = virFork()) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (pid == 0) { umask(0077); @@ -604,8 +606,10 @@ int virNetSocketNewConnectUNIX(const char *path, _exit(EXIT_SUCCESS); }
- if (virProcessWait(pid, &status, false) < 0) + if (virProcessWait(pid, &status, false) < 0) { + VIR_FORCE_CLOSE(passfd); goto error; + }
if (status != EXIT_SUCCESS) { /*
Unless I'm missing something, passfd will be leaked on all error paths unless virNetSocketForkDaemon succeeds. Jan
Since '337a13628' - Coverity complains that 'net' is VIR_ALLOC()'d, but on various 'cleanup' exit paths from the code there is no corresponding cleanup. Since 'net' is eventually appended onto a list of nets we don't want to delete the last one - be sure to set it to NULL, but still call the free function in cleanup Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenconfig/xen_common.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 398e9ec..c487feb 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -949,6 +949,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) goto cleanup; + net = NULL; skipnic: list = list->next; @@ -960,6 +961,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) return 0; cleanup: + virDomainNetDefFree(net); VIR_FREE(script); return -1; } -- 1.9.3
On 22.08.2014 17:28, John Ferlan wrote:
Since '337a13628' - Coverity complains that 'net' is VIR_ALLOC()'d, but on various 'cleanup' exit paths from the code there is no corresponding cleanup. Since 'net' is eventually appended onto a list of nets we don't want to delete the last one - be sure to set it to NULL, but still call the free function in cleanup
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenconfig/xen_common.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 398e9ec..c487feb 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -949,6 +949,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) goto cleanup; + net = NULL;
This is not needed. VIR_APPEND_ELEMENT() should clear out the @net variable. Or does coverity fail to see it?
skipnic: list = list->next; @@ -960,6 +961,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) return 0;
cleanup: + virDomainNetDefFree(net); VIR_FREE(script); return -1; }
In fact this chunk alone should be enough. Michal
On 08/22/2014 05:28 PM, John Ferlan wrote:
Since '337a13628' - Coverity complains that 'net' is VIR_ALLOC()'d, but on various 'cleanup' exit paths from the code there is no corresponding cleanup. Since 'net' is eventually appended onto a list of nets we don't want to delete the last one - be sure to set it to NULL, but still call the free function in cleanup
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenconfig/xen_common.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 398e9ec..c487feb 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -949,6 +949,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) goto cleanup; + net = NULL;
This bit should not be necessary. VIR_APPEND_ELEMENT expands to virInsertElementsN with clearOriginal=true
skipnic: list = list->next; @@ -960,6 +961,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) return 0;
cleanup: + virDomainNetDefFree(net); VIR_FREE(script); return -1; }
ACK Jan
On 08/22/2014 11:28 AM, John Ferlan wrote:
Since '337a13628' - Coverity complains that 'net' is VIR_ALLOC()'d, but on various 'cleanup' exit paths from the code there is no corresponding cleanup. Since 'net' is eventually appended onto a list of nets we don't want to delete the last one - be sure to set it to NULL, but still call the free function in cleanup
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/xenconfig/xen_common.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/src/xenconfig/xen_common.c b/src/xenconfig/xen_common.c index 398e9ec..c487feb 100644 --- a/src/xenconfig/xen_common.c +++ b/src/xenconfig/xen_common.c @@ -949,6 +949,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def)
if (VIR_APPEND_ELEMENT(def->nets, def->nnets, net) < 0) goto cleanup; + net = NULL;
OK - so removing this worked fine - my brain certainly thought it was necessary based on previous times I've made coverity changes... John
skipnic: list = list->next; @@ -960,6 +961,7 @@ xenParseVif(virConfPtr conf, virDomainDefPtr def) return 0;
cleanup: + virDomainNetDefFree(net); VIR_FREE(script); return -1; }
On 08/22/2014 11:28 AM, John Ferlan wrote:
Resolve some issues seen by my daily Coverity run.
Interesting that they didn't show up on the internal Jenkins Coverity scanner, but did show up on my daily run. Perhaps because I have 7.0.3 and the Jenkins has 7.0.0... I also see 7.5.0 is available - I'll give that a go too...
John Ferlan (3): commandtest: Resolve Coverity RESOURCE_LEAK virnetsocket: Resolve Coverity RESOURCE_LEAK xenconfig: Resolve Coverity RESOURCE_LEAK
src/rpc/virnetsocket.c | 8 ++++++-- src/xenconfig/xen_common.c | 2 ++ tests/commandtest.c | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)
I've resolved the points from review and pushed. Thanks for the quick reviews! John FWIW: I should never have upgraded my coverity to 7.5.0 - it's *way too* chatty! <sigh> Although it seems to have found some more "interesting" latent things, but also it seems a new set of false positives. <double sigh>... Only 164 things to look at!
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik