836715 - [Bluetooth] Unpairing failed with SIGSEGV

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: logcat

I was trying to reproduce bug 836523 with the revs mentioned in that report. After I turned off Bluetooth during a file transfer fro my PC, I unpaired, paired and unpaired my phone with the PC. The second unpairing failed with a segmentation fault. Stack, register and thread infos are shown below. From the logcat, I'd say that pairing didn't succeed in the first place.

*****

tdz@linux-6f0r:~/Projects/mozilla/src/B2G-unagi> ./run-gdb.sh attach 109
..killing gdbserver pid 447
Terminated 
Attached; pid = 109
Listening on port 11109
prebuilt/linux-x86/toolchain/arm-linux-androideabi-4.4.x/bin/arm-linux-androideabi-gdb -x /tmp/b2g.gdbinit.tdz /home/tdz/Projects/mozilla/src/B2G-unagi/objdir-gecko/dist/bin/b2g
GNU gdb (GDB) 7.1-android-gg2
Copyright (C) 2010 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "--host=i686-linux-gnu --target=arm-elf-linux".
For bug reporting instructions, please see:
<http://www.gnu.org/software/gdb/bugs/>...
Really redefine built-in command "frame"? (y or n) [answered Y; input not from terminal]
Really redefine built-in command "thread"? (y or n) [answered Y; input not from terminal]
Really redefine built-in command "start"? (y or n) [answered Y; input not from terminal]
Reading symbols from /home/tdz/Projects/mozilla/src/B2G-unagi/objdir-gecko/dist/bin/b2g...done.
Remote debugging from host 127.0.0.1
warning: .dynamic section for "/home/tdz/Projects/mozilla/src/B2G-unagi/objdir-gecko/dist/bin/libxul.so" is not at the expected address (wrong library or version mismatch?)
_______________________________________________________________________________
Error while running hook_stop:
Value can't be converted to integer.
syscall () at bionic/libc/arch-arm/bionic/syscall.S:50
50	    ldmfd   sp!, {r4, r5, r6, r7}
gdb> c
[New Thread 109.546]

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 109.546]
_______________________________________________________________________________
Error while running hook_stop:
Value can't be converted to integer.
link_before (list=0x443a775c, before_this_link=0x0, link=0x0) at external/dbus/dbus/dbus-list.c:113
113	      link->prev = link;
gdb> bt
#0  link_before (list=0x443a775c, before_this_link=0x0, link=0x0) at external/dbus/dbus/dbus-list.c:113
#1  0x41f4db76 in _dbus_list_prepend_link (list=<value optimized out>, link=0x0) at external/dbus/dbus/dbus-list.c:313
#2  0x41f4db80 in _dbus_list_append_link (list=0x443a775c, link=0x0) at external/dbus/dbus/dbus-list.c:295
#3  0x41f48036 in _dbus_connection_queue_synthesized_message_link (connection=0x443a7740, link=0x0) at external/dbus/dbus/dbus-connection.c:563
#4  0x41f481ce in notify_disconnected_and_dispatch_complete_unlocked (connection=0x443a7740) at external/dbus/dbus/dbus-connection.c:4239
#5  _dbus_connection_get_dispatch_status_unlocked (connection=0x443a7740) at external/dbus/dbus/dbus-connection.c:4285
#6  0x41f4987c in _dbus_connection_block_pending_call (pending=0x48fd80d0) at external/dbus/dbus/dbus-connection.c:2456
#7  0x41f5a0f4 in dbus_pending_call_block (pending=0x48fd80d0) at external/dbus/dbus/dbus-pending-call.c:685
#8  0x41f4a03a in dbus_connection_send_with_reply_and_block (connection=<value optimized out>, message=0x47ffa790, timeout_milliseconds=<value optimized out>, error=0x47a340c0) at external/dbus/dbus/dbus-connection.c:3548
#9  0x4116cbbe in mozilla::ipc::dbus_func_args_timeout_valist (conn=0x443a7740, timeout_ms=0xffffffff, err=0x47a340c0, path=<value optimized out>, ifc=0x416563d2 "org.bluez.Adapter", func=0x416563e4 "RemoveDevice", first_arg_type=0x6f, args=...) at /home/tdz/Projects/mozilla/src/B2G-unagi/gecko/ipc/dbus/DBusUtils.cpp:200
#10 0x4116cc52 in mozilla::ipc::dbus_func_args (conn=0x41f79a70, path=0x477ffdf8 "/org/bluez/566/hci0", ifc=<value optimized out>, func=<value optimized out>, first_arg_type=0x6f) at /home/tdz/Projects/mozilla/src/B2G-unagi/gecko/ipc/dbus/DBusUtils.cpp:242
#11 0x40e05766 in RemoveDeviceTask::Run (this=0x47b90020) at /home/tdz/Projects/mozilla/src/B2G-unagi/gecko/dom/bluetooth/linux/BluetoothDBusService.cpp:182
#12 0x41190dda in nsThread::ProcessNextEvent (this=0x48bde880, mayWait=0x0, result=0x477ffeb7) at /home/tdz/Projects/mozilla/src/B2G-unagi/gecko/xpcom/threads/nsThread.cpp:560
#13 0x411711f6 in nsCancelableRunnable::QueryInterface (this=<value optimized out>, aIID=..., aInstancePtr=<value optimized out>) at /home/tdz/Projects/mozilla/src/B2G-unagi/objdir-gecko/xpcom/build/nsThreadUtils.cpp:39
#14 0x01000000 in ?? ()
#15 0x01000000 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
gdb> info registers
r0             0x443a775c	0x443a775c
r1             0x0	0x0
r2             0x0	0x0
r3             0x0	0x0
r4             0x443a775c	0x443a775c
r5             0x0	0x0
r6             0x41f6dc0c	0x41f6dc0c
r7             0x41f6dc3f	0x41f6dc3f
r8             0x41f76f80	0x41f76f80
r9             0x48fd80d0	0x48fd80d0
r10            0x41f775d8	0x41f775d8
r11            0x0	0x0
r12            0x19	0x19
sp             0x477ffc20	0x477ffc20
lr             0x41f4db77	0x41f4db77
pc             0x41f4db50	0x41f4db50 <link_before+4>
cpsr           0x60000030	0x60000030
gdb> info threads
[New Thread 109.230]
[New Thread 109.232]
[New Thread 109.233]
[New Thread 109.234]
[New Thread 109.235]
[New Thread 109.237]
[New Thread 109.238]
[New Thread 109.239]
[New Thread 109.240]
[New Thread 109.241]
[New Thread 109.242]
[New Thread 109.243]
[New Thread 109.244]
[New Thread 109.245]
[New Thread 109.246]
[New Thread 109.247]
[New Thread 109.248]
[New Thread 109.249]
[New Thread 109.250]
[New Thread 109.251]
[New Thread 109.252]
[New Thread 109.253]
[New Thread 109.254]
[New Thread 109.255]
[New Thread 109.256]
[New Thread 109.257]
[New Thread 109.264]
[New Thread 109.269]
[New Thread 109.308]
[New Thread 109.309]
[New Thread 109.331]
[New Thread 109.341]
[New Thread 109.380]
[New Thread 109.539]
[New Thread 109.567]
[New Thread 109.569]
  38 Thread 109.569  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:183
  37 Thread 109.567  poll () at bionic/libc/arch-arm/syscalls/poll.S:10
  36 Thread 109.539  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  35 Thread 109.380  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  34 Thread 109.341  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  33 Thread 109.331  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  32 Thread 109.309  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  31 Thread 109.308  __ioctl () at bionic/libc/arch-arm/syscalls/__ioctl.S:9
  30 Thread 109.269  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  29 Thread 109.264  poll () at bionic/libc/arch-arm/syscalls/poll.S:10
  28 Thread 109.257  syscall () at bionic/libc/arch-arm/bionic/syscall.S:50
  27 Thread 109.256  read () at bionic/libc/arch-arm/syscalls/read.S:9
  26 Thread 109.255  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  25 Thread 109.254  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  24 Thread 109.253  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  23 Thread 109.252  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  22 Thread 109.251  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  21 Thread 109.250  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  20 Thread 109.249  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  19 Thread 109.248  poll () at bionic/libc/arch-arm/syscalls/poll.S:10
  18 Thread 109.247  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  17 Thread 109.246  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  16 Thread 109.245  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  15 Thread 109.244  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  14 Thread 109.243  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:183
  13 Thread 109.242  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  12 Thread 109.241  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  11 Thread 109.240  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  10 Thread 109.239  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  9 Thread 109.238  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:183
  8 Thread 109.237  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  7 Thread 109.235  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  6 Thread 109.234  0xffff0520 in ?? ()
  5 Thread 109.233  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
  4 Thread 109.232  syscall () at bionic/libc/arch-arm/bionic/syscall.S:50
  3 Thread 109.230  __futex_syscall3 () at bionic/libc/arch-arm/bionic/atomics_arm.S:182
