[PATCH] sdp: Prevent duplicate records registration

[PATCH] sdp: Prevent duplicate records registration

[Frédéric Danis]

Check if a record with same UUID and protocol descriptor already exists
before adding new record to server
---

When BlueZ is built with --enable-pnat option, it provides DUN support on RFComm
port 1.
When current version of oFono is started, it also provides DUN support on same
port.
So, we get to 2 SDP records for same UUID and RFComm port.
This patch prevents this.


 src/sdpd-service.c |   35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

diff --git a/src/sdpd-service.c b/src/sdpd-service.c
index 39e05ab..9e8b90c 100644
--- a/src/sdpd-service.c
+++ b/src/sdpd-service.c
@@ -235,6 +235,9 @@ int add_record_to_server(const bdaddr_t *src, sdp_record_t *rec)
 {
 	sdp_data_t *data;
 	sdp_list_t *pattern;
+	sdp_list_t *record;
+	sdp_list_t *protos;
+	int port = -1, psm = -1;
 
 	if (rec->handle == 0xffffffff) {
 		rec->handle = sdp_next_handle();
@@ -245,6 +248,38 @@ int add_record_to_server(const bdaddr_t *src, sdp_record_t *rec)
 			return -EEXIST;
 	}
 
+	if (sdp_get_access_protos(rec, &protos) == 0) {
+		psm = sdp_get_proto_port(protos, L2CAP_UUID);
+		port = sdp_get_proto_port(protos, RFCOMM_UUID);
+
+		sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free, NULL);
+		sdp_list_free(protos, NULL);
+	}
+
+	for (record = sdp_get_record_list(); record; record = record->next) {
+		sdp_record_t *tmp = record->data;
+		int tmp_port = -1, tmp_psm = -1;
+
+		if (sdp_uuid_cmp(&tmp->svclass, &rec->svclass) != 0)
+			continue;
+
+		if (sdp_get_access_protos(tmp, &protos) == 0) {
+			tmp_psm = sdp_get_proto_port(protos, L2CAP_UUID);
+			tmp_port = sdp_get_proto_port(protos, RFCOMM_UUID);
+
+			sdp_list_foreach(protos,
+					 (sdp_list_func_t) sdp_list_free, NULL);
+			sdp_list_free(protos, NULL);
+		}
+
+		if (psm != tmp_psm || port != tmp_port)
+			continue;
+
+		DBG("Record not added as handle 0x%05x already exists with "
+				"same uuid and protos", tmp->handle);
+		return -EEXIST;
+	}
+
 	DBG("Adding record with handle 0x%05x", rec->handle);
 
 	sdp_record_add(src, rec);
-- 
1.7.9.5

Re: [PATCH] sdp: Prevent duplicate records registration

[Johan Hedberg]

Hi Frédéric,

