10:28 a.m.
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
10:28 a.m.
New subject: [libvirt] [PATCH 1/3] commandtest: Resolve Coverity RESOURCE_LEAK
Since '62f263a73' - Coverity complains if the !pidfile path is taken,
then newfd1 would be leaked.
Signed-off-by: John Ferlan <jferlan(a)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
10:49 a.m.
New subject: [libvirt] [PATCH 1/3] commandtest: Resolve Coverity RESOURCE_LEAK
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(a)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
10:51 a.m.
New subject: [libvirt] [PATCH 1/3] commandtest: Resolve Coverity RESOURCE_LEAK
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(a)redhat.com>
---
tests/commandtest.c | 1 +
1 file changed, 1 insertion(+)
ACK
Jan
10:28 a.m.
New subject: [libvirt] [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK
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>
---
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
10:49 a.m.
New subject: [libvirt] [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK
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>
---
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
11:42 a.m.
New subject: [libvirt] [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK
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>
> ---
> 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
11:48 a.m.
New subject: [libvirt] [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:51 a.m.
New subject: [libvirt] [PATCH 2/3] virnetsocket: Resolve Coverity RESOURCE_LEAK
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(a)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
10:28 a.m.
New subject: [libvirt] [PATCH 3/3] xenconfig: Resolve Coverity RESOURCE_LEAK
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>
---
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
10:49 a.m.
New subject: [libvirt] [PATCH 3/3] xenconfig: Resolve Coverity RESOURCE_LEAK
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(a)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
10:51 a.m.
New subject: [libvirt] [PATCH 3/3] xenconfig: Resolve Coverity RESOURCE_LEAK
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(a)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
11:43 a.m.
New subject: [libvirt] [PATCH 3/3] xenconfig: Resolve Coverity RESOURCE_LEAK
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(a)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;
}
12:07 p.m.
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!
3748
days inactive
3748
days old
13 comments
3 participants
participants (3)
-
John Ferlan -
Ján Tomko -
Michal Privoznik