Bug 89234

Comment on attachment 113674 [details] [review]
Attempt at adding the functionality [untested]

>From fe53b57cc06b91e9b46bd59642fb751d572eff2d Mon Sep 17 00:00:00 2001
>From: Thiago Macieira <thiago@kde.org>
>Date: Thu, 25 Dec 2014 02:30:00 -0200
>Subject: [PATCH 1/1] Add dbus_message_{marshal,demarshal}_with_unix_fds
>
>The original versions (a.k.a "d-tubes") predate the feature of passing
>Unix file descriptors and we never updated the API with it. Without the
>ability to get and set the Unix FDs in the DBusMessage, the "d-tubes"
>API is severely limited.
>---
> dbus/dbus-message.c | 99 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> dbus/dbus-message.h | 15 ++++++++
> 2 files changed, 113 insertions(+), 1 deletion(-)
>
>diff --git a/dbus/dbus-message.c b/dbus/dbus-message.c
>index 01c2367..fe3db04 100644
>--- a/dbus/dbus-message.c
>+++ b/dbus/dbus-message.c
>@@ -675,6 +675,7 @@ dbus_message_cache_or_finalize (DBusMessage *message)
>       _dbus_assert_not_reached ("we would have initialized global locks "
>           "the first time we constructed a message");
>     }
>+  goto out;
> 
>   if (!message_cache_shutdown_registered)
>     {
>@@ -4712,6 +4713,9 @@ dbus_message_type_to_string (int type)
>  * Generally, this function is only useful for encapsulating D-Bus messages in
>  * a different protocol.
>  *
>+ * The byte array returned in marshalled_data_p must be freed with free() when
>+ * no longer needed.
>+ *
>  * @param msg the DBusMessage
>  * @param marshalled_data_p the location to save the marshalled form to
>  * @param len_p the location to save the length of the marshalled form to
>@@ -4722,13 +4726,43 @@ dbus_message_marshal (DBusMessage  *msg,
>                       char        **marshalled_data_p,
>                       int          *len_p)
> {
>+  return dbus_message_marshal_with_unix_fds(msg, marshalled_data_p, len_p, NULL, NULL);
>+}
>+
>+/**
>+ * Turn a DBusMessage into the marshalled form as described in the D-Bus
>+ * specification.
>+ *
>+ * Generally, this function is only useful for encapsulating D-Bus messages in
>+ * a different protocol.
>+ *
>+ * The byte array returned in marshalled_data_p must be freed with free() when
>+ * no longer needed. The array of fds returned in unix_fds_p belongs to the
>+ * DBusMessage and must not be freed and the file descriptors will be closed by
>+ * DBusMessage.
>+ *
>+ * @param msg the DBusMessage
>+ * @param marshalled_data_p the location to save the marshalled form to
>+ * @param len_p the location to save the length of the marshalled form to
>+ * @param unix_fds_p the pointer where the Unix fds will be stored
>+ * @param n_fds_p the location to save the number of fds in the array
>+ * @returns #FALSE if there was not enough memory
>+ */
>+dbus_bool_t
>+dbus_message_marshal_with_unix_fds (DBusMessage *msg,
>+                                    char       **marshalled_data_p,
>+                                    int         *len_p,
>+                                    const int  **unix_fds_p,
>+                                    unsigned    *n_fds_p)
>+{
>   DBusString tmp;
>   dbus_bool_t was_locked;
> 
>   _dbus_return_val_if_fail (msg != NULL, FALSE);
>   _dbus_return_val_if_fail (marshalled_data_p != NULL, FALSE);
>   _dbus_return_val_if_fail (len_p != NULL, FALSE);
>-  
>+  _dbus_return_val_if_fail ((unix_fds_p == NULL) == (n_fds_p == NULL), FALSE);
>+
>   if (!_dbus_string_init (&tmp))
>     return FALSE;
> 
>@@ -4753,6 +4787,16 @@ dbus_message_marshal (DBusMessage  *msg,
> 
>   _dbus_string_free (&tmp);
> 
>+  if (unix_fds_p)
>+    {
>+#ifdef HAVE_UNIX_FD_PASSING
>+      _dbus_message_get_unix_fds (msg, unix_fds_p, n_fds_p);
>+#else
>+      *unix_fds_p = NULL;
>+      *n_fds_p = 0;
>+#endif
>+    }
>+
>   if (!was_locked)
>     msg->locked = FALSE;
> 
>@@ -4784,11 +4828,49 @@ dbus_message_demarshal (const char *str,
>                         int         len,
>                         DBusError  *error)
> {
>+  return dbus_message_demarshal_with_unix_fds (str, len, NULL, 0, error);
>+}
>+
>+/**
>+ * Demarshal a D-Bus message from the format described in the D-Bus
>+ * specification.
>+ *
>+ * Generally, this function is only useful for encapsulating D-Bus messages in
>+ * a different protocol.
>+ *
>+ * This function will copy both arrays (str and unix_fds_p), but it will take
>+ * ownership of the fds themselves.
>+ *
>+ * @param str the marshalled DBusMessage
>+ * @param len the length of str
>+ * @param unix_fds_p the array of fds
>+ * @param n_fds the number of fds in the array
>+ * @param error the location to save errors to
>+ * @returns #NULL if there was an error
>+ */
>+DBusMessage *
>+dbus_message_demarshal_with_unix_fds (const char *str,
>+                                      int         len,
>+                                      const int  *unix_fds_p,
>+                                      unsigned    n_fds,
>+                                      DBusError  *error)
>+{
>+
>   DBusMessageLoader *loader;
>   DBusString *buffer;
>   DBusMessage *msg;
> 
>   _dbus_return_val_if_fail (str != NULL, NULL);
>+  _dbus_return_val_if_fail (unix_fds_p != NULL || n_fds == 0, NULL);
>+
>+#ifndef HAVE_UNIX_FD_PASSING
>+  if (n_fds != 0)
>+    {
>+      dbus_set_error (error, DBUS_ERROR_NOT_SUPPORTED,
>+                      "Passing file descriptors not supported on this platform");
>+      return NULL;
>+    }
>+#endif
> 
>   loader = _dbus_message_loader_new ();
> 
>@@ -4810,6 +4892,18 @@ dbus_message_demarshal (const char *str,
>   if (!msg)
>     goto fail_oom;
> 
>+#ifdef HAVE_UNIX_FD_PASSING
>+  if (n_fds)
>+    {
>+      int *a = dbus_realloc (msg->unix_fds, n_fds * sizeof(unix_fds_p[0]));
>+      if (!a)
>+        goto fail_oom_unref;
>+
>+      msg->unix_fds = a;
>+      msg->n_unix_fds = n_fds;
>+    }
>+#endif
>+
>   _dbus_message_loader_unref (loader);
>   return msg;
> 
>@@ -4819,6 +4913,9 @@ dbus_message_demarshal (const char *str,
>   _dbus_message_loader_unref (loader);
>   return NULL;
> 
>+ fail_oom_unref:
>+  dbus_message_unref (msg);
>+
>  fail_oom:
>   _DBUS_SET_OOM (error);
>   _dbus_message_loader_unref (loader);
>diff --git a/dbus/dbus-message.h b/dbus/dbus-message.h
>index baa7d7b..14112e5 100644
>--- a/dbus/dbus-message.h
>+++ b/dbus/dbus-message.h
>@@ -293,12 +293,27 @@ DBUS_EXPORT
> dbus_bool_t  dbus_message_marshal   (DBusMessage  *msg,
>                                      char        **marshalled_data_p,
>                                      int          *len_p);
>+
>+DBUS_EXPORT
>+dbus_bool_t  dbus_message_marshal_with_unix_fds (DBusMessage  *msg,
>+                                                 char        **marshalled_data_p,
>+                                                 int          *len_p,
>+                                                 const int   **unix_fds_p,
>+                                                 unsigned     *n_fds_p);
>+
> DBUS_EXPORT
> DBusMessage* dbus_message_demarshal (const char *str,
>                                      int         len,
>                                      DBusError  *error);
> 
> DBUS_EXPORT
>+DBusMessage* dbus_message_demarshal_with_unix_fds (const char *str,
>+                                                   int         len,
>+                                                   const int  *unix_fds_p,
>+                                                   unsigned    n_fds,
>+                                                   DBusError  *error);
>+
>+DBUS_EXPORT
> int          dbus_message_demarshal_bytes_needed (const char *str, 
>                                                   int len);
> 
>-- 
>2.1.4
>

Comment on attachment 113674 [details] [review]
Attempt at adding the functionality [untested]

Review of attachment 113674 [details] [review]:
-----------------------------------------------------------------

::: dbus/dbus-message.c
@@ +675,4 @@
>        _dbus_assert_not_reached ("we would have initialized global locks "
>            "the first time we constructed a message");
>      }
> +  goto out;

Oops, unintended.

Created attachment 113675 [details] [review]
v2: New (untested) attempt at providing the functionality

Of course, you only realise your mistakes when you read the patch...

Comment on attachment 113675 [details] [review]
v2: New (untested) attempt at providing the functionality

Review of attachment 113675 [details] [review]:
-----------------------------------------------------------------

Sorry, I must have missed this when you posted it.

> Without the
> ability to get and set the Unix FDs in the DBusMessage, the "d-tubes"
> API is severely limited.

I think that's over-stating it. You use these functions because you want to pass D-Bus messages over an existing side-channel; but you typically use Unix fd passing in D-Bus when you want to set up a side-channel. Chickens, eggs :-)

The "Tubes" name refers to a feature of Telepathy, which tunnels TCP-like streams or D-Bus messages over (possibly link-local) XMPP. Passing Unix fds over that is never going to work.

::: dbus/dbus-message.c
@@ +4737,5 @@
> + *
> + * The byte array returned in marshalled_data_p must be freed with free() when
> + * no longer needed. The array of fds returned in unix_fds_p belongs to the
> + * DBusMessage and must not be freed and the file descriptors will be closed by
> + * DBusMessage.

I'm a bit surprised by the marshalled data being copied whereas the fds are not, but I can see why you did it like this. (I think the equivalent APIs in GDBus do a lot of dup() though.)

@@ +4893,5 @@
>  
> +#ifdef HAVE_UNIX_FD_PASSING
> +  if (n_fds)
> +    {
> +      int *a = dbus_realloc (msg->unix_fds, n_fds * sizeof(unix_fds_p[0]));

This path also needs to set n_unix_fds_allocated.

@@ +4918,1 @@
>   fail_oom:

I really prefer the pattern where we do things like

DBusMessage *msg = NULL;

...

if (!thing)
  goto fail_oom:

...

fail_oom:
  if (msg != NULL)
    dbus_message_unref (msg);

  if (whatever != NULL)
    whatever_free (whatever);
  ...

rather than proliferating labels to free different amounts of stuff.

I'd be happy to apply a tested patch with the n_unix_fds_allocated thing and the cleanup fixed.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/121.