Created attachment 69822 [details] [review] Proposal patch to close leaked fd to /dev/null for some products. Hi, When looking at the fd list of dbus-daemon (and later found the same issue with other dbus tool), I found that the program is leaking the fd 3 opened to /dev/null, deduced by the fact that fd 0, 1 and 2 where set to /dev/null. Looking at the code, it is obviously leaking the setup fd at various places. See the patch attached. Note that I made this patch on top of master branch, it applies cleanly on top of 1.6.8. 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.
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).
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.