Bug 56927

Comment on attachment 69822 [details] [review]
Proposal patch to close leaked fd to /dev/null for some products.

Review of attachment 69822 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-sysdeps-unix.c
@@ -3301,5 @@
>        close (result_pipe[READ_END]);
>        close (errors_pipe[READ_END]);
> -      close (0);                /* close stdin */
> -      close (1);                /* close stdout */
> -      close (2);                /* close stderr */

You're right that these close() calls aren't really needed, because dup2() will close the "destination" if necessary.

@@ +3309,3 @@
>          _exit (1);
>  
> +      close (fd);

This close() call is unnecessary...

@@ +3313,1 @@
>        _dbus_close_all ();

... because this closes all fds except 0, 1 and 2 anyway.

::: dbus/dbus-sysdeps-util-unix.c
@@ +123,4 @@
>              dup2 (dev_null_fd, 2);
>            else
>              _dbus_verbose ("keeping stderr open due to DBUS_DEBUG_OUTPUT\n");
> +          close (dev_null_fd);

Correct.

::: tools/dbus-launch.c
@@ +633,4 @@
>        s = getenv ("DBUS_DEBUG_OUTPUT");
>        if (s == NULL || *s == '\0')
>          dup2 (dev_null_fd, 2);
> +      close (dev_null_fd);

Also correct.

> @@ +3309,3 @@
> >          _exit (1);
> >  
> > +      close (fd);
> 
> This close() call is unnecessary...
> 
> @@ +3313,1 @@
> >        _dbus_close_all ();
> 
> ... because this closes all fds except 0, 1 and 2 anyway.
> 

I know about that, I only made this explicit for to make it clean, and to help _dbus_close_all() which looked quite *expensive* compared to a sure to be closed fd.

(In reply to comment #1)
> ::: dbus/dbus-sysdeps-util-unix.c
> ::: tools/dbus-launch.c

Both fixed in git for 1.6.10 and 1.7.0, thanks.

(In reply to comment #1)
> You're right that these close() calls aren't really needed, because dup2()
> will close the "destination" if necessary.

Fixed in git for 1.7.0. I didn't apply this on the stable branch to reduce the diff there, because the extra close() calls are essentially harmless.

(In reply to comment #2)
> > ... because this closes all fds except 0, 1 and 2 anyway.
> > 
> 
> I know about that, I only made this explicit for to make it clean, and to
> help _dbus_close_all() which looked quite *expensive* compared to a sure to
> be closed fd.

I didn't apply this part, on the basis that less code leads to fewer bugs. _dbus_close_all() is going to do just as much (or as little) work either way (well, OK, it'll have to do one more string-to-integer operation on Linux, but that's insignificant compared with the cost of a fork()+exec()).

(In reply to comment #0)
> It does not apply on top of 1.4.20 where I noticed the issue
> so it needs to be adapted for 1.4 branch.

I've cherry-picked the part that actually fixes a leak from 1.6 to 1.4. It applies cleanly.

It will be in 1.4.26 if we ever release that version (hopefully not - the 1.2 and 1.4 branches are only supported for security fixes now).