Since '62f263a73' - Coverity complains if the !pidfile path is taken, then newfd1 would be leaked. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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);

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(a)redhat.com&gt; --- 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; }

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(a)redhat.com&gt; > --- > 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.

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(a)redhat.com&gt; >> --- >> 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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list