Bug 624546 – Modification of GDBusMessage in filter function

Allow modifying a GDBusMessage in a filter function and also add tests
for this. This breaks API but leaves ABI (almost) intact - at least
dconf's GSettings backend (the only big user I know of) will keep
working.

https://bugzilla.gnome.org/show_bug.cgi?id=624546

Signed-off-by: David Zeuthen <davidz@redhat.com>

docs/reference/gio/gio-sections.txt

@@ -2425,6 +2425,7 @@ g_dbus_connection_send_message
 g_dbus_connection_send_message_with_reply
 g_dbus_connection_send_message_with_reply_finish
 g_dbus_connection_send_message_with_reply_sync
+GDBusMessageFilterResult
 GDBusMessageFilterFunction
 g_dbus_connection_add_filter
 g_dbus_connection_remove_filter

gio/gdbusconnection.c

@@ -1944,6 +1944,7 @@ on_worker_message_received (GDBusWorker  *worker,
   GDBusConnection *connection = G_DBUS_CONNECTION (user_data);
   FilterCallback *filters;
   gboolean consumed_by_filter;
+  gboolean altered_by_filter;
   guint num_filters;
   guint n;
 
@@ -1965,17 +1966,39 @@ on_worker_message_received (GDBusWorker  *worker,
 
   /* the call the filters in order (without holding the lock) */
   consumed_by_filter = FALSE;
+  altered_by_filter = FALSE;
   for (n = 0; n < num_filters; n++)
     {
-      consumed_by_filter = filters[n].func (connection,
+      GDBusMessageFilterResult result;
-                                            message,
+      result = filters[n].func (connection,
-                                            TRUE,
+                                message,
-                                            filters[n].user_data);
+                                TRUE,
+                                filters[n].user_data);
+      switch (result)
+        {
+        case G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT:
+          /* do nothing */
+          break;
+
+        default:
+          g_warning ("Treating unknown value %d for GDBusMessageFilterResult from filter "
+                     "function on incoming message as MESSAGE_CONSUMED.", result);
+          /* explicit fallthrough */
+        case G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_CONSUMED:
+          consumed_by_filter = TRUE;
+          break;
+
+        case G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED:
+          altered_by_filter = TRUE;
+          break;
+        }
       if (consumed_by_filter)
         break;
     }
 
-  /* Standard dispatch unless the filter ate the message */
+  /* Standard dispatch unless the filter ate the message - no need to
+   * do anything if the message was altered
+   */
   if (!consumed_by_filter)
     {
       GDBusMessageType message_type;
@@ -2020,19 +2043,21 @@ on_worker_message_received (GDBusWorker  *worker,
 }
 
 /* Called in worker's thread */
-static gboolean
+static GDBusMessageFilterResult
 on_worker_message_about_to_be_sent (GDBusWorker  *worker,
                                     GDBusMessage *message,
                                     gpointer      user_data)
 {
   GDBusConnection *connection = G_DBUS_CONNECTION (user_data);
   FilterCallback *filters;
-  gboolean consumed_by_filter;
   guint num_filters;
   guint n;
+  GDBusMessageFilterResult ret;
 
   //g_debug ("in on_worker_message_about_to_be_sent");
 
+  ret = G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
+
   g_object_ref (connection);
 
   /* First collect the set of callback functions */
@@ -2047,22 +2072,39 @@ on_worker_message_about_to_be_sent (GDBusWorker  *worker,
     }
   CONNECTION_UNLOCK (connection);
 
-  /* the call the filters in order (without holding the lock) */
+  /* then call the filters in order (without holding the lock) */
-  consumed_by_filter = FALSE;
   for (n = 0; n < num_filters; n++)
     {
-      consumed_by_filter = filters[n].func (connection,
+      GDBusMessageFilterResult result;
-                                            message,
+      result = filters[n].func (connection,
-                                            FALSE,
+                                message,
-                                            filters[n].user_data);
+                                FALSE,
-      if (consumed_by_filter)
+                                filters[n].user_data);
-        break;
+      switch (result)
+        {
+        case G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT:
+          /* do nothing, ret might already be _ALTERED */
+          break;
+
+        default:
+          g_warning ("Treating unknown value %d for GDBusMessageFilterResult from filter "
+                     "function on outgoing message as MESSAGE_CONSUMED.", result);
+          /* explicit fallthrough */
+        case G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_CONSUMED:
+          ret = G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_CONSUMED;
+          goto out;
+
+        case G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED:
+          ret = G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED;
+          break;
+        }
     }
 
+ out:
   g_object_unref (connection);
   g_free (filters);
 
-  return consumed_by_filter;
+  return ret;
 }
 
 /* Called in worker's thread - we must not block */
@@ -2674,7 +2716,9 @@ static guint _global_filter_id = 1;
  * are run in the order that they were added.  The same handler can be
  * added as a filter more than once, in which case it will be run more
  * than once.  Filters added during a filter callback won't be run on
- * the message being processed.
+ * the message being processed. Filter functions are allowed to modify
+ * and even drop messages - see the #GDBusMessageFilterResult
+ * enumeration for details.
  *
  * Note that filters are run in a dedicated message handling thread so
  * they can't block and, generally, can't do anything but signal a
@@ -2683,10 +2727,9 @@ static guint _global_filter_id = 1;
  * g_dbus_connection_signal_subscribe() or
  * g_dbus_connection_call() instead.
  *
- * If a filter consumes an incoming message (by returning %TRUE), the
+ * If a filter consumes an incoming message the message is not
- * message is not dispatched anywhere else - not even the standard
+ * dispatched anywhere else - not even the standard dispatch machinery
- * dispatch machinery (that API such as
+ * (that API such as g_dbus_connection_signal_subscribe() and
- * g_dbus_connection_signal_subscribe() and
  * g_dbus_connection_send_message_with_reply() relies on) will see the
  * message. Similary, if a filter consumes an outgoing message, the
  * message will not be sent to the other peer.

gio/gdbusconnection.h

@@ -475,15 +475,21 @@ void             g_dbus_connection_signal_unsubscribe         (GDBusConnection
  *
  * Signature for function used in g_dbus_connection_add_filter().
  *
- * Returns: %TRUE if the filter handled @message, %FALSE to let other
+ * If you modify an outgoing message, make sure to return
- * handlers run.
+ * %G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED instead of
+ * %G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT so the message can be
+ * re-serialized. If an error occurs during re-serialization, a
+ * warning will be printed on standard error.
+ *
+ * Returns: A value from the #GDBusMessageFilterResult enumeration
+ * describing what to do with @message.
  *
  * Since: 2.26
  */
-typedef gboolean (*GDBusMessageFilterFunction) (GDBusConnection *connection,
+typedef GDBusMessageFilterResult (*GDBusMessageFilterFunction) (GDBusConnection *connection,
-                                                GDBusMessage    *message,
+                                                                GDBusMessage    *message,
-                                                gboolean         incoming,
+                                                                gboolean         incoming,
-                                                gpointer         user_data);
+                                                                gpointer         user_data);
 
 guint g_dbus_connection_add_filter (GDBusConnection            *connection,
                                     GDBusMessageFilterFunction  filter_function,

gio/gdbusprivate.c

@@ -468,12 +468,12 @@ _g_dbus_worker_emit_message_received (GDBusWorker  *worker,
     worker->message_received_callback (worker, message, worker->user_data);
 }
 
-static gboolean
+static GDBusMessageFilterResult
 _g_dbus_worker_emit_message_about_to_be_sent (GDBusWorker  *worker,
                                               GDBusMessage *message)
 {
-  gboolean ret;
+  GDBusMessageFilterResult ret;
-  ret = FALSE;
+  ret = G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
   if (!worker->stopped)
     ret = worker->message_about_to_be_sent_callback (worker, message, worker->user_data);
   return ret;
@@ -1241,23 +1241,56 @@ maybe_write_next_message (GDBusWorker *worker)
    */
   if (data != NULL)
     {
-      gboolean message_was_dropped;
+      GDBusMessageFilterResult filter_result;
-      message_was_dropped = _g_dbus_worker_emit_message_about_to_be_sent (worker, data->message);
+      guchar *new_blob;
-      if (G_UNLIKELY (message_was_dropped))
+      gsize new_blob_size;
+      GError *error;
+
+      filter_result = _g_dbus_worker_emit_message_about_to_be_sent (worker, data->message);
+      switch (filter_result)
         {
+        case G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT:
+          /* do nothing */
+          break;
+
+        case G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_CONSUMED:
+          /* drop message */
           g_mutex_lock (worker->write_lock);
           worker->num_writes_pending -= 1;
           g_mutex_unlock (worker->write_lock);
           message_to_write_data_free (data);
           goto write_next;
+
+        case G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED:
+          /* reencode altered message */
+          error = NULL;
+          new_blob = g_dbus_message_to_blob (data->message,
+                                             &new_blob_size,
+                                             worker->capabilities,
+                                             &error);
+          if (new_blob == NULL)
+            {
+              /* if filter make the GDBusMessage unencodeable, just complain on stderr and send
+               * the old message instead
+               */
+              g_warning ("Error encoding GDBusMessage with serial %d altered by filter function: %s",
+                         g_dbus_message_get_serial (data->message),
+                         error->message);
+              g_error_free (error);
+            }
+          else
+            {
+              g_free (data->blob);
+              data->blob = (gchar *) new_blob;
+              data->blob_size = new_blob_size;
+            }
+          break;
         }
-      else
+
-        {
+      write_message_async (worker,
-          write_message_async (worker,
+                           data,
-                               data,
+                           write_message_cb,
-                               write_message_cb,
+                           data);
-                               data);
-        }
     }
 }
 

gio/gdbusprivate.h

@@ -39,9 +39,9 @@ typedef void (*GDBusWorkerMessageReceivedCallback) (GDBusWorker   *worker,
                                                     GDBusMessage  *message,
                                                     gpointer       user_data);
 
-typedef gboolean (*GDBusWorkerMessageAboutToBeSentCallback) (GDBusWorker   *worker,
+typedef GDBusMessageFilterResult (*GDBusWorkerMessageAboutToBeSentCallback) (GDBusWorker   *worker,
-                                                             GDBusMessage  *message,
+                                                                             GDBusMessage  *message,
-                                                             gpointer       user_data);
+                                                                             gpointer       user_data);
 
 typedef void (*GDBusWorkerDisconnectedCallback)    (GDBusWorker   *worker,
                                                     gboolean       remote_peer_vanished,

gio/gio.symbols

@@ -1040,6 +1040,7 @@ g_dbus_signal_flags_get_type G_GNUC_CONST
 g_dbus_send_message_flags_get_type G_GNUC_CONST
 g_credentials_type_get_type G_GNUC_CONST
 g_dbus_message_byte_order_get_type G_GNUC_CONST
+g_dbus_message_filter_result_get_type G_GNUC_CONST
 #endif
 #endif
 

gio/gioenums.h

@@ -1214,6 +1214,31 @@ typedef enum
   G_DBUS_MESSAGE_BYTE_ORDER_LITTLE_ENDIAN = 'l'
 } GDBusMessageByteOrder;
 
+/**
+ * GDBusMessageFilterResult:
+ * @G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT: The filter function had
+ * no effect on the message - the message will be passed on to the
+ * next filter function and/or sent to the remote peer.
+ * @G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_CONSUMED: The message was
+ * consumed by the filter function and will be dropped - the message
+ * will not be passed to other filter functions and/or sent to the
+ * remote peer.
+ * @G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED: The message was
+ * modified - the message will still be passed on to the next filter
+ * function and/or sent to the remote peer.
+ *
+ * Possible return values for #GDBusMessageFilterFunction when
+ * handling a #GDBusMessage.
+ *
+ * Since: 2.26
+ */
+typedef enum
+{
+  G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT,
+  G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_CONSUMED,
+  G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED
+} GDBusMessageFilterResult;
+
 G_END_DECLS
 
 #endif /* __GIO_ENUMS_H__ */

gio/tests/gdbus-connection.c

@@ -62,13 +62,13 @@ static const GDBusInterfaceVTable boo_vtable =
   NULL  /* _set_property */
 };
 
-static gboolean
+static GDBusMessageFilterResult
 some_filter_func (GDBusConnection *connection,
                   GDBusMessage    *message,
                   gboolean         incoming,
                   gpointer         user_data)
 {
-  return FALSE;
+  return G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
 }
 
 static void
@@ -686,7 +686,7 @@ typedef struct
   guint32 serial;
 } FilterData;
 
-static gboolean
+static GDBusMessageFilterResult
 filter_func (GDBusConnection *connection,
              GDBusMessage    *message,
              gboolean         incoming,
@@ -706,6 +706,75 @@ filter_func (GDBusConnection *connection,
       data->num_outgoing += 1;
     }
 
+  return G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
+}
+
+typedef struct
+{
+  GDBusMessageFilterResult incoming;
+  GDBusMessageFilterResult outgoing;
+} FilterEffects;
+
+static GDBusMessageFilterResult
+other_filter_func (GDBusConnection *connection,
+                   GDBusMessage    *message,
+                   gboolean         incoming,
+                   gpointer         user_data)
+{
+  FilterEffects *effects = user_data;
+  GDBusMessageFilterResult ret;
+
+  if (incoming)
+    ret = effects->incoming;
+  else
+    ret = effects->outgoing;
+
+  if (ret == G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED)
+    {
+      GVariant *body;
+      gchar *s;
+      gchar *s2;
+      body = g_dbus_message_get_body (message);
+      g_variant_get (body, "(s)", &s);
+      s2 = g_strdup_printf ("MOD: %s", s);
+      g_dbus_message_set_body (message, g_variant_new ("(s)", s2));
+      g_free (s2);
+      g_free (s);
+    }
+
+  return ret;
+}
+
+static void
+test_connection_filter_name_owner_changed_signal_handler (GDBusConnection  *connection,
+                                                          const gchar      *sender_name,
+                                                          const gchar      *object_path,
+                                                          const gchar      *interface_name,
+                                                          const gchar      *signal_name,
+                                                          GVariant         *parameters,
+                                                          gpointer         user_data)
+{
+  const gchar *name;
+  const gchar *old_owner;
+  const gchar *new_owner;
+
+  g_variant_get (parameters,
+                 "(&s&s&s)",
+                 &name,
+                 &old_owner,
+                 &new_owner);
+
+  if (g_strcmp0 (name, "com.example.TestService") == 0 && strlen (new_owner) > 0)
+    {
+      g_main_loop_quit (loop);
+    }
+}
+
+static gboolean
+test_connection_filter_on_timeout (gpointer user_data)
+{
+  g_printerr ("Timeout waiting 1000 msec on service\n");
+  g_assert_not_reached ();
   return FALSE;
 }
 
@@ -718,6 +787,11 @@ test_connection_filter (void)
   GDBusMessage *r;
   GError *error;
   guint filter_id;
+  guint timeout_mainloop_id;
+  guint signal_handler_id;
+  FilterEffects effects;
+  GVariant *result;
+  const gchar *s;
 
   memset (&data, '\0', sizeof (FilterData));
 
@@ -781,6 +855,73 @@ test_connection_filter (void)
   g_assert_cmpint (data.num_handled, ==, 3);
   g_assert_cmpint (data.num_outgoing, ==, 3);
 
+  /* this is safe; testserver will exit once the bus goes away */
+  g_assert (g_spawn_command_line_async (SRCDIR "/gdbus-testserver.py", NULL));
+  /* wait for service to be available */
+  signal_handler_id = g_dbus_connection_signal_subscribe (c,
+                                                          "org.freedesktop.DBus", /* sender */
+                                                          "org.freedesktop.DBus",
+                                                          "NameOwnerChanged",
+                                                          "/org/freedesktop/DBus",
+                                                          NULL, /* arg0 */
+                                                          G_DBUS_SIGNAL_FLAGS_NONE,
+                                                          test_connection_filter_name_owner_changed_signal_handler,
+                                                          NULL,
+                                                          NULL);
+  g_assert_cmpint (signal_handler_id, !=, 0);
+  timeout_mainloop_id = g_timeout_add (1000, test_connection_filter_on_timeout, NULL);
+  g_main_loop_run (loop);
+  g_source_remove (timeout_mainloop_id);
+  g_dbus_connection_signal_unsubscribe (c, signal_handler_id);
+
+  /* now test all nine combinations... */
+
+  filter_id = g_dbus_connection_add_filter (c,
+                                            other_filter_func,
+                                            &effects,
+                                            NULL);
+  /* -- */
+  effects.incoming = G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
+  effects.outgoing = G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
+  error = NULL;
+  result = g_dbus_connection_call_sync (c,
+                                        "com.example.TestService",      /* bus name */
+                                        "/com/example/TestObject",      /* object path */
+                                        "com.example.Frob",             /* interface name */
+                                        "HelloWorld",                   /* method name */
+                                        g_variant_new ("(s)", "Cat"),   /* parameters */
+                                        G_VARIANT_TYPE ("(s)"),         /* return type */
+                                        G_DBUS_CALL_FLAGS_NONE,
+                                        -1,
+                                        NULL,
+                                        &error);
+  g_assert_no_error (error);
+  g_variant_get (result, "(&s)", &s);
+  g_assert_cmpstr (s, ==, "You greeted me with 'Cat'. Thanks!");
+  g_variant_unref (result);
+  /* -- */
+  effects.incoming = G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED;
+  effects.outgoing = G_DBUS_MESSAGE_FILTER_RESULT_MESSAGE_ALTERED;
+  error = NULL;
+  result = g_dbus_connection_call_sync (c,
+                                        "com.example.TestService",      /* bus name */
+                                        "/com/example/TestObject",      /* object path */
+                                        "com.example.Frob",             /* interface name */
+                                        "HelloWorld",                   /* method name */
+                                        g_variant_new ("(s)", "Cat"),   /* parameters */
+                                        G_VARIANT_TYPE ("(s)"),         /* return type */
+                                        G_DBUS_CALL_FLAGS_NONE,
+                                        -1,
+                                        NULL,
+                                        &error);
+  g_assert_no_error (error);
+  g_variant_get (result, "(&s)", &s);
+  g_assert_cmpstr (s, ==, "MOD: You greeted me with 'MOD: Cat'. Thanks!");
+  g_variant_unref (result);
+
+
+  g_dbus_connection_remove_filter (c, filter_id);
+
   _g_object_wait_for_single_ref (c);
   g_object_unref (c);
   g_object_unref (m);

gio/tests/gdbus-peer.c

@@ -1250,7 +1250,7 @@ test_credentials (void)
 #define OVERFLOW_NUM_SIGNALS 5000
 #define OVERFLOW_TIMEOUT_SEC 10
 
-static gboolean
+static GDBusMessageFilterResult
 overflow_filter_func (GDBusConnection *connection,
                       GDBusMessage    *message,
                       gboolean         incoming,
@@ -1258,7 +1258,7 @@ overflow_filter_func (GDBusConnection *connection,
 {
   volatile gint *counter = user_data;
   *counter += 1;
-  return FALSE; /* don't drop the message */
+  return G_DBUS_MESSAGE_FILTER_RESULT_NO_EFFECT;
 }
 
 static gboolean