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

[Peter Krempa]

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
>>>