835474 - crash in mozilla::dom::bluetooth::BluetoothHfpManager::ReceiveSocketData

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Michael Vines [:m1] [:evilmachines]]

Crash seen during stability test on AU 185.

Test Steps:
1. Run of Reboot script continuously with a connection of WIFI and BT Car kit.


Top frames
----------
Crash reason:  SIGSEGV
Crash address: 0x4a100000

Thread 0 (crashed)
 0  libc.so!strlen [strlen.c : 62 + 0x5c]
     r4 = 0x4a0ffff0    r5 = 0xffffffff    r6 = 0xbe945628    r7 = 0x0000000a
     r8 = 0xbe94575f    r9 = 0x40307c0c   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbe945448    lr = 0x4118ef6f    pc = 0x40078b4c
    Found by: given as instruction pointer in context
 1  libxul.so!nsACString_internal::Assign [nsCharTraits.h : 590 + 0x5]
     r4 = 0x4a0ffff0    r5 = 0xffffffff    r6 = 0xbe945628    r7 = 0x0000000a
     r8 = 0xbe94575f    r9 = 0x40307c0c   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbe945450    pc = 0x4118ef6f
    Found by: call frame info
 2  libxul.so!nsACString_internal::Assign [nsTSubstring.cpp : 290 + 0x5]
     r4 = 0x488fa0b0    r5 = 0x00000000    r6 = 0xbe945628    r7 = 0x0000000a
     r8 = 0xbe94575f    r9 = 0x40307c0c   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbe945478    pc = 0x4118efe5
    Found by: call frame info
 3  libxul.so!mozilla::dom::bluetooth::BluetoothHfpManager::ReceiveSocketData [nsTString.h : 463 + 0x3]
     r4 = 0x488fa0b0    r5 = 0x00000000    r6 = 0xbe945628    r7 = 0x0000000a
     r8 = 0xbe94575f    r9 = 0x40307c0c   r10 = 0x00000000    fp = 0x00000000
     sp = 0xbe945490    pc = 0x40defe37
    Found by: call frame info

Comment 1

[Michael Vines [:m1] [:evilmachines]]

Attachment: decoded minidump of crash

Comment 2

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

According to the minidump, this may be caused by the following line in BluetoothHfpManager::ReceiveSocketData():

  nsAutoCString msg((const char*)aMessage->mData.get());

This would be dangerous if aMessage->mData is not terminated by '\0'. If that happens, then it may be possible getting into crash when trying to get the string length in ctor of nsAutoCString.

I don't know exactly when would this case happen, but it seems to be possible (and looks like it /did/ happen). I'll provide a patch asap.

Comment 3

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

Attachment: patch 1: v1: assign length of the message with aMessage->mSize

Hi Greg,

Please apply this patch to b2g18 branch and see if this would still happen. Thank you.

Eric

Comment 4

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

(In reply to Eric Chou [:ericchou] [:echou] from comment #3)
> Created attachment 707497 [details] [diff] [review]
> patch 1: v1: assign length of the message with aMessage->mSize
> 
> Hi Greg,
> 

Sorry, I thought this was filed by Greg.

Michael, could you help with this?

Comment 5

[Michael Vines [:m1] [:evilmachines]]

Patch makes sense to me, although are you confident that stripping the CR won't cause a regression? 

Suggest we just land this so it'll get picked up in the next stability run in a couple days or so.   If it's seen again we can reopen the bug.

Comment 6

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

(In reply to Michael Vines [:m1] [:evilmachines] from comment #5)
> Patch makes sense to me, although are you confident that stripping the CR
> won't cause a regression? 
>

Yes, it should be fine.

> Suggest we just land this so it'll get picked up in the next stability run
> in a couple days or so.   If it's seen again we can reopen the bug.

Sure. Thanks.

Comment 7

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

Attachment: patch 1: v2: assign length of the message with aMessage->mSize

* We don't need to strip '\r' by ourselves because msg.StripWhitespaces() has done that. The only thing we need to do is making sure the string length is right.

Comment 8

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 707970 [details] [diff] [review]
patch 1: v2: assign length of the message with aMessage->mSize

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

Thanks for spending time on this, r=me :)

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f8cdfa25ad9f

Comment 10

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

Attachment: patch for b2g18

Due to this is a tef+ bug, the patch should be checked into b2g18. However this cannot be directly applied or there will be a conflict. Therefore, please use this patch for checking into b2g18. The logic is totally the same as the reviewed patch.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f8cdfa25ad9f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ed7302e4801c
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_0/rev/361d9359f4f3

Comment 13

[amiller]

does not make sense to create a regression issue.

Comment 14

[Darren]

Cannot verify, need steps to blackbox test this issue.