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