Hi Marcel:
On Mon, Jan 28, 2013 at 1:04 PM, Marcel Holtmann <[email protected]> wrote:
> Hi Claudio,
>
>> This patch implements additional verification checking the Bluetooth
>> source address instead of the remote address only.
>> ---
>> plugins/hfp_hf_bluez5.c | 64
>> ++++++++++++++++++++++++++++++++++---------------
>> 1 file changed, 45 insertions(+), 19 deletions(-)
>>
>> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
>> index d79647b..504b0f3 100644
>> --- a/plugins/hfp_hf_bluez5.c
>> +++ b/plugins/hfp_hf_bluez5.c
>> @@ -60,6 +60,12 @@
>> struct hfp {
>> struct hfp_slc_info info;
>> DBusMessage *msg;
>> + char adapter[18];
>> +};
>> +
>> +struct bt_peer {
>> + char adapter[18];
>> + char device[18];
>> };
>
> I do not get this. I am really not sure what this is all about and why
> having Bluetooth addresses as strings here is a good idea.
Ok. I will use the bdaddr_t
I am trying to keep a standard: the device address is a string stored
in the modem "object".
hfp_hf_bluez4 is also using the same approach.
>
>>
>> static GHashTable *modem_hash = NULL;
>> @@ -70,11 +76,18 @@ static guint sco_watch = 0;
>> static gboolean modem_address_cmp(gpointer key, gpointer value,
>> gpointer user_data)
>> {
>> - const char *dst = user_data;
>> + const struct bt_peer *bt_peer = user_data;
>> struct ofono_modem *modem = value;
>> - const char *address = ofono_modem_get_string(modem, "Address");
>> + const char *device = ofono_modem_get_string(modem, "Address");
>> + struct hfp *hfp;
>> + gboolean ret;
>>
>> - return g_strcmp0(address, dst) != 0 ? FALSE : TRUE;
>> + ret = g_strcmp0(device, bt_peer->device);
>> + if (ret != 0)
>> + return FALSE;
>> +
>> + hfp = ofono_modem_get_data(modem);
>> + return g_strcmp0(hfp->adapter, bt_peer->adapter) != 0 ? FALSE : TRUE;
>> }
>>
>> static void hfp_debug(const char *str, void *user_data)
>> @@ -284,14 +297,14 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>> {
>> struct hfp *hfp;
>> struct ofono_modem *modem;
>> + struct sockaddr_rc src, dst;
>> DBusMessageIter iter;
>> GDBusProxy *proxy;
>> DBusMessageIter entry;
>> - const char *device, *alias, *address;
>> + const char *device, *alias;
>> + char device_address[18];
>> int fd, err;
>>
>> - DBG("Profile handler NewConnection");
>> -
>> if (dbus_message_iter_init(msg, &entry) == FALSE)
>> goto invalid;
>>
>> @@ -310,11 +323,6 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>>
>> dbus_message_iter_get_basic(&iter, &alias);
>>
>> - if (g_dbus_proxy_get_property(proxy, "Address", &iter) == FALSE)
>> - goto invalid;
>> -
>> - dbus_message_iter_get_basic(&iter, &address);
>> -
>> dbus_message_iter_next(&entry);
>> if (dbus_message_iter_get_arg_type(&entry) != DBUS_TYPE_UNIX_FD)
>> goto invalid;
>> @@ -323,7 +331,13 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>> if (fd < 0)
>> goto invalid;
>>
>> - modem = modem_register(device, address, alias);
>> + if (bt_getsockpeers(fd, (struct sockaddr *) &src,
>> + (struct sockaddr *) &dst,
>> + sizeof(struct sockaddr_rc)) < 0)
>> + goto invalid;
>> +
>> + bt_ba2str(&dst.rc_bdaddr, device_address);
>> + modem = modem_register(device, device_address, alias);
>> if (modem == NULL) {
>> close(fd);
>> return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
>> @@ -339,6 +353,9 @@ static DBusMessage
>> *profile_new_connection(DBusConnection *conn,
>>
>> hfp = ofono_modem_get_data(modem);
>> hfp->msg = dbus_message_ref(msg);
>> + bt_ba2str(&src.rc_bdaddr, hfp->adapter);
>> +
>> + DBG("Profile handler NewConnection: %s", device_address);
>>
>> return NULL;
>>
>> @@ -394,9 +411,9 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition
>> cond,
>> gpointer user_data)
>> {
>> struct ofono_modem *modem;
>> - struct sockaddr_sco saddr;
>> + struct sockaddr_sco src, dst;
>> + struct bt_peer bt_peer;
>> socklen_t optlen;
>> - char dst[18];
>> int sk, nsk;
>>
>> if (cond & (G_IO_ERR | G_IO_HUP | G_IO_NVAL))
>> @@ -404,15 +421,24 @@ static gboolean sco_accept(GIOChannel *io,
>> GIOCondition cond,
>>
>> sk = g_io_channel_unix_get_fd(io);
>>
>> - memset(&saddr, 0, sizeof(saddr));
>> - optlen = sizeof(saddr);
>> + memset(&dst, 0, sizeof(dst));
>> + optlen = sizeof(dst);
>>
>> - nsk = accept(sk, (struct sockaddr *) &saddr, &optlen);
>> + nsk = accept(sk, (struct sockaddr *) &dst, &optlen);
>> if (nsk < 0)
>> return TRUE;
>>
>> - bt_ba2str(&saddr.sco_bdaddr, dst);
>> - modem = g_hash_table_find(modem_hash, modem_address_cmp, dst);
>> + if (bt_getsockpeers(nsk, (struct sockaddr *) &src, NULL, optlen) < 0) {
>> + close(nsk);
>> + return TRUE;
>> + }
>> +
>> + bt_ba2str(&src.sco_bdaddr, bt_peer.adapter);
>> + bt_ba2str(&dst.sco_bdaddr, bt_peer.device);
>> +
>> + DBG("SCO: %s < %s", bt_peer.adapter, bt_peer.device);
>> +
>> + modem = g_hash_table_find(modem_hash, modem_address_cmp, &bt_peer);
>
> You do realize that using a hash table if you have to iterate it is
> pretty inefficient. Have you compared your find vs lookup operations?
I agree, but did you realize that the hash key is the device object
path(not the address)?
Remember that we need to support multiple adapters also. NewConnection
sends the device object, Device InterfaceAdded informs the device
object path, the device address and the adapter *object path*.
I could add an additional adapter proxy hash table to be able to map
adapter path to adapter Bluetooth Address, but it doesn't worth.
Regards,
Claudio.
>
> Regards
>
> Marcel
>
>
> _______________________________________________
> ofono mailing list
> [email protected]
> http://lists.ofono.org/listinfo/ofono
_______________________________________________
ofono mailing list
[email protected]
http://lists.ofono.org/listinfo/ofono
