[pulseaudio-discuss] [PATCHv2 30/60] bluetooth: Implement org.bluez.MediaEndpoint1.SetConfiguration()

[Tanu Kaskinen]

On Mon, 2013-08-19 at 13:57 -0300, João Paulo Rechi Vita wrote:
> On Sun, Aug 18, 2013 at 6:03 AM, Tanu Kaskinen
> <tanu.kaskinen at linux.intel.com> wrote:
> > On Tue, 2013-08-13 at 01:54 -0300, jprvita at gmail.com wrote:
> >> From: João Paulo Rechi Vita <jprvita at openbossa.org>
> >>
> >> ---
> >>  src/modules/bluetooth/bluez5-util.c | 171 +++++++++++++++++++++++++++++++++++-
> >>  src/modules/bluetooth/bluez5-util.h |   5 ++
> >>  2 files changed, 174 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/src/modules/bluetooth/bluez5-util.c b/src/modules/bluetooth/bluez5-util.c
> >> index 9687997..d0428a9 100644
> >> --- a/src/modules/bluetooth/bluez5-util.c
> >> +++ b/src/modules/bluetooth/bluez5-util.c
> >> @@ -33,6 +33,8 @@
> >>  #include <pulsecore/refcnt.h>
> >>  #include <pulsecore/shared.h>
> >>
> >> +#include "a2dp-codecs.h"
> >> +
> >>  #include "bluez5-util.h"
> >>
> >>  #define BLUEZ_SERVICE "org.bluez"
> >> @@ -359,12 +361,177 @@ fail:
> >>      return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
> >>  }
> >>
> >> +const char *pa_bluetooth_profile_to_string(pa_bluetooth_profile_t profile) {
> >> +    switch(profile) {
> >> +        case PROFILE_A2DP_SINK:
> >> +            return "a2dp_sink";
> >> +        case PROFILE_A2DP_SOURCE:
> >> +            return "a2dp_source";
> >> +        case PROFILE_OFF:
> >> +            return "off";
> >> +    }
> >> +
> >> +    return NULL;
> >> +}
> >> +
> >>  static DBusMessage *endpoint_set_configuration(DBusConnection *conn, DBusMessage *m, void *userdata) {
> >> +    pa_bluetooth_discovery *y = userdata;
> >> +    pa_bluetooth_device *d;
> >> +    pa_bluetooth_transport *t;
> >> +    const char *sender, *path, *endpoint_path, *dev_path = NULL, *uuid = NULL;
> >> +    uint8_t *config = NULL;
> >
> > config should be marked as const, because it will point to memory owned
> > by the DBusMessage object.
> >
> 
> Ok.
> 
> >> +    int size = 0;
> >> +    pa_bluetooth_profile_t p = PROFILE_OFF;
> >> +    DBusMessageIter args, props;
> >>      DBusMessage *r;
> >>
> >> -    pa_assert_se(r = dbus_message_new_error(m, BLUEZ_MEDIA_ENDPOINT_INTERFACE ".Error.NotImplemented",
> >> -                                            "Method not implemented"));
> >> +    if (!dbus_message_iter_init(m, &args) || !pa_streq(dbus_message_get_signature(m), "oa{sv}")) {
> >> +        pa_log_error("Invalid signature for method SetConfiguration()");
> >> +        goto fail2;
> >> +    }
> >> +
> >> +    dbus_message_iter_get_basic(&args, &path);
> >> +
> >> +    if (pa_hashmap_get(y->transports, path)) {
> >> +        pa_log_error("Endpoint SetConfiguration(): Transport %s is already configured.", path);
> >> +        goto fail2;
> >> +    }
> >> +
> >> +    pa_assert_se(dbus_message_iter_next(&args));
> >> +
> >> +    dbus_message_iter_recurse(&args, &props);
> >> +    if (dbus_message_iter_get_arg_type(&props) != DBUS_TYPE_DICT_ENTRY)
> >> +        goto fail;
> >> +
> >> +    /* Read transport properties */
> >> +    while (dbus_message_iter_get_arg_type(&props) == DBUS_TYPE_DICT_ENTRY) {
> >> +        const char *key;
> >> +        DBusMessageIter value, entry;
> >> +        int var;
> >> +
> >> +        dbus_message_iter_recurse(&props, &entry);
> >> +        dbus_message_iter_get_basic(&entry, &key);
> >> +
> >> +        dbus_message_iter_next(&entry);
> >> +        dbus_message_iter_recurse(&entry, &value);
> >> +
> >> +        var = dbus_message_iter_get_arg_type(&value);
> >> +
> >> +        if (pa_streq(key, "UUID")) {
> >> +            if (var != DBUS_TYPE_STRING) {
> >> +                pa_log_error("Property %s of wrong type %c", key, (char)var);
> >> +                goto fail;
> >> +            }
> >> +
> >> +            dbus_message_iter_get_basic(&value, &uuid);
> >> +        } else if (pa_streq(key, "Device")) {
> >> +            if (var != DBUS_TYPE_OBJECT_PATH) {
> >> +                pa_log_error("Property %s of wrong type %c", key, (char)var);
> >> +                goto fail;
> >> +            }
> >> +
> >> +            dbus_message_iter_get_basic(&value, &dev_path);
> >> +        } else if (pa_streq(key, "Configuration")) {
> >> +            DBusMessageIter array;
> >> +            a2dp_sbc_t *c;
> >> +
> >> +            if (var != DBUS_TYPE_ARRAY) {
> >> +                pa_log_error("Property %s of wrong type %c", key, (char)var);
> >> +                goto fail;
> >> +            }
> >> +
> >> +            dbus_message_iter_recurse(&value, &array);
> >> +            var = dbus_message_iter_get_arg_type(&array);
> >> +            if (var != DBUS_TYPE_BYTE) {
> >> +                pa_log_error("%s is an array of wrong type %c", key, (char)var);
> >> +                goto fail;
> >> +            }
> >> +
> >> +            dbus_message_iter_get_fixed_array(&array, &config, &size);
> >> +            c = (a2dp_sbc_t *) config;
> >
> > You should check that the config size is what you expect it to be.
> >
> > Also, casting the byte array to an a2dp_sbc_t pointer seems hazardous
> > from memory alignment point of view. I think it would be best to copy
> > the config to a real a2dp_sbc_t variable before accessing the config.
> >
> 
> Why it is hazardous? The BlueZ' media API documentation explicitly
> states that we should the binary blob as-is. From doc/media-api.txt:
> "Configuration blob, it is used as it is so the size and byte order
> must match.". Additionally, the a2dp_sbc_t struct is defined ia a
> BlueZ header, which is duplicated in PulseAudio's tree.

Some CPU architectures have rules about how different data types can be
accessed in memory. Stuff like "32-bit variables must be stored in
memory addresses divisible by 4". I don't remember or know the exact
rules on various architectures, but IIRC ARM is much stricter than x86.

So the problem is that the returned byte array can be located anywhere
in the memory, because there are no alignment requirements for one-byte
data types. a2dp_sbc_t, however, contains... umm... it contains only
uint8_t fields, so never mind. It's safe to cast a byte array to
a2dp_sbc_t after all.

-- 
Tanu