Created attachment 276598 [details] [review]
gtestutils: Fix a very unlikely FD leak in test fork handling

Somehow if g_open("/dev/null") returns an FD < 3, it would previously
have been leaked.

Coverity issue: #1159482

Review of attachment 276598 [details] [review]:

Looks right.

Comment on attachment 276598 [details] [review]
gtestutils: Fix a very unlikely FD leak in test fork handling

>       if (!(test_trap_flags & G_TEST_TRAP_INHERIT_STDIN))
>         fd0 = g_open ("/dev/null", O_RDONLY, 0);
>-      if (sane_dup2 (stdout_pipe[1], 1) < 0 || sane_dup2 (stderr_pipe[1], 2) < 0 || (fd0 >= 0 && sane_dup2 (fd0, 0) < 0))
>+      if (sane_dup2 (stdout_pipe[1], 1) < 0 || sane_dup2 (stderr_pipe[1], 2) < 0 || (fd0 > -1 && sane_dup2 (fd0, 0) < 0))

It probably should g_error() out if g_open() returns -1

>         g_error ("failed to dup2() in forked test program: %s", g_strerror (errno));
>-      if (fd0 >= 3)
>+      if (fd0 > -1)
>         close (fd0);

This is wrong; if fd0 had been 0, 1, or, 2, then it already would have been closed at this point by one of the preceding dup2() calls. (And if it was 1 or 2, then the following dup2 call would have failed, causing us to hit the g_error().)

Created attachment 276692 [details] [review]
gtestutils: Error out if /dev/null stdin redirection fails

Comment on attachment 276598 [details] [review]
gtestutils: Fix a very unlikely FD leak in test fork handling

(In reply to comment #3)
> This is wrong; if fd0 had been 0, 1, or, 2, then it already would have been
> closed at this point by one of the preceding dup2() calls. (And if it was 1 or
> 2, then the following dup2 call would have failed, causing us to hit the
> g_error().)

ooh, that's subtle. Looks like the Coverity issue was a false positive then.

The new patch just adds a g_error().

Review of attachment 276692 [details] [review]:

::: glib/gtestutils.c
@@ +2686,3 @@
+            g_error ("failed to open /dev/null for stdin redirection");
+        }
+      if (sane_dup2 (stdout_pipe[1], 1) < 0 || sane_dup2 (stderr_pipe[1], 2) < 0 || (fd0 > -1 && sane_dup2 (fd0, 0) < 0))

Can you drop the spurious change here?

Pushed with that change removed.

Attachment 276692 [details] pushed as d3fd88d - gtestutils: Error out if /dev/null stdin redirection fails