On Wed, Jun 20, 2012, Frédéric Danis wrote:
> Check if a record with same UUID and protocol descriptor already exists
> before adding new record to server
> ---
> 
> When BlueZ is built with --enable-pnat option, it provides DUN support on RFComm
> port 1.
> When current version of oFono is started, it also provides DUN support on same
> port.
> So, we get to 2 SDP records for same UUID and RFComm port.
> This patch prevents this.
> 
> 
>  src/sdpd-service.c |   35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/src/sdpd-service.c b/src/sdpd-service.c
> index 39e05ab..9e8b90c 100644
> --- a/src/sdpd-service.c
> +++ b/src/sdpd-service.c
> @@ -235,6 +235,9 @@ int add_record_to_server(const bdaddr_t *src, sdp_record_t *rec)
>  {
>  	sdp_data_t *data;
>  	sdp_list_t *pattern;
> +	sdp_list_t *record;
> +	sdp_list_t *protos;
> +	int port = -1, psm = -1;
>  
>  	if (rec->handle == 0xffffffff) {
>  		rec->handle = sdp_next_handle();
> @@ -245,6 +248,38 @@ int add_record_to_server(const bdaddr_t *src, sdp_record_t *rec)
>  			return -EEXIST;
>  	}
>  
> +	if (sdp_get_access_protos(rec, &protos) == 0) {
> +		psm = sdp_get_proto_port(protos, L2CAP_UUID);
> +		port = sdp_get_proto_port(protos, RFCOMM_UUID);
> +
> +		sdp_list_foreach(protos, (sdp_list_func_t) sdp_list_free, NULL);
> +		sdp_list_free(protos, NULL);
> +	}
> +
> +	for (record = sdp_get_record_list(); record; record = record->next) {
> +		sdp_record_t *tmp = record->data;
> +		int tmp_port = -1, tmp_psm = -1;
> +
> +		if (sdp_uuid_cmp(&tmp->svclass, &rec->svclass) != 0)
> +			continue;
> +
> +		if (sdp_get_access_protos(tmp, &protos) == 0) {
> +			tmp_psm = sdp_get_proto_port(protos, L2CAP_UUID);
> +			tmp_port = sdp_get_proto_port(protos, RFCOMM_UUID);
> +
> +			sdp_list_foreach(protos,
> +					 (sdp_list_func_t) sdp_list_free, NULL);
> +			sdp_list_free(protos, NULL);
> +		}
> +
> +		if (psm != tmp_psm || port != tmp_port)
> +			continue;
> +
> +		DBG("Record not added as handle 0x%05x already exists with "
> +				"same uuid and protos", tmp->handle);
> +		return -EEXIST;
> +	}
> +
>  	DBG("Adding record with handle 0x%05x", rec->handle);
>  
>  	sdp_record_add(src, rec);

My initial reaction is that I don't think this is something that needs
to be part of the SDP server. The admin of the system should be smart
enough to not try to configure to identical & conflicting services.

Also, the RFCOMM server socket code in the kernel should already give an
error if binding to the same channel is attempted twice, so this would
look like a bug in one of the DUN implementations that they do not
unregister their service record when binding the server socket fails. So
simply fixing this bug would also make sure that two service records
aren't present (though it would remove the existence of the clueless
admin ;)

However, even if this was introduced it should be in its own function
(e.g. sdpd_check_duplicate() called from within add_record_to_server)
and not unnecessarily bloat the size of add_record_to_server.

Johan

Re: [PATCH] sdp: Prevent duplicate records registration

[Frederic Danis]

Hello Johan,

On 20/06/2012 11:35, Johan Hedberg wrote:
> Hi Frédéric,
>
> On Wed, Jun 20, 2012, Frédéric Danis wrote:
>> Check if a record with same UUID and protocol descriptor already exists
>> before adding new record to server
>> ---
>>
>> When BlueZ is built with --enable-pnat option, it provides DUN support on RFComm
>> port 1.
>> When current version of oFono is started, it also provides DUN support on same
>> port.
>> So, we get to 2 SDP records for same UUID and RFComm port.
>> This patch prevents this.
>>
>
> My initial reaction is that I don't think this is something that needs
> to be part of the SDP server. The admin of the system should be smart
> enough to not try to configure to identical&  conflicting services.
>
> Also, the RFCOMM server socket code in the kernel should already give an
> error if binding to the same channel is attempted twice, so this would
> look like a bug in one of the DUN implementations that they do not
> unregister their service record when binding the server socket fails. So
> simply fixing this bug would also make sure that two service records
> aren't present (though it would remove the existence of the clueless
> admin ;)
>
I took a look to oFono code, which used a copy of btio.c to create a 
listening socket.
But call to bt_io_listen() (which calls bind to rfcomm port 1) did not 
fail. So, oFono continue and register the sdp record.

I use upstream BlueZ and oFono, with kernel 3.2.0-25-generic.

> However, even if this was introduced it should be in its own function
> (e.g. sdpd_check_duplicate() called from within add_record_to_server)
> and not unnecessarily bloat the size of add_record_to_server.
>
I will send an updated version of the patch.

Regards

Fred


-- 
Frederic Danis                            Open Source Technology Center
frederic.danis@intel.com                              Intel Corporation

Re: [PATCH] sdp: Prevent duplicate records registration

[Luiz Augusto von Dentz]

Hi Johan,