* 2 Thread 109.546  link_before (list=0x443a775c, before_this_link=0x0, link=0x0) at external/dbus/dbus/dbus-list.c:113
  1 Thread 109.109  syscall () at bionic/libc/arch-arm/bionic/syscall.S:50
gdb>

Comment 1

[Andrew Overholt [:overholt]]

Ben/Blake:  could this be fixed by the recent changes you've made?

Comment 2

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: fix potential invalid memory usage

Unpair process would be done via RemoveDeviceTask. The problem was that, an argument |const char* aDeviceObjectPath| passed into this task was owned by a local variable |nsCString tempDeviceObjectPath| in BluetoothDBusService::RemoveDeviceInternal(). Therefore this argument may be used after it had been GC'd and cause unexpected errors.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 720603 [details] [diff] [review]
patch 1: v1: fix potential invalid memory usage

Review of attachment 720603 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the drive by but this patch won't work like you expect in all cases.

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +155,5 @@
>                     BluetoothReplyRunnable* aRunnable)
>      : mAdapterPath(aAdapterPath)
>      , mDeviceObjectPath(aDeviceObjectPath)
>      , mRunnable(aRunnable)
>    {

Rather than removing this can you instead assert that aDeviceObjectPath is not empty?

@@ +167,5 @@
>      BluetoothValue v = true;
>      nsString errorStr;
>  
> +    const char* tempDeviceObjectPath =
> +      NS_ConvertUTF16toUTF8(mDeviceObjectPath).get();

This isn't safe. The result of .get() is only guaranteed to be valid while the NS_ConvertUTF16toUTF8 is alive. Since you're creating a temporary here it is immediately being destructed.

@@ +190,4 @@
>  
>  private:
>    nsString mAdapterPath;
> +  nsString mDeviceObjectPath;

Let's make this a nsCString, then assign in the constructor from the nsAString argument with NS_ConvertUTF16toUTF8. That way the string will survive for the time you need.

Comment 4

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v2: fix potential invalid memory usage

* Modified RemoveDeviceTask according to Ben's review.

Comment 5

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v3: fix potential invalid memory usage

* Added length checking in ctor of RemoveDeviceTask

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 721034 [details] [diff] [review]
patch 1: v3: fix potential invalid memory usage

Review of attachment 721034 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bluetooth/linux/BluetoothDBusService.cpp
@@ +151,4 @@
>  class RemoveDeviceTask : public nsRunnable {
>  public:
>    RemoveDeviceTask(const nsAString& aAdapterPath,
> +                   const NS_ConvertUTF16toUTF8& aDeviceObjectPath,

AFter comment below, this should probably be an nsACString&

@@ +2221,5 @@
> +    new RemoveDeviceTask(aAdapterPath,
> +                         NS_ConvertUTF16toUTF8(
> +                           GetObjectPathFromAddress(aAdapterPath,
> +                                                    aDeviceAddress)),
> +                         aRunnable));

Ok, so, I think you may've fixed the wrong thing here. The original nsCString was ok, it was the fact that we were passing the .get() into RemoveDeviceTask that was a problem, since the const char* you get from the .get() scopes out at the end of the function. Passing the nsACString& reference then initializing the internal nsCString to the value of the nsACString& should copy things without the prior issues. 

I think. :)

Comment 7

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v4: fix potential invalid memory usage

Kyle, you're right. I misunderstood the idea of comment 3. The patch has been revised, please review again. Thanks.

Comment 8

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6e2c092fbc

Comment 9

[Eric Chou [:ericchou] [:echou]]

Nominate as tef+ because this could be also seen on b2g-18.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d6e2c092fbc

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8760606ff386
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c649f9873c0e

Comment 12

[amiller]

Does not make sense to create a regression issue.

Comment 13

[cbarker]

Requires PC with Bluetooth connection to Verify

Comment 14

[Walter Chen[:ypwalter][:wachen]]

Verified fixed in v1train and v101 of 2013/4/7 pvt build.