Created attachment 48389 [details] [review] patch fixing issue The D-Bus daemon does not have a SIGTERM handler, so it always leaves /tmp/dbus-XXX socket files. Each time a user logs in it leaves one socket file for the "gdm" user and another for the user who just logged out. This is likely not an issue for Linux since abstract sockets are used in that environment. However, it seems nicer to make D-Bus properly clean things up on shutdown like this. Adding a SIGTERM handler that works like SIGHUP fixes this issue. See attached patch.
Review of attachment 48389 [details] [review]: ::: dbus-1.2.28/bus/main.c-orig @@ +66,3 @@ + action[0] = ACTION_RELOAD; + action[1] = '\0'; + _dbus_string_init_const (&str, action); Make action local too? DBusString str; char action[2] = { ACTION_RELOAD, '\0' }; ... Actually, since POSIX signal handlers are necessarily Unix-only, I'd somewhat prefer to just use write() directly, something like: char action = ACTION_RELOAD; if ((reload_pipe[RELOAD_WRITE_END] > 0) && write (reload_pipe[RELOAD_WRITE_END], &char, 1) != 1) but that would require backporting commit c7ef3ead5 from master to stop pretending this is portable to non-Unix (which might be a good idea anyway). @@ +85,3 @@ + !_dbus_write_socket (reload_pipe[RELOAD_WRITE_END], &str, 0, 1)) + { + _dbus_warn ("Unable to write to reload pipe.\n"); _dbus_warn() is not safe in an async signal handler. It calls fprintf, which is a libc stdio function, which can call malloc - but in a signal handler, we might be half way through a call to malloc. In master, the SIGHUP case was fixed in commit c7ef3ead5, to use write(). @@ +194,3 @@ + action = ACTION_UNSET; + action_str = _dbus_string_get_data (&str); + if (action_str != NULL && action_str[0] == ACTION_RELOAD) How about just: char action; if (action_str != NULL) action = action_str[0]; (We know that action_str is at most one byte long.) @@ +209,3 @@ */ dbus_error_init (&error); + if (action == ACTION_RELOAD) I'd vaguely prefer this to be a switch statement, so it doesn't become a big if/elseif/elseif... construct if we add more actions. @@ +223,3 @@ + else if (action == ACTION_QUIT) + { + _dbus_loop_quit (bus_context_get_loop (context)); Colin removed an existing SIGTERM handler back in 2001, to fix Bug #26303 (also on Solaris, as it happens). So, you should add a comment something like this in some relevant place (perhaps here): /* on OSs without abstract sockets, we want to quit * gracefully rather than being killed by SIGTERM, * so that DBusServer gets a chance to clean up the * sockets from the filesystem. fd.o #38656 */ However, you should also be careful that Bug #26303 does not come back: check whether bus_context_get_loop (context) can ever return NULL (including situations where we get SIGTERM multiple times, for instance), and if it can, don't call _dbus_loop_quit then.
With SIGTERM going via a pipe, the rationale for ignoring errors given in this commit: http://cgit.freedesktop.org/~smcv/dbus/commit/?id=50c81a35a7c6 will no longer be true. Perhaps the way the signal handler requests termination should be close() on the write end of the pipe, rather than write()? (This would result in the watch callback being invoked, but read() returning 0 - not to be confused with an actual read error, which would be signalled as read() returning negative.) This commit: http://cgit.freedesktop.org/~smcv/dbus/commit/?id=c7ef3ead558e147 (and probably also 50c81a35a7c6, linked above) should be backported before making the changes Brian has suggested. Would people be OK with that for the stable branch, in principle?
Created attachment 48489 [details] [review] Updated patch Here is an updated patch that reflects your comments. I've improved the code with most of the suggestions. I did not change the code to use write() since I am not sure I really understand the POSIX ramifications you speak about. Also, the code now seems to look pretty nice with the other changes you recommended. That said, I am happy if someone wants to make that change.
Created attachment 48560 [details] [review] Brian's patch as applied to dbus-1.4, in git format-patch format Sorry, I didn't notice while reviewing that your patch was against D-Bus 1.2. We no longer support 1.2 for anything other than security fixes; please upgrade to 1.4 if possible. The patch basically applies to 1.4, but some of the context needed a bit of adjustment; here's what I plan to apply. I've set this patch to be attributed to you, I hope that's OK. If you commit patches to a local git branch and send them in git format-patch format in future, they'll be correctly attributed to you already.
Created attachment 48561 [details] [review] Conditionalize inclusion of unistd.h Windows doesn't have it, for instance. (Just a simple portability fix for Brian's patch; please review.)
Created attachment 48562 [details] [review] bus signal_handler: don't pretend to be portable away from Unix Signals are POSIX but not ISO C, so guard it with DBUS_UNIX. dbus-sysdeps-util-win doesn't actually implement _dbus_set_signal_handler anyway, so not compiling this code on non-Unix seems more honest. Backported to dbus-1.4, originally part of commit c7ef3ead558e147. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33336 Reviewed-by: Thiago Macieira <thiago@kde.org> (I'll consider this pre-reviewed and commit it to 1.4 unless someone objects, since it's a simple backport.)
Created attachment 48563 [details] [review] bus signal_handler: comment why it's OK if the reload pipe gets full Backported to dbus-1.4, originally part of commit c7ef3ead558e147. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=33336 Reviewed-by: Thiago Macieira <thiago@kde.org> (I'll consider this pre-reviewed and commit it to 1.4 unless someone objects, since it's a simple backport.)
Created attachment 48564 [details] [review] bus signal_handler: call _exit in the unlikely event that the pipe is full On OSs with abstract sockets, this is close enough. On OSs without abstract sockets, this results in failing to clean up Unix sockets in /tmp if someone has sent us thousands of SIGHUP signals since we last entered the main loop - I think that's acceptable.
Created attachment 49266 [details] [review] _dbus_server_new_for_domain_socket: don't try to unlink abstract sockets Our abstract socket names look like filenames (/tmp/dbus-MwozdykBNK or whatever), so if we incorrectly unlink the abstract socket name, in highly unlikely circumstances we could accidentally unlink a non-abstract socket belonging to another process!
Review of attachment 48561 [details] [review]: ++
Review of attachment 48564 [details] [review]: This seems acceptable to me, too.
Review of attachment 49266 [details] [review]: Looks good.
Thanks, fixed in 1.4.16, 1.5.8
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.