On Wed, Jun 20, 2012 at 12:35 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:

> Also, the RFCOMM server socket code in the kernel should already give an
> error if binding to the same channel is attempted twice, so this would
> look like a bug in one of the DUN implementations that they do not
> unregister their service record when binding the server socket fails. So
> simply fixing this bug would also make sure that two service records
> aren't present (though it would remove the existence of the clueless
> admin ;)

Good point, and should actually work for L2CAP as well, apparently
__rfcomm_get_sock_by_addr don't check BDADDR_ANY:

		if (rfcomm_pi(sk)->channel == channel &&
				!bacmp(&bt_sk(sk)->src, src))
			break;

So the question is, should we check in every adapter if there is
listen socket for the given channel? This complicates a little bit
more the code, the other option is to oFono don't do the socket
management itself, this might be possible with org.bluez.Telephony, so
just register an agent for DUN as it will be for HFP, bluetoothd takes
care of handling the connection and authorization and when its done
contact oFono.

-- 
Luiz Augusto von Dentz

Re: [PATCH] sdp: Prevent duplicate records registration

[Johan Hedberg]

Hi Luiz,

On Thu, Jun 21, 2012, Luiz Augusto von Dentz wrote:
> On Wed, Jun 20, 2012 at 12:35 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> 
> > Also, the RFCOMM server socket code in the kernel should already give an
> > error if binding to the same channel is attempted twice, so this would
> > look like a bug in one of the DUN implementations that they do not
> > unregister their service record when binding the server socket fails. So
> > simply fixing this bug would also make sure that two service records
> > aren't present (though it would remove the existence of the clueless
> > admin ;)
> 
> Good point, and should actually work for L2CAP as well, apparently
> __rfcomm_get_sock_by_addr don't check BDADDR_ANY:
> 
> 		if (rfcomm_pi(sk)->channel == channel &&
> 				!bacmp(&bt_sk(sk)->src, src))
> 			break;
> 
> So the question is, should we check in every adapter if there is
> listen socket for the given channel?

I think the kernel should return an error is bind is tried with
BDADDR_ANY more than once for the same PSM or RFCOMM channel.

> This complicates a little bit more the code, the other option is to
> oFono don't do the socket management itself, this might be possible
> with org.bluez.Telephony, so just register an agent for DUN as it will
> be for HFP, bluetoothd takes care of handling the connection and
> authorization and when its done

Isn't that exactly what the last two patches (for DUN and SAP) from the
telephony patch set do?

Johan

Re: [PATCH] sdp: Prevent duplicate records registration

[Luiz Augusto von Dentz]

Hi Johan,

On Fri, Jun 29, 2012 at 4:35 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>> This complicates a little bit more the code, the other option is to
>> oFono don't do the socket management itself, this might be possible
>> with org.bluez.Telephony, so just register an agent for DUN as it will
>> be for HFP, bluetoothd takes care of handling the connection and
>> authorization and when its done
>
> Isn't that exactly what the last two patches (for DUN and SAP) from the
> telephony patch set do?

I guess so, but I didn't remember those in the original series so it
might be introduced latter, Frederic?


-- 
Luiz Augusto von Dentz

Re: [PATCH] sdp: Prevent duplicate records registration

[Frederic Danis]

Hello Luiz,

On 29/06/2012 15:43, Luiz Augusto von Dentz wrote:
> Hi Johan,
>
> On Fri, Jun 29, 2012 at 4:35 PM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
>>> This complicates a little bit more the code, the other option is to
>>> oFono don't do the socket management itself, this might be possible
>>> with org.bluez.Telephony, so just register an agent for DUN as it will
>>> be for HFP, bluetoothd takes care of handling the connection and
>>> authorization and when its done
>>
>> Isn't that exactly what the last two patches (for DUN and SAP) from the
>> telephony patch set do?
>
> I guess so, but I didn't remember those in the original series so it
> might be introduced latter, Frederic?
>
You are right, this was introduced in v10 of patches.


-- 
Frederic Danis                            Open Source Technology Center
frederic.danis@intel.com                              Intel Corporation