Bug 38656

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