10:42 a.m.
This is the last round of cleanup with a few Coverity errors from the
tests trees and a couple that just showed up in my recent scans within
security and virsh. The commandtest still has Valgrind errors, but errors
are not new to this change.
After this round of Coverity changes, the only remaining Coverity warnings
reported are within the gnulib tree. A few have been there all along within
vasnprintf.c, while others are new with the recent gnulib update (regexec.c,
regex_internal.c, and regcomp.c).
Once these are submitted I will have a baseline from which I can make
periodic comparisons. Since Coverity will sometimes "find" issues unrelated
to recent changes, using git bisect to point the fickle finger of fate at
a recent submission may not be possible.
John Ferlan (5):
drivermodule: Ignore coverity warning about leaked_storage
commandtest: Resolve some coverity resource leaks
vircommand: Remove unnecessary sa_assert
virsh: Resolve possible NULL dereference
security: Remove unnecessary checks for mgr == NULL
src/security/security_dac.c | 6 ------
src/security/security_selinux.c | 6 ------
src/util/vircommand.c | 3 ---
tests/commandtest.c | 17 ++++++++++++-----
tests/virdrivermoduletest.c | 2 ++
tools/virsh.c | 4 ++--
6 files changed, 16 insertions(+), 22 deletions(-)
--
1.7.11.7
10:42 a.m.
New subject: [libvirt] [PATCH 1/5] drivermodule: Ignore coverity warning about leaked_storage
---
tests/virdrivermoduletest.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c
index de8cf70..bbceb96 100644
--- a/tests/virdrivermoduletest.c
+++ b/tests/virdrivermoduletest.c
@@ -39,10 +39,12 @@ static int testDriverModule(const void *args)
{
const struct testDriverData *data = args;
+ /* coverity[leaked_storage] */
if (data->dep1 &&
!virDriverLoadModule(data->dep1))
return -1;
+ /* coverity[leaked_storage] */
if (!virDriverLoadModule(data->name))
return -1;
--
1.7.11.7
11:17 a.m.
New subject: [libvirt] [PATCH 1/5] drivermodule: Ignore coverity warning about leaked_storage
On 02/14/13 17:42, John Ferlan wrote:
---
tests/virdrivermoduletest.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tests/virdrivermoduletest.c b/tests/virdrivermoduletest.c
index de8cf70..bbceb96 100644
--- a/tests/virdrivermoduletest.c
+++ b/tests/virdrivermoduletest.c
@@ -39,10 +39,12 @@ static int testDriverModule(const void *args)
{
const struct testDriverData *data = args;
+ /* coverity[leaked_storage] */
I presume the leaked storage is from the one shot initializers.
if (data->dep1 &&
!virDriverLoadModule(data->dep1))
return -1;
+ /* coverity[leaked_storage] */
if (!virDriverLoadModule(data->name))
return -1;
ACK.
Peter
10:42 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
---
tests/commandtest.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 93c6333..3bfc358 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -680,7 +680,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED)
goto cleanup;
}
- if (!outbuf || *outbuf) {
+ if (*outbuf) {
puts("output buffer is not an allocated empty string");
goto cleanup;
}
@@ -702,7 +702,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED)
goto cleanup;
}
- if (!outbuf || *outbuf || !errbuf || *errbuf) {
+ if (*outbuf || *errbuf) {
puts("output buffers are not allocated empty strings");
goto cleanup;
}
@@ -936,6 +936,7 @@ mymain(void)
int fd;
virCommandTestDataPtr test = NULL;
int timer = -1;
+ int virinitret;
if (virThreadInitialize() < 0)
return EXIT_FAILURE;
@@ -963,16 +964,19 @@ mymain(void)
dup2(fd, 6) < 0 ||
dup2(fd, 7) < 0 ||
dup2(fd, 8) < 0 ||
- (fd > 8 && VIR_CLOSE(fd) < 0))
+ (fd > 8 && VIR_CLOSE(fd) < 0)) {
+ VIR_FORCE_CLOSE(fd);
return EXIT_FAILURE;
+ }
+ sa_assert(fd == -1);
/* Prime the debug/verbose settings from the env vars,
* since we're about to reset 'environ' */
ignore_value(virTestGetDebug());
ignore_value(virTestGetVerbose());
- if (virInitialize() < 0)
- return EXIT_FAILURE;
+ /* Make sure to not leak fd's */
+ virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */
fd = 3;
@@ -988,6 +992,9 @@ mymain(void)
fd = 8;
VIR_FORCE_CLOSE(fd);
+ if (virinitret < 0)
+ return EXIT_FAILURE;
+
virEventRegisterDefaultImpl();
if (VIR_ALLOC(test) < 0) {
virReportOOMError();
--
1.7.11.7
11:15 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/14/13 17:42, John Ferlan wrote:
---
tests/commandtest.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 93c6333..3bfc358 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
...
@@ -963,16 +964,19 @@ mymain(void)
dup2(fd, 6) < 0 ||
dup2(fd, 7) < 0 ||
dup2(fd, 8) < 0 ||
- (fd > 8 && VIR_CLOSE(fd) < 0))
+ (fd > 8 && VIR_CLOSE(fd) < 0)) {
+ VIR_FORCE_CLOSE(fd);
return EXIT_FAILURE;
+ }
+ sa_assert(fd == -1);
according to man of dup2():
* If oldfd is a valid file descriptor, and newfd has the same value
as oldfd, then dup2() does nothing, and returns newfd.
It is possible that open returns fd < 8, dup2() on that does nothing and
afterwards this assertion won't be true.
/* Prime the debug/verbose settings from the env vars,
* since we're about to reset 'environ' */
ignore_value(virTestGetDebug());
ignore_value(virTestGetVerbose());
- if (virInitialize() < 0)
- return EXIT_FAILURE;
+ /* Make sure to not leak fd's */
+ virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */
fd = 3;
ACK with the assertion removed or a sufficient explanation provided.
Peter
11:43 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/14/2013 10:15 AM, Peter Krempa wrote:
On 02/14/13 17:42, John Ferlan wrote:
> ---
> tests/commandtest.c | 17 ++++++++++++-----
> 1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/tests/commandtest.c b/tests/commandtest.c
> index 93c6333..3bfc358 100644
> --- a/tests/commandtest.c
> +++ b/tests/commandtest.c
...
> @@ -963,16 +964,19 @@ mymain(void)
> dup2(fd, 6) < 0 ||
> dup2(fd, 7) < 0 ||
> dup2(fd, 8) < 0 ||
> - (fd > 8 && VIR_CLOSE(fd) < 0))
> + (fd > 8 && VIR_CLOSE(fd) < 0)) {
> + VIR_FORCE_CLOSE(fd);
> return EXIT_FAILURE;
> + }
> + sa_assert(fd == -1);
according to man of dup2():
* If oldfd is a valid file descriptor, and newfd has the same value
as oldfd, then dup2() does nothing, and returns newfd.
It is possible that open returns fd < 8, dup2() on that does nothing and
afterwards this assertion won't be true.
I gave my ACK too soon - Peter is right that fd _might_ not be -1 at
this point. There are two cases to consider:
1. start life with fds 3-8 already opened by inheritance (rare). Opening
/dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was
inherited from the environment), then we close 9. If we fail to dup2,
we still close 9, but none of the other fds which we forcefully
overwrote. In this case, fd==-1 is true when you get to the sa_assert.
2. start life with at least one of fd 3-8 closed (most common). Opening
/dev/null then gets the first open fd, which we then dup to all the
remaining fds between 3-8. We don't close fd in this case, because we
want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false.
Does the assert actually fix anything? I think it is best to remove it.
>
> /* Prime the debug/verbose settings from the env vars,
> * since we're about to reset 'environ' */
> ignore_value(virTestGetDebug());
> ignore_value(virTestGetVerbose());
>
> - if (virInitialize() < 0)
> - return EXIT_FAILURE;
> + /* Make sure to not leak fd's */
> + virinitret = virInitialize();
>
> /* Phase two of killing interfering fds; see above. */
> fd = 3;
ACK with the assertion removed or a sufficient explanation provided.
Peter
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
1 p.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/14/2013 12:43 PM, Eric Blake wrote:
On 02/14/2013 10:15 AM, Peter Krempa wrote:
> On 02/14/13 17:42, John Ferlan wrote:
>> ---
>> tests/commandtest.c | 17 ++++++++++++-----
>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/tests/commandtest.c b/tests/commandtest.c
>> index 93c6333..3bfc358 100644
>> --- a/tests/commandtest.c
>> +++ b/tests/commandtest.c
>
> ...
>
>> @@ -963,16 +964,19 @@ mymain(void)
>> dup2(fd, 6) < 0 ||
>> dup2(fd, 7) < 0 ||
>> dup2(fd, 8) < 0 ||
>> - (fd > 8 && VIR_CLOSE(fd) < 0))
>> + (fd > 8 && VIR_CLOSE(fd) < 0)) {
>> + VIR_FORCE_CLOSE(fd);
>> return EXIT_FAILURE;
>> + }
>> + sa_assert(fd == -1);
>
> according to man of dup2():
> * If oldfd is a valid file descriptor, and newfd has the same value
> as oldfd, then dup2() does nothing, and returns newfd.
>
> It is possible that open returns fd < 8, dup2() on that does nothing and
> afterwards this assertion won't be true.
I gave my ACK too soon - Peter is right that fd _might_ not be -1 at
this point. There are two cases to consider:
1. start life with fds 3-8 already opened by inheritance (rare). Opening
/dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was
inherited from the environment), then we close 9. If we fail to dup2,
we still close 9, but none of the other fds which we forcefully
overwrote. In this case, fd==-1 is true when you get to the sa_assert.
2. start life with at least one of fd 3-8 closed (most common). Opening
/dev/null then gets the first open fd, which we then dup to all the
remaining fds between 3-8. We don't close fd in this case, because we
want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false.
Does the assert actually fix anything? I think it is best to remove it.
Without the sa_assert, the following is coughed up by Coverity:
979
980 /* Phase two of killing interfering fds; see above. */
(20) Event overwrite_var: Overwriting handle "fd" in "fd = 3" leaks
the
handle.
Also see events:
[open_fn][var_assign][noescape][noescape][noescape][noescape][noescape][noescape]
981 fd = 3;
982 VIR_FORCE_CLOSE(fd);
When I first modified this code it was still fd 3->5 being dup2'd and
the sa_assert() worked for at least shutting coverity up, but I see the
point about the last if condition.
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
but that too triggers Coverity to complain that we're overwriting the
fd. So without creating a new variable and adding more busy work, adding
the following prior to the initialization removes the warning.
/* coverity[overwrite_var] - silence the obvious */
fd = 3;
Also, yes, it's strange on the error case of the if condition that one
doesn't have to do the same close 3->8 game even though the
VIR_FORCE_CLOSE(fd) was needed.
>
>>
>> /* Prime the debug/verbose settings from the env vars,
>> * since we're about to reset 'environ' */
>> ignore_value(virTestGetDebug());
>> ignore_value(virTestGetVerbose());
>>
>> - if (virInitialize() < 0)
>> - return EXIT_FAILURE;
>> + /* Make sure to not leak fd's */
>> + virinitret = virInitialize();
>>
>> /* Phase two of killing interfering fds; see above. */
>> fd = 3;
>
> ACK with the assertion removed or a sufficient explanation provided.
>
> Peter
>
> --
> libvir-list mailing list
> libvir-list(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
>
3:01 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/14/13 20:00, John Ferlan wrote:
On 02/14/2013 12:43 PM, Eric Blake wrote:
> On 02/14/2013 10:15 AM, Peter Krempa wrote:
>> On 02/14/13 17:42, John Ferlan wrote:
>>> ---
>>> tests/commandtest.c | 17 ++++++++++++-----
>>> 1 file changed, 12 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/tests/commandtest.c b/tests/commandtest.c
>>> index 93c6333..3bfc358 100644
>>> --- a/tests/commandtest.c
>>> +++ b/tests/commandtest.c
>>
>> ...
>>
>>> @@ -963,16 +964,19 @@ mymain(void)
>>> dup2(fd, 6) < 0 ||
>>> dup2(fd, 7) < 0 ||
>>> dup2(fd, 8) < 0 ||
>>> - (fd > 8 && VIR_CLOSE(fd) < 0))
>>> + (fd > 8 && VIR_CLOSE(fd) < 0)) {
>>> + VIR_FORCE_CLOSE(fd);
>>> return EXIT_FAILURE;
>>> + }
>>> + sa_assert(fd == -1);
>>
>> according to man of dup2():
>> * If oldfd is a valid file descriptor, and newfd has the same value
>> as oldfd, then dup2() does nothing, and returns newfd.
>>
>> It is possible that open returns fd < 8, dup2() on that does nothing and
>> afterwards this assertion won't be true.
>
> I gave my ACK too soon - Peter is right that fd _might_ not be -1 at
> this point. There are two cases to consider:
>
> 1. start life with fds 3-8 already opened by inheritance (rare). Opening
> /dev/null gets fd 9, which we then dup to 3-8 (overwriting whatever was
> inherited from the environment), then we close 9. If we fail to dup2,
> we still close 9, but none of the other fds which we forcefully
> overwrote. In this case, fd==-1 is true when you get to the sa_assert.
>
> 2. start life with at least one of fd 3-8 closed (most common). Opening
> /dev/null then gets the first open fd, which we then dup to all the
> remaining fds between 3-8. We don't close fd in this case, because we
> want fds 3-8 opened on /dev/null; so in this case, fd==-1 is false.
>
> Does the assert actually fix anything? I think it is best to remove it.
>
Without the sa_assert, the following is coughed up by Coverity:
979
980 /* Phase two of killing interfering fds; see above. */
(20) Event overwrite_var: Overwriting handle "fd" in "fd = 3" leaks
the
handle.
From a human point of view this is a false positive:
1) In case fd <= 8, it gets dup2()-ed to itself and we close it later.
2) In case fd > we close it anyway
But the analyzer has hard time following our manual closing of the
handles afterwards.
Also see events:
[open_fn][var_assign][noescape][noescape][noescape][noescape][noescape][noescape]
981 fd = 3;
982 VIR_FORCE_CLOSE(fd);
When I first modified this code it was still fd 3->5 being dup2'd and
the sa_assert() worked for at least shutting coverity up, but I see the
point about the last if condition.
So, one could logically believe the check could change to:
sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Hm, I think I know why this shuts coverity up: the assertion statement
forces coverity to think that the last part of the if statement was true
and thus fd was explicitly closed.
In this case, I think it's okay to use it to shut coverity up as long as
"sa_assert" doesn't have any effect outside of static analysis.
We know that we're doing the right thing with the fds so no need for
coverity to whine about that.
I don't know if this is worth bothering any more and even filing a bug
against coverity. We're doing some pretty ugly stuff here from the POV
of static analysis.
but that too triggers Coverity to complain that we're overwriting the
fd. So without creating a new variable and adding more busy work, adding
the following prior to the initialization removes the warning.
/* coverity[overwrite_var] - silence the obvious */
fd = 3;
Also, yes, it's strange on the error case of the if condition that one
doesn't have to do the same close 3->8 game even though the
VIR_FORCE_CLOSE(fd) was needed.
>>
>>>
>>> /* Prime the debug/verbose settings from the env vars,
>>> * since we're about to reset 'environ' */
>>> ignore_value(virTestGetDebug());
>>> ignore_value(virTestGetVerbose());
>>>
>>> - if (virInitialize() < 0)
>>> - return EXIT_FAILURE;
>>> + /* Make sure to not leak fd's */
>>> + virinitret = virInitialize();
>>>
>>> /* Phase two of killing interfering fds; see above. */
>>> fd = 3;
>>
>> ACK with the assertion removed or a sufficient explanation provided.
I think this is a sufficient explanation, so ACK (as long as sa_assert
has no side effects normally).
>>
>> Peter
>>
7:17 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/15/2013 02:01 AM, Peter Krempa wrote:
> So, one could logically believe the check could change to:
>
> sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
>>> ACK with the assertion removed or a sufficient
explanation provided.
I think this is a sufficient explanation, so ACK (as long as sa_assert
has no side effects normally).
sa_assert is a no-op when static analysis is not being performed.
That's why we prefer it over raw assert(), because sa_assert() does not
change based on NDEBUG, only based on toolchain.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
7:54 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/15/13 14:17, Eric Blake wrote:
On 02/15/2013 02:01 AM, Peter Krempa wrote:
>> So, one could logically believe the check could change to:
>>
>> sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
Yes, I like that.
If I understood it correctly, the above condition won't shut up
coverity, only sa_assert(fd == -1) does as coverity then thinks that fd
was > 8 and thus closed. Otherwise it does not detect the magic we're
doing later.
Peter
9:12 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/15/2013 08:54 AM, Peter Krempa wrote:
On 02/15/13 14:17, Eric Blake wrote:
> On 02/15/2013 02:01 AM, Peter Krempa wrote:
>
>>> So, one could logically believe the check could change to:
>>>
>>> sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
>
> Yes, I like that.
If I understood it correctly, the above condition won't shut up
coverity, only sa_assert(fd == -1) does as coverity then thinks that fd
was > 8 and thus closed. Otherwise it does not detect the magic we're
doing later.
Yes, correct, hence the need for the "/* coverity[overwrite_var] */ tag.
Coverity will try 2 paths and only tell you when it fails. I figured
that out when going back through this exercise based on the review.
Paying close attention to trail of messages it leaves is the key - when
I first made changes to this code - I'm not sure I picked that up.
John
Peter
--
libvir-list mailing list
libvir-list(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:44 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/15/2013 08:12 AM, John Ferlan wrote:
On 02/15/2013 08:54 AM, Peter Krempa wrote:
> On 02/15/13 14:17, Eric Blake wrote:
>> On 02/15/2013 02:01 AM, Peter Krempa wrote:
>>
>>>> So, one could logically believe the check could change to:
>>>>
>>>> sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
>>
>> Yes, I like that.
>
> If I understood it correctly, the above condition won't shut up
> coverity, only sa_assert(fd == -1) does as coverity then thinks that fd
> was > 8 and thus closed. Otherwise it does not detect the magic we're
> doing later.
Yes, correct, hence the need for the "/* coverity[overwrite_var] */ tag.
If you're using the /* coverity[overwrite_var] */ tag, then do we still
need the sa_assert? This is one case where leaving comments to shut up
coverity is fair game, because it is a test program, and because we
already know we are doing some unusual games with fds to get into a
known state.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:42 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/15/2013 10:44 AM, Eric Blake wrote:
On 02/15/2013 08:12 AM, John Ferlan wrote:
> On 02/15/2013 08:54 AM, Peter Krempa wrote:
>> On 02/15/13 14:17, Eric Blake wrote:
>>> On 02/15/2013 02:01 AM, Peter Krempa wrote:
>>>
>>>>> So, one could logically believe the check could change to:
>>>>>
>>>>> sa_assert(fd == -1 || (fd >= 3 && fd <= 8));
>>>
>>> Yes, I like that.
>>
>> If I understood it correctly, the above condition won't shut up
>> coverity, only sa_assert(fd == -1) does as coverity then thinks that fd
>> was > 8 and thus closed. Otherwise it does not detect the magic we're
>> doing later.
>
> Yes, correct, hence the need for the "/* coverity[overwrite_var] */ tag.
If you're using the /* coverity[overwrite_var] */ tag, then do we still
need the sa_assert? This is one case where leaving comments to shut up
coverity is fair game, because it is a test program, and because we
already know we are doing some unusual games with fds to get into a
known state.
The sa_assert() would not be required. I think by setting to just -1,
Coverity chose to not check fd >=3 && <=8. That is perhaps it
"tells"
Coverity that we know our inputs and we're guaranteeing that the
VIR_CLOSE will happen.
I will remove the sa_assert and keep the comment.
John
11:38 a.m.
New subject: [libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks
On 02/14/2013 09:42 AM, John Ferlan wrote:
---
tests/commandtest.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/tests/commandtest.c b/tests/commandtest.c
index 93c6333..3bfc358 100644
--- a/tests/commandtest.c
+++ b/tests/commandtest.c
@@ -680,7 +680,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED)
goto cleanup;
}
- if (!outbuf || *outbuf) {
+ if (*outbuf) {
puts("output buffer is not an allocated empty string");
goto cleanup;
}
@@ -702,7 +702,7 @@ static int test17(const void *unused ATTRIBUTE_UNUSED)
goto cleanup;
}
- if (!outbuf || *outbuf || !errbuf || *errbuf) {
+ if (*outbuf || *errbuf) {
puts("output buffers are not allocated empty strings");
goto cleanup;
This part seems reasonable.
}
@@ -936,6 +936,7 @@ mymain(void)
int fd;
virCommandTestDataPtr test = NULL;
int timer = -1;
+ int virinitret;
if (virThreadInitialize() < 0)
return EXIT_FAILURE;
@@ -963,16 +964,19 @@ mymain(void)
dup2(fd, 6) < 0 ||
dup2(fd, 7) < 0 ||
dup2(fd, 8) < 0 ||
- (fd > 8 && VIR_CLOSE(fd) < 0))
+ (fd > 8 && VIR_CLOSE(fd) < 0)) {
+ VIR_FORCE_CLOSE(fd);
return EXIT_FAILURE;
Odd that Coverity complains about the leak of fd, but not about the leak
of all the other dup2'd fds; and at any rate, the leak is
inconsequential since we are about to exit. But if this shuts things
up, I'm okay with it.
+ }
+ sa_assert(fd == -1);
Yes, this is true, and if it helps Coverity, I'm fine with it.
/* Prime the debug/verbose settings from the env vars,
* since we're about to reset 'environ' */
ignore_value(virTestGetDebug());
ignore_value(virTestGetVerbose());
- if (virInitialize() < 0)
- return EXIT_FAILURE;
+ /* Make sure to not leak fd's */
+ virinitret = virInitialize();
/* Phase two of killing interfering fds; see above. */
fd = 3;
@@ -988,6 +992,9 @@ mymain(void)
fd = 8;
VIR_FORCE_CLOSE(fd);
+ if (virinitret < 0)
+ return EXIT_FAILURE;
Seems okay to me.
ACK.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
10:42 a.m.
New subject: [libvirt] [PATCH 3/5] vircommand: Remove unnecessary sa_assert
Changes from commit '3178df9a' removed the need for the sa_assert(infd).
---
src/util/vircommand.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/util/vircommand.c b/src/util/vircommand.c
index 5837fee..aa3bce0 100644
--- a/src/util/vircommand.c
+++ b/src/util/vircommand.c
@@ -1853,9 +1853,6 @@ virCommandProcessIO(virCommandPtr cmd)
fds[i].fd == cmd->inpipe) {
int done;
- /* Coverity 5.3.0 can't see that we only get here if
- * infd is in the set because it was non-negative. */
- sa_assert(infd != -1);
done = write(cmd->inpipe, cmd->inbuf + inoff,
inlen - inoff);
if (done < 0) {
--
1.7.11.7
10:53 a.m.
New subject: [libvirt] [PATCH 3/5] vircommand: Remove unnecessary sa_assert
On 02/14/13 17:42, John Ferlan wrote:
Changes from commit '3178df9a' removed the need for the
sa_assert(infd).
---
src/util/vircommand.c | 3 ---
1 file changed, 3 deletions(-)
ACK. I'm not able to verify if it's not needed anymore but it doesn't
break the code.
Peter
10:42 a.m.
New subject: [libvirt] [PATCH 4/5] virsh: Resolve possible NULL dereference
Coverity found that commit '41b5e845' had a possible NULL dereference on
arg->data processing
---
tools/virsh.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 4f75e8e..f5a01b3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1,7 +1,7 @@
/*
* virsh.c: a shell to exercise the libvirt API
*
- * Copyright (C) 2005, 2007-2012 Red Hat, Inc.
+ * Copyright (C) 2005, 2007-2013 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -1475,7 +1475,7 @@ vshCommandOptStringReq(vshControl *ctl,
if (!arg->data)
error = N_("Programming error: Requested option is a boolean");
- if (!*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK))
+ if (arg->data && !*arg->data && !(arg->def->flags &
VSH_OFLAG_EMPTY_OK))
error = N_("Option argument is empty");
if (error) {
--
1.7.11.7
10:52 a.m.
New subject: [libvirt] [PATCH 4/5] virsh: Resolve possible NULL dereference
On 02/14/13 17:42, John Ferlan wrote:
Coverity found that commit '41b5e845' had a possible NULL
dereference on
arg->data processing
---
tools/virsh.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index 4f75e8e..f5a01b3 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -1,7 +1,7 @@
/*
* virsh.c: a shell to exercise the libvirt API
*
- * Copyright (C) 2005, 2007-2012 Red Hat, Inc.
+ * Copyright (C) 2005, 2007-2013 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -1475,7 +1475,7 @@ vshCommandOptStringReq(vshControl *ctl,
if (!arg->data)
error = N_("Programming error: Requested option is a boolean");
- if (!*arg->data && !(arg->def->flags & VSH_OFLAG_EMPTY_OK))
+ if (arg->data && !*arg->data && !(arg->def->flags &
VSH_OFLAG_EMPTY_OK))
Ah, yeah, that assignment doesn't skip this condition. I missed that
while writing it.
error = N_("Option argument is empty");
if (error) {
ACK.
Peter
10:42 a.m.
New subject: [libvirt] [PATCH 5/5] security: Remove unnecessary checks for mgr == NULL
Coverity found the DACGenLabel was checking for mgr == NULL after a
possible dereference; however, in order to get into the function the
virSecurityManagerGenLabel would have already dereferenced sec_managers[i]
so the check was unnecessary. Same check is made in SELinuxGenSecurityLabel.
---
src/security/security_dac.c | 6 ------
src/security/security_selinux.c | 6 ------
2 files changed, 12 deletions(-)
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index b115bb0..0b274b7 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -913,12 +913,6 @@ virSecurityDACGenLabel(virSecurityManagerPtr mgr,
virSecurityLabelDefPtr seclabel;
virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr);
- if (mgr == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
- _("invalid security driver"));
- return rc;
- }
-
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_DAC_NAME);
if (seclabel == NULL) {
return rc;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index a61e0f0..a042b26 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -560,12 +560,6 @@ virSecuritySELinuxGenSecurityLabel(virSecurityManagerPtr mgr,
virSecuritySELinuxDataPtr data;
const char *baselabel;
- if (mgr == NULL) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- "%s", _("invalid security driver"));
- return rc;
- }
-
seclabel = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME);
if (seclabel == NULL) {
return rc;
--
1.7.11.7
11 a.m.
New subject: [libvirt] [PATCH 5/5] security: Remove unnecessary checks for mgr == NULL
On 02/14/13 17:42, John Ferlan wrote:
Coverity found the DACGenLabel was checking for mgr == NULL after a
possible dereference; however, in order to get into the function the
virSecurityManagerGenLabel would have already dereferenced sec_managers[i]
so the check was unnecessary. Same check is made in SELinuxGenSecurityLabel.
---
src/security/security_dac.c | 6 ------
src/security/security_selinux.c | 6 ------
2 files changed, 12 deletions(-)
Yep, probably a relic since layered security drivers were added.
ACK.
Peter
8:34 a.m.
On 02/14/2013 11:42 AM, John Ferlan wrote:
src/security/security_dac.c | 6 ------
src/security/security_selinux.c | 6 ------
src/util/vircommand.c | 3 ---
tests/commandtest.c | 17 ++++++++++++-----
tests/virdrivermoduletest.c | 2 ++
tools/virsh.c | 4 ++--
6 files changed, 16 insertions(+), 22 deletions(-)
Thanks for the reviews, changes were pushed.
John
4295
days inactive
4299
days old
20 comments
3 participants
participants (3)
-
Eric Blake -
John Ferlan -
Peter Krempa