Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

[Fabian Greffrath]

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

Dear Bluez developers,

recently I have experienced a problem with one of my USB bluetooth 
dongles: Whenever I plugged it into an USB port, the kernel detected 
the device (i.e. reasonable output in dmesg) and the correct kernel 
module (i.e. btusb) was loaded, but bluez frontends like 
gnome-bluetooth could still not activate the device. The bug that I 
reported against this software can be found at: 
<https://bugzilla.gnome.org/show_bug.cgi?id=617050>.

It turned out that my USB dongle was the culprit, since it had an 
invalid default device name that contained non-UTF-8 characters, as 
reported by 'hciconfig -a'. When I changed the device name to 
something reasonable via e.g. 'hciconfig hci0 name foobar', I had to 
*restart* the bluetoothd daemon afterwards in order to get everything 
working as expected. However, when I unplugged the USB dongle and 
plugged it back in, the device name was reset to the defective default 
again and I had to repeat this procedure.

Since I found it unreasonable to be forced to manually change the 
device name and restart the daemon each and every time I plugged my 
USB dongle in, I wanted a solution that detected a faulty device name 
string during the device initialization phase and instantly fixed it 
by writing a converted string back to the device. The attached patch 
does exact this and makes my USB dongle work out-of-the-box, despite 
its defective default device name.

Please review the patch for integration into Bluez.

Cheers,
Fabian

PS: There is still one open question that I would like to address: 
Since I am going to print both the faulty and the fixed device name 
strings to stderr via debug(), should I make sure the strings do not 
contain any ASCII control characters prior to that? At least this is 
what is done in tools/hciconfig.c:442.


[-- Attachment #2: fix_invalid_non-utf-8_device_name.patch --]
[-- Type: text/x-diff, Size: 1064 bytes --]

--- bluez-4.64.orig/plugins/hciops.c
+++ bluez-4.64/plugins/hciops.c
@@ -119,6 +119,7 @@ static void init_device(int index)
 	struct hci_dev_info di;
 	pid_t pid;
 	int dd, err;
+	char name[249];
 
 	/* Do initialization in the separate process */
 	pid = fork();
@@ -143,6 +144,30 @@ static void init_device(int index)
 		exit(1);
 	}
 
+	if (hci_read_local_name(dd, sizeof(name), name, 1000) < 0) {
+		error("Can't read local name on hci%d: %s (%d)\n",
+						index, strerror(errno), errno);
+		exit(1);
+	}
+
+	name[248] = '\0';
+
+	if (!g_utf8_validate(name, -1, NULL)) {
+		char *utf8_name;
+
+		utf8_name = g_convert(name, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL);
+		if (utf8_name) {
+			debug("Fix invalid non-UTF-8 device name \"%s\" on hci%d to \"%s\"",
+							name, index, utf8_name);
+			if (hci_write_local_name(dd, utf8_name, 2000) < 0) {
+				error("Can't change local name on hci%d: %s (%d)\n",
+								index, strerror(errno), errno);
+				exit(1);
+			}
+			g_free(utf8_name);
+		}
+	}
+
 	memset(&dr, 0, sizeof(dr));
 	dr.dev_id = index;
 

Re: Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

[Fabian Greffrath]

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

Dear Bluez list,

I am sorry, I know this is the worst way to start getting in touch 
with a software project, but I accidently attached the wrong patch to 
my previous email. It's configure_device() that must be patched. 
Please find a corrected patch attached (it has been written against 
bluez 4.63 but applies perfectly against 4.64).

Cheers,
Fabian

[-- Attachment #2: fix_invalid_non-utf-8_device_name.patch --]
[-- Type: text/x-diff, Size: 1106 bytes --]

--- bluez-4.63.orig/plugins/hciops.c
+++ bluez-4.63/plugins/hciops.c
@@ -81,6 +81,7 @@ static void configure_device(int index)
 	struct hci_dev_info di;
 	uint16_t policy;
 	int dd, err;
+	char name[249];
 
 	if (hci_devinfo(index, &di) < 0)
 		return;
@@ -96,6 +97,30 @@ static void configure_device(int index)
 		return;
 	}
 
