[bionic]blutoothd segfault when you cancel the keyboard pairing during the dialog for pairing code

[Alex Tu]

[Impact]

This patch is for this issue:
steps:
1. pair bluetooth keyboard
2. see the dialog asking user input the code for pairing.
3. press "esc" to cancel it.
4. blutoothd segfault shows in dmesg after a while.
5. Bluetooth shows off on setting UI of right top corner. dmesg shows: [ 978.138593] bluetoothd[1569]: segfault at 0 ip 000055564abe0a06 sp 00007ffe4bec6410 error 4 in bluetoothd[55564ab77000+f3000]

[Test Case]

 1. pair bluetooth keyboard
 2. see the dialog asking user input the code for pairing.
 3. press "esc" to cancel it.
 4. the bluetooth should still work to pair another bluetooth device.

[Regression Potential]

 * This patch workaround the case that a queue node was created but not yet assigned function before user input pairing keycode. If the user cancel the paring before inputting pairing keycode then assign the function pointer a dummy 'direct_match'.

* Bluetoothd responses to Bluetooth functions and "queue" is a shared common data structure, so in case of regression happens then blutoothd systemd service would be crashed.

 * We can verify this by operating add/remove BT devices to trigger queue operations.

 * I verified on target machine BIOS ID:0983 on BT mouse, keyboard, headset on pairing, remove and functionality checking.

[Other Info]

 * NO.

[Alex Tu (alextu)]

after this patch, the issue can not be reproduced:
commit 24249e091ab3a935f6ea87b7f7355c689c045a80 (HEAD -> refs/heads/master)
Author: Luiz Augusto von Dentz <email address hidden>
Date: Mon Apr 9 14:48:41 2018 +0300

    shared/queue: Handle NULL as direct match on queue_remove_if

    As with queue_find when function is set to NULL use direct_match as
    callback.

diff --git a/src/shared/queue.c b/src/shared/queue.c
index 5ddb8326d..60df11143 100644
--- a/src/shared/queue.c
+++ b/src/shared/queue.c
@@ -280,9 +280,12 @@ void *queue_remove_if(struct queue *queue, queue_match_func_t function,
 {
        struct queue_entry *entry, *prev = NULL;

- if (!queue || !function)
+ if (!queue)
                return NULL;

+ if (!function)
+ function = direct_match;
+
        entry = queue->head;

[Daniel van Vugt (vanvugt)]

It's good you know the fix already. Otherwise you might need this workaround to get a core dump:
https://bugs.launchpad.net/ubuntu/+source/apport/+bug/1870060/comments/3

[Sebastien Bacher (seb128)]

Is the issue fixed in the current version? Do you plan to work on a SRU and targetting what serie?

[Alex Tu (alextu)]

this is the debdiff which applied the patch , and I also host the modified code there : https://code.launchpad.net/~alextu/ubuntu/+source/bluez/+git/bluez/+ref/lp1887910-bluetoothd-segv-keyboard

[Sebastien Bacher (seb128)]

@Alex, thanks, a few comments:

* could you make the bug SRU compliant (adding a description of the impact and the regression potential)

* it would be nice to have an user friendly description of what the patch is solving in the changelog, also the email you used is not a valid one

* could you include details on what upstream version / ubuntu serie already include the fix?

[Alex Tu (alextu)]

@seb128, yes, I modified the description and the debdiff is there in #4.

[Sebastien Bacher (seb128)]

@Alex, sorry I though you were familiar with the SRU process, see https://wiki.ubuntu.com/StableReleaseUpdates for details

especially

'A [Regression Potential] section with a discussion of how regressions are most likely to manifest, or may manifest even if it is unlikely, as a result of this change.'

'low, becuase' isn't really enough...

Also I saw the debdiff, did you read my points about the description and email used in that one?

[Daniel van Vugt (vanvugt)]

I'm going to remove Ubuntu Sponsors from this briefly, until the following are fixed:

1. The issue mentioned in comment #7 - The Bug Description on this page needs improving.

2. The email address used in the patch <u@SoundWire> needs replacing with your real email.

3. Need to mention the bug ID and the commit ID where the patch came from, in the patch.

[Daniel van Vugt (vanvugt)]

Oh, I can't remove Ubuntu Sponsors. Still, please fix all of the above.

[Daniel van Vugt (vanvugt)]

BTW the fix first appeared in 5.50 (commit f1b8cdef95d6b51cab122f2be0694521f47092d0).

[Alex Tu (alextu)]

Sorry,
the debugging and debdiff was done on the target machine so that the email ID was missed.

This attached new debdiff also included the patch and lp number information in the changelog.

And also updated the truncated description.

[Sebastien Bacher (seb128)]

Alex, did you read comment 7? 'low' isn't responding to what the regression potential should provide, it's there to describe what impact the change could have to know on what to focus the testing

[Alex Tu (alextu)]

sorry for bad habit that I miss-understanded before.
I revised it and put detail information there.

[Alex Tu (alextu)]

It's seems not just one patch can fix this issue. I found another corner case that still can trigger a blutoothd segmentation fault.

I pair one Bluetooth keyboard but cancel before input pairing keycode. Before timeout of latest keyboard pairing, I pair another Bluetooth keyboard and still cancel it before input pairing keycode.

Please hold this SRU process and I'm debugging for that corner case.

[Sebastien Bacher (seb128)]

Unsubscribing sponsors for now, Alex are you still working on that? Please subscribe the team back if you get something ready for upload

[Sebastien Bacher (seb128)]

Going to wontfix for bionic since there doesn't seem to be a need for it at this point