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