+	if (hci_read_local_name(dd, sizeof(name), name, 1000) < 0) {
+		error("Can't read local name on hci%d: %s (%d)\n",
+						index, strerror(errno), errno);
+		return;
+	}
+
+	name[248] = '\0';
+
+	if (!g_utf8_validate(name, -1, NULL)) {
+		char *utf8_name;
+
+		utf8_name = g_convert(name, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL);
+		if (utf8_name) {
+			debug("Fix invalid non-UTF-8 device name \"%s\" on hci%d to \"%s\"",
+							name, index, utf8_name);
+			if (hci_write_local_name(dd, utf8_name, 2000) < 0) {
+				error("Can't change local name on hci%d: %s (%d)\n",
+								index, strerror(errno), errno);
+				return;
+			}
+			g_free(utf8_name);
+		}
+	}
+
 	/* Set page timeout */
 	if ((main_opts.flags & (1 << HCID_SET_PAGETO))) {
 		write_page_timeout_cp cp;

Re: Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

[Stefan Seyfried]

On Wed, 05 May 2010 12:27:37 +0200
Fabian Greffrath <fabian@greffrath.com> wrote:

> Dear Bluez list,
> 
> I am sorry, I know this is the worst way to start getting in touch 
> with a software project, but I accidently attached the wrong patch to 
> my previous email. It's configure_device() that must be patched. 
> Please find a corrected patch attached (it has been written against 
> bluez 4.63 but applies perfectly against 4.64).

I have one questions about the code from a cursory look (hint:
sending the patch inline would help commenting on it)

* where does the 249 in "char name[249];" come from? Is it from the BT
  spec? Or from somewhere else? A comment in the code might help. (If this
  is a number from the spec that is used all over the same code file and
  explained elsewhere, this question is obviously moot)

Then it would probably good if you could send the patch against current
git (even if it still applies cleanly) and in a format that "git am" can
process directly. That makes it very easy for the maintainers to apply the
code and in the same run makes sure you get proper attribution for your
contribution ;)

Have fun,

	Stefan
-- 
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."

Re: Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

[Fabian Greffrath]

Am 05.05.2010 17:02, schrieb Stefan Seyfried:
> I have one questions about the code from a cursory look (hint:
> sending the patch inline would help commenting on it)

Alright, I'll know for the next time.

> * where does the 249 in "char name[249];" come from? Is it from the BT
>    spec? Or from somewhere else? A comment in the code might help. (If this
>    is a number from the spec that is used all over the same code file and
>    explained elsewhere, this question is obviously moot)

I took this part of the patch over from tools/hciconfig.c:433 and 
indeed I think the fact that a device name may be up to 248 characters 
long is part of the BT spec: 
<http://www.palowireless.com/infotooth/tutorial/k1_gap.asp#Bluetooth%20Parameter%20Representation>

> Then it would probably good if you could send the patch against current
> git (even if it still applies cleanly) and in a format that "git am" can
> process directly. That makes it very easy for the maintainers to apply the
> code and in the same run makes sure you get proper attribution for your
> contribution ;)

Thanks again. I'd like to get some more feedback for the current patch 
and will then repost it in the desired format.

BTW, I'll be on vacation from May 7th to 14th, so please excuse if I 
reply with some days delay.

Re: Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

[Fabian Greffrath]

Hi bluez,

Am Mittwoch, den 05.05.2010, 17:02 +0200 schrieb Stefan Seyfried:
> Then it would probably good if you could send the patch against current
> git (even if it still applies cleanly) and in a format that "git am" can
> process directly. That makes it very easy for the maintainers to apply the
> code and in the same run makes sure you get proper attribution for your
> contribution ;)

I have reapplied my patch against current git, I have replaced the
obscure 249 in "char name[249];" by MAX_NAME_LENGTH as defined in
src/adapter.h and I am now posting it inline. Happy reviewing! ;)


>From 5447e55aeeda8a2307de0b7765abd8883c5f16a9 Mon Sep 17 00:00:00 2001
From: Fabian Greffrath <fabian+bluez@greffrath.com>
Date: Thu, 6 May 2010 16:37:43 +0200
Subject: [PATCH] Instantly fix non-UTF-8 local device names during device configuration phase

