Use a better NoReply message for disconnection with reply pending

As an implementation detail, dbus-daemon handles this situation by
artificially triggering a timeout (even if its configured timeout for
method calls is in fact infinite). However, using the same debug message
for both is misleading, and can lead people who are debugging a service
crash to blame dbus-daemon instead, wasting their time.

Bug: https://bugs.freedesktop.org/show_bug.cgi?id=76112

diff --git a/bus/connection.c b/bus/connection.c
index 519122c..0df8a3a 100644 (file)
--- a/bus/connection.c
+++ b/bus/connection.c
@@ -1619,7 +1619,12 @@ bus_pending_reply_send_no_reply (BusConnections  *connections,
                                     DBUS_ERROR_NO_REPLY))
     goto out;
 
-  errmsg = "Message did not receive a reply (timeout by message bus)";
+  /* If you change these messages, adjust test/dbus-daemon.c to match */
+  if (pending->will_send_reply == NULL)
+    errmsg = "Message recipient disconnected from message bus without replying";
+  else
+    errmsg = "Message did not receive a reply (timeout by message bus)";
+
   dbus_message_iter_init_append (message, &iter);
   if (!dbus_message_iter_append_basic (&iter, DBUS_TYPE_STRING, &errmsg))
     goto out;

diff --git a/configure.ac b/configure.ac
index 89e4d2e..de34d3e 100644 (file)
--- a/configure.ac
+++ b/configure.ac
@@ -1779,6 +1779,7 @@ dbus-1.pc
 dbus-1-uninstalled.pc
 test/data/valid-config-files/debug-allow-all.conf
 test/data/valid-config-files/debug-allow-all-sha1.conf
+test/data/valid-config-files/finite-timeout.conf
 test/data/valid-config-files/incoming-limit.conf
 test/data/valid-config-files-system/debug-allow-all-pass.conf
 test/data/valid-config-files-system/debug-allow-all-fail.conf

diff --git a/test/Makefile.am b/test/Makefile.am
index 1ceb5b6..173df74 100644 (file)
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -254,6 +254,7 @@ in_data = \
        data/valid-config-files-system/debug-allow-all-pass.conf.in \
        data/valid-config-files/debug-allow-all-sha1.conf.in \
        data/valid-config-files/debug-allow-all.conf.in \
+       data/valid-config-files/finite-timeout.conf.in \
        data/valid-config-files/incoming-limit.conf.in \
        data/invalid-service-files-system/org.freedesktop.DBus.TestSuiteNoExec.service.in \
        data/invalid-service-files-system/org.freedesktop.DBus.TestSuiteNoService.service.in \

diff --git a/test/data/valid-config-files/finite-timeout.conf.in b/test/data/valid-config-files/finite-timeout.conf.in
new file mode 100644 (file)
index 0000000..7d26d71
--- /dev/null
+++ b/test/data/valid-config-files/finite-timeout.conf.in
@@ -0,0 +1,19 @@
+<!DOCTYPE busconfig PUBLIC "-//freedesktop//DTD D-Bus Bus Configuration 1.0//EN"
+ "http://www.freedesktop.org/standards/dbus/1.0/busconfig.dtd">
+<busconfig>
+  <!-- Our well-known bus type, don't change this -->
+  <type>session</type>
+  <listen>@TEST_LISTEN@</listen>
+
+  <policy context="default">
+    <!-- Allow everything to be sent -->
+    <allow send_destination="*" eavesdrop="true"/>
+    <!-- Allow everything to be received -->
+    <allow eavesdrop="true"/>
+    <!-- Allow anyone to own anything -->
+    <allow own="*"/>
+  </policy>
+
+  <!-- Forcibly time out method calls after 100ms -->
+  <limit name="reply_timeout">100</limit>
+</busconfig>

diff --git a/test/dbus-daemon.c b/test/dbus-daemon.c
index 4b3b61e..1db4e44 100644 (file)
--- a/test/dbus-daemon.c
+++ b/test/dbus-daemon.c
@@ -57,6 +57,7 @@ typedef struct {
 
     DBusConnection *right_conn;
     gboolean right_conn_echo;
+    gboolean wait_forever_called;
 } Fixture;
 
 #define assert_no_error(e) _assert_no_error (e, __FILE__, __LINE__)
@@ -157,11 +158,19 @@ echo_filter (DBusConnection *connection,
     DBusMessage *message,
     void *user_data)
 {
+  Fixture *f = user_data;
   DBusMessage *reply;
 
   if (dbus_message_get_type (message) != DBUS_MESSAGE_TYPE_METHOD_CALL)
     return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
 
+  /* WaitForever() never replies, emulating a service that has got stuck */
+  if (dbus_message_is_method_call (message, "com.example", "WaitForever"))
+    {
+      f->wait_forever_called = TRUE;
+      return DBUS_HANDLER_RESULT_HANDLED;
+    }
+
   reply = dbus_message_new_method_return (message);
 
   if (reply == NULL)
@@ -257,7 +266,7 @@ setup (Fixture *f,
 static void
 add_echo_filter (Fixture *f)
 {
-  if (!dbus_connection_add_filter (f->right_conn, echo_filter, NULL, NULL))
+  if (!dbus_connection_add_filter (f->right_conn, echo_filter, f, NULL))
     g_error ("OOM");
 
   f->right_conn_echo = TRUE;
@@ -343,6 +352,80 @@ pending_call_store_reply (DBusPendingCall *pc,
 }
 
 static void
+test_no_reply (Fixture *f,
+    gconstpointer context)
+{
+  const Config *config = context;
+  DBusMessage *m;
+  DBusPendingCall *pc;
+  DBusMessage *reply = NULL;
+  enum { TIMEOUT, DISCONNECT } mode;
+  gboolean ok;
+
+  if (f->skip)
+    return;
+
+  g_test_bug ("76112");
+
+  if (config != NULL && config->config_file != NULL)
+    mode = TIMEOUT;
+  else
+    mode = DISCONNECT;
+
+  m = dbus_message_new_method_call (
+      dbus_bus_get_unique_name (f->right_conn), "/",
+      "com.example", "WaitForever");
+
+  add_echo_filter (f);
+
+  if (m == NULL)
+    g_error ("OOM");
+
+  if (!dbus_connection_send_with_reply (f->left_conn, m, &pc,
+                                        DBUS_TIMEOUT_INFINITE) ||
+      pc == NULL)
+    g_error ("OOM");
+
+  if (dbus_pending_call_get_completed (pc))
+    pending_call_store_reply (pc, &reply);
+  else if (!dbus_pending_call_set_notify (pc, pending_call_store_reply, &reply,
+        NULL))
+    g_error ("OOM");
+
+  dbus_pending_call_unref (pc);
+  dbus_message_unref (m);
+
+  if (mode == DISCONNECT)
+    {
+      while (!f->wait_forever_called)
+        test_main_context_iterate (f->ctx, TRUE);
+
+      dbus_connection_remove_filter (f->right_conn, echo_filter, f);
+      dbus_connection_close (f->right_conn);
+      dbus_connection_unref (f->right_conn);
+      f->right_conn = NULL;
+    }
+
+  while (reply == NULL)
+    test_main_context_iterate (f->ctx, TRUE);
+
+  /* using inefficient string comparison for better assertion message */
+  g_assert_cmpstr (
+      dbus_message_type_to_string (dbus_message_get_type (reply)), ==,
+      dbus_message_type_to_string (DBUS_MESSAGE_TYPE_ERROR));
+  ok = dbus_set_error_from_message (&f->e, reply);
+  g_assert (ok);
+  g_assert_cmpstr (f->e.name, ==, DBUS_ERROR_NO_REPLY);
+
+  if (mode == DISCONNECT)
+    g_assert_cmpstr (f->e.message, ==,
+        "Message recipient disconnected from message bus without replying");
+  else
+    g_assert_cmpstr (f->e.message, ==,
+        "Message did not receive a reply (timeout by message bus)");
+}
+
+static void
 test_creds (Fixture *f,
     gconstpointer context)
 {
@@ -475,7 +558,7 @@ teardown (Fixture *f,
     {
       if (f->right_conn_echo)
         {
-          dbus_connection_remove_filter (f->right_conn, echo_filter, NULL);
+          dbus_connection_remove_filter (f->right_conn, echo_filter, f);
           f->right_conn_echo = FALSE;
         }
 
@@ -503,6 +586,10 @@ static Config limited_config = {
     "34393", 10000, "valid-config-files/incoming-limit.conf"
 };
 
+static Config finite_timeout_config = {
+    NULL, 1, "valid-config-files/finite-timeout.conf"
+};
+
 int
 main (int argc,
     char **argv)
@@ -513,6 +600,10 @@ main (int argc,
   g_test_add ("/echo/session", Fixture, NULL, setup, test_echo, teardown);
   g_test_add ("/echo/limited", Fixture, &limited_config,
       setup, test_echo, teardown);
+  g_test_add ("/no-reply/disconnect", Fixture, NULL,
+      setup, test_no_reply, teardown);
+  g_test_add ("/no-reply/timeout", Fixture, &finite_timeout_config,
+      setup, test_no_reply, teardown);
   g_test_add ("/creds", Fixture, NULL, setup, test_creds, teardown);
 
   return g_test_run ();