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

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.

@@ -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;
 

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

@@ -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;

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."

Am 05.05.2010 17:02, schrieb Stefan Seyfried:

Alright, I'll know for the next time.

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>

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.

Hi bluez,

Am Mittwoch, den 05.05.2010, 17:02 +0200 schrieb Stefan Seyfried:

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: Fabian Greffrath <[email protected]>
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(-)

index 05c3d4e..836c291 100644
@@ -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

Hi,

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

Am 06.05.2010 17:03, schrieb Johan Hedberg:

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

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