---
 plugins/hciops.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/plugins/hciops.c b/plugins/hciops.c
index 05c3d4e..836c291 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -81,6 +81,7 @@ static void configure_device(int index)
 	struct hci_dev_info di;
 	uint16_t policy;
 	int dd, err;
+	char name[MAX_NAME_LENGTH + 1];
 
 	if (hci_devinfo(index, &di) < 0)
 		return;
@@ -96,6 +97,30 @@ static void configure_device(int index)
 		return;
 	}
 
+	if (hci_read_local_name(dd, sizeof(name), name, 1000) < 0) {
+		error("Can't read local name on hci%d: %s (%d)\n",
+						index, strerror(errno), errno);
+		return;
+	}
+
+	name[MAX_NAME_LENGTH] = '\0';
+
+	if (!g_utf8_validate(name, -1, NULL)) {
+		char *utf8_name;
+
+		utf8_name = g_convert(name, -1, "UTF-8", "ISO-8859-1", NULL, NULL, NULL);
+		if (utf8_name) {
+			debug("Fix invalid non-UTF-8 device name \"%s\" on hci%d to \"%s\"",
+							name, index, utf8_name);
+			if (hci_write_local_name(dd, utf8_name, 2000) < 0) {
+				error("Can't change local name on hci%d: %s (%d)\n",
+								index, strerror(errno), errno);
+				return;
+			}
+			g_free(utf8_name);
+		}
+	}
+
 	/* Set page timeout */
 	if ((main_opts.flags & (1 << HCID_SET_PAGETO))) {
 		write_page_timeout_cp cp;
-- 
1.7.1

Re: Detect invalid (i.e. non-UTF-8) device names and fix them during initialization phase

[Johan Hedberg]

Hi,

On Thu, May 06, 2010, Fabian Greffrath wrote:
> Am Mittwoch, den 05.05.2010, 17:02 +0200 schrieb Stefan Seyfried:
> > Then it would probably good if you could send the patch against current
> > git (even if it still applies cleanly) and in a format that "git am" can
> > process directly. That makes it very easy for the maintainers to apply the
> > code and in the same run makes sure you get proper attribution for your
> > contribution ;)
> 
> I have reapplied my patch against current git, I have replaced the
> obscure 249 in "char name[249];" by MAX_NAME_LENGTH as defined in
> src/adapter.h and I am now posting it inline. Happy reviewing! ;)

BlueZ will (or at least should) set the name for the adapter as follows:

1. If there's a name in /var/lib/bluetooth/... use that
2. Else if there's a name in main.conf use that
3. If all else fails set the name to "BlueZ"

So I fail to see why this patch is needed at all. It sounds like there's
something else wrong in the initialization process which makes the
initialzation fail if the adapter contains some invalid default name (we
shouldn't as far as I see be trying to read the name at all from the
adapter before we've written it ourselves from the host side). I.e. I
suspect the patch might be just working around the real issue instead of
fixing it.

Johan

Re: [PATCH] Instantly fix non-UTF-8 local device names during device configuration phase

[Fabian Greffrath]

Am 06.05.2010 17:03, schrieb Johan Hedberg:
> BlueZ will (or at least should) set the name for the adapter as follows:
>
> 1. If there's a name in /var/lib/bluetooth/... use that
> 2. Else if there's a name in main.conf use that
> 3. If all else fails set the name to "BlueZ"

Hm, IIRC it was set to $(hostname)-$(hci-id) last time I established a 
connection.

> So I fail to see why this patch is needed at all. It sounds like there's
> something else wrong in the initialization process which makes the
> initialzation fail if the adapter contains some invalid default name (we
> shouldn't as far as I see be trying to read the name at all from the
> adapter before we've written it ourselves from the host side). I.e. I
> suspect the patch might be just working around the real issue instead of
> fixing it.

Yes, obviously Bluez fails to set a reasonable device name if the 
initial device name isn't already valid. However, I believe Bluez 
shouldn't necessarily be expected to handle invalid device names, as 
they clearly violate the specs. Instead, if Bluez detects an invalid 
name, it should convert it into a valid name as soon in the 
initialization process as possible. This sounds just naturally to me 
and is exactly what my patch does.

Cheers,
Fabian