Re: [PATCH v1 7/8] hfp_hf_bluez5: Add rejecting SCO connection

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5399 bytes --]

Hi Claudio,

On 01/28/2013 03:11 PM, Claudio Takahasi wrote:
> This patch rejects the SCO incoming connection if there isn't a modem
> (service level connection) associated with the address.

Why service level connection is in parenthesis?

> ---
>   plugins/hfp_hf_bluez5.c | 82 ++++++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 68 insertions(+), 14 deletions(-)
>
> diff --git a/plugins/hfp_hf_bluez5.c b/plugins/hfp_hf_bluez5.c
> index 87ae5e3..871c720 100644
> --- a/plugins/hfp_hf_bluez5.c
> +++ b/plugins/hfp_hf_bluez5.c
> @@ -61,6 +61,13 @@
>   struct hfp {
>   	struct hfp_slc_info info;
>   	DBusMessage *msg;
> +	bdaddr_t src;
> +	bdaddr_t dst;
> +};

What is src and what is dst? This is completely context dependent and 
confusing.  Also, we are already storing the device address on the modem 
object, why do we need this yet again?

> +
> +struct sock_bdaddr {
> +	bdaddr_t src;
> +	bdaddr_t dst;
>   };

I'd really like to avoid this structure.  At the very least change the 
name into something that conveys its purpose.  e.g. bdaddr_pair, or 
something.  The way it is now is just silly.

>
>   static GHashTable *modem_hash = NULL;
> @@ -68,6 +75,21 @@ static GHashTable *devices_proxies = NULL;
>   static GDBusClient *bluez = NULL;
>   static guint sco_watch = 0;
>
> +static gboolean modem_bacmp(gpointer key, gpointer value,
> +						gpointer user_data)
> +{
> +	const struct sock_bdaddr *bda = user_data;
> +	struct ofono_modem *modem = value;
> +	struct hfp *hfp = ofono_modem_get_data(modem);
> +	int ret;
> +
> +	ret = bt_bacmp(&bda->dst,&hfp->dst);
> +	if (ret != 0)
> +		return FALSE;
> +
> +	return bt_bacmp(&bda->src,&hfp->src) != 0 ? FALSE : TRUE;
> +}
> +
>   static void hfp_debug(const char *str, void *user_data)
>   {
>   	const char *prefix = user_data;
> @@ -275,14 +297,15 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   {
>   	struct hfp *hfp;
>   	struct ofono_modem *modem;
> +	struct sockaddr_rc src, dst;
> +	socklen_t alen;
>   	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;
>
> @@ -301,11 +324,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;
> @@ -314,7 +332,24 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   	if (fd<  0)
>   		goto invalid;
>
> -	modem = modem_register(device, address, alias);
> +	memset(&src, 0, sizeof(src));
> +	alen = sizeof(src);
> +	if (getsockname(fd, (struct sockaddr *)&src,&alen)<  0) {
> +		close(fd);
> +		goto invalid;
> +	}
> +
> +	memset(&dst, 0, sizeof(dst));
> +	alen = sizeof(dst);
> +	if (getpeername(fd, (struct sockaddr *)&dst,&alen)<  0) {
> +		close(fd);
> +		goto invalid;
> +	}
> +
> +	bt_ba2str(&dst.rc_bdaddr, device_address);
> +	DBG("Profile handler NewConnection: %s", device_address);
> +
> +	modem = modem_register(device, device_address, alias);
>   	if (modem == NULL) {
>   		close(fd);
>   		return g_dbus_create_error(msg, BLUEZ_ERROR_INTERFACE
> @@ -329,6 +364,8 @@ static DBusMessage *profile_new_connection(DBusConnection *conn,
>   					"Not enough resources");
>
>   	hfp = ofono_modem_get_data(modem);
> +	bt_bacpy(&hfp->src,&src.rc_bdaddr);
> +	bt_bacpy(&hfp->dst,&dst.rc_bdaddr);
>   	hfp->msg = dbus_message_ref(msg);
>

Fair enough, but this chunk really belongs in a separate patch.

>   	return NULL;
> @@ -384,7 +421,9 @@ static const GDBusMethodTable profile_methods[] = {
>   static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>   							gpointer user_data)
>   {
> -	struct sockaddr_sco saddr;
> +	struct ofono_modem *modem;
> +	struct sockaddr_sco src, dst;
> +	struct sock_bdaddr bda;
>   	socklen_t alen;
>   	int sk, nsk;
>
> @@ -393,14 +432,29 @@ static gboolean sco_accept(GIOChannel *io, GIOCondition cond,
>
>   	sk = g_io_channel_unix_get_fd(io);
>
> -	memset(&saddr, 0, sizeof(saddr));
> -	alen = sizeof(saddr);
> +	memset(&dst, 0, sizeof(dst));
> +	alen = sizeof(dst);
>
> -	nsk = accept(sk, (struct sockaddr *)&saddr,&alen);
> +	nsk = accept(sk, (struct sockaddr *)&dst,&alen);

When reading this patch I go 'what the...'

>   	if (nsk<  0)
>   		return TRUE;
>
> -	/* TODO: Verify if the device has a modem */
> +	memset(&src, 0, sizeof(src));
> +	alen = sizeof(src);
> +	if (getsockname(nsk, (struct sockaddr *)&src,&alen)<  0) {
> +		close(nsk);
> +		return TRUE;
> +	}
> +
> +	bt_bacpy(&bda.src,&src.sco_bdaddr);
> +	bt_bacpy(&bda.dst,&dst.sco_bdaddr);
> +
> +	modem = g_hash_table_find(modem_hash, modem_bacmp,&bda);
> +	if (modem == NULL) {
> +		ofono_error("Rejecting SCO: SLC connection missing!");
> +		close(nsk);
> +		return TRUE;
> +	}
>
>   	return TRUE;
>   }

Regards,
-Denis