Closed Bug 835474 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox OS Graveyard :: General, defect)

ARM
Gonk (Firefox OS)
defect
Not set
critical

Tracking

(blocking-b2g:tef+, firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 fixed, b2g18-v1.0.1 fixed)

RESOLVED FIXED
B2G C4 (2jan on)
blocking-b2g tef+
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed
b2g18-v1.0.1 --- fixed

People

(Reporter: m1, Assigned: echou)

References

Details

(Keywords: crash, Whiteboard: [b2g-crash][BTG-1070] QARegressExclude)

Crash Data

Attachments

(3 files, 1 obsolete file)

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
Severity: normal → critical
Crash Signature: [@ strlen | nsACString_internal::Assign ]
Keywords: crash
Whiteboard: [BTG-1070] → [b2g-crash][BTG-1070]
Assignee: nobody → 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.
Hi Greg,

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

Eric
Flags: needinfo?(mvines)
blocking-b2g: tef? → tef+
(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?
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.
Flags: needinfo?(mvines)
(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.
* 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.
Attachment #707497 - Attachment is obsolete: true
Attachment #707970 - Flags: review?(gyeh)
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 :)
Attachment #707970 - Flags: review?(gyeh) → review+
Attached patch patch for b2g18Splinter Review
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.
https://hg.mozilla.org/mozilla-central/rev/f8cdfa25ad9f
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
does not make sense to create a regression issue.
Flags: in-moztrap-
Whiteboard: [b2g-crash][BTG-1070] → [b2g-crash][BTG-1070] QARegressExclude
Cannot verify, need steps to blackbox test this issue.
You need to log in before you can comment on or make changes to this bug.