If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[MMS] Receive zero size file will crash when broadcast the dom message.

VERIFIED FIXED in Firefox 24, Firefox OS v1.1hd

Status

Firefox OS
Gaia::SMS
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: pyang, Assigned: kk1fff)

Tracking

(Blocks: 1 bug, {crash, stackwanted})

unspecified
1.1 QE3 (26jun)
ARM
Gonk (Firefox OS)
crash, stackwanted

Firefox Tracking Flags

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

Details

(Whiteboard: [b2g-crash] leorun3)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
build: v1-train pvt build 2013061007
device: Leo

Prerequisite:
Use attached file, sent MMS with an android or other phone.
Auto-retrieve enabled (not required)

REPRODUCE STEP:
1. Send MMS with attachment (a unsupported file)
2. Click notification

EXPECT:
An unsupported preview icon is shown

ACTUAL:
B2G crashed while entering thread view
(Reporter)

Comment 1

4 years ago
attach file to bugzilla failed

The attachment is a 0 Byte file, its name is .nomedia

Comment 2

4 years ago
Please provide a crash ID (see https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Getting_crashes_off_the_Device).
Severity: normal → critical
Keywords: crash, stackwanted
Whiteboard: [b2g-crash]
(Reporter)

Comment 3

4 years ago
Created attachment 763429 [details]
adb log

Provide adb log in attachment
Could you check if this is this related with 875408?? Thanks!
Flags: needinfo?(pyang)
Summary: [MMS] Receive unsupported file and crash when entering thread view → [MMS] Receive zero size file will crash when broadcast the dom message.
Broja, 
I think it is unrelated. This bug is for the file which is zero size.
Flags: needinfo?(pyang)
When receiving a zero length file, content of the attachment structure will be null (instead of a zero length blob). This crashes main process when trying to form an MmsMessageData for an IPC operation. We should store a zero length blob for zero length attachment instead of using null pointer.
Assignee: nobody → pwang
Created attachment 764029 [details] [diff] [review]
Patch: returning empty object instead of null when decoding mms attachment
Attachment #764029 - Flags: review?(gene.lian)
Blocks: 744684
blocking-b2g: --- → leo?
Created attachment 764078 [details] [diff] [review]
Patch: returning empty array instead of null when decoding mms attachment

We should return an empty array instead of an empty object. try: https://tbpl.mozilla.org/?tree=Try&rev=6e90a0f7c8e3
Attachment #764029 - Attachment is obsolete: true
Attachment #764029 - Flags: review?(gene.lian)
(Assignee)

Updated

4 years ago
Attachment #764078 - Flags: review?(gene.lian)
Comment on attachment 764078 [details] [diff] [review]
Patch: returning empty array instead of null when decoding mms attachment

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

::: dom/mobilemessage/src/ril/WspPduHelper.jsm
@@ -2291,4 @@
>  
>          let octetArray = Octet.decodeMultiple(data, contentEnd);
>          let content = null;
> -        if (octetArray) {

We can still have null octetArray here.  Please keep it until we have better error handling for these broken blobs.
Triage - as easily reproduced this could be a potential attack on all firefox phones. Blocking.
blocking-b2g: leo? → leo+
Comment on attachment 764078 [details] [diff] [review]
Patch: returning empty array instead of null when decoding mms attachment

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

Looks good!
Attachment #764078 - Flags: review?(gene.lian) → review+
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #9)
> We can still have null octetArray here.  Please keep it until we have better
> error handling for these broken blobs.

After some more discuss, |Octet.decodeMultiple| can no longer return null after this change, so checking nullity of its return value is meaningless.

I concerned about a conformance requirement saying that we should do our best effort to handle broken PDU and return as much information as possible.  Even so, we should catch the possible exception here and create an empty blob instead.  But that's off the topic here.
https://hg.mozilla.org/projects/birch/rev/9a3ee53f3421
Backed out for no reviewer on patch comment. https://hg.mozilla.org/projects/birch/rev/0664d639c478
https://hg.mozilla.org/projects/birch/rev/1b659b9958cb
https://hg.mozilla.org/mozilla-central/rev/1b659b9958cb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
status-firefox24: --- → fixed

Updated

4 years ago
Whiteboard: [b2g-crash] → [b2g-crash] leorun3
https://hg.mozilla.org/releases/mozilla-b2g18/rev/8be1c897c934
status-b2g18: --- → fixed
status-b2g18-v1.0.0: --- → wontfix
status-b2g18-v1.0.1: --- → wontfix
status-b2g-v1.1hd: --- → affected
status-firefox22: --- → wontfix
status-firefox23: --- → wontfix
Target Milestone: --- → 1.1 QE3 (24jun)
Duplicate of this bug: 885089
https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8be1c897c934
status-b2g-v1.1hd: affected → fixed
(Reporter)

Comment 20

4 years ago
Verified by pvt build 20130715230201
Issue resolved without crash
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.