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