Re: [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
>
>