[libvirt] [PATCH 2/5] commandtest: Resolve some coverity resource leaks

[John Ferlan]

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 at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
>>
>>
>