On Tue, Aug 20, 2013 at 2:46 AM, Tanu Kaskinen
<tanu.kaskinen at linux.intel.com> wrote:
> 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.
>
Ok.
--
Jo?o Paulo Rechi Vita
http://about.me/jprvita
