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

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::SMS, defect)

Description

[Paul Yang [:pyang] (away)]

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

Comment 1

[Paul Yang [:pyang] (away)]

attach file to bugzilla failed

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

Comment 2

[Scoobidiver (away)]

Please provide a crash ID (see https://wiki.mozilla.org/B2G/QA/Tips_And_Tricks#Getting_crashes_off_the_Device).

Comment 3

[Paul Yang [:pyang] (away)]

Attachment: adb log

Provide adb log in attachment

Comment 4

[Borja Salguero [:borjasalguero]]

Could you check if this is this related with 875408?? Thanks!

Comment 5

[Chia-hung Tai [:ctai :ctai_mozilla :cht]]

Broja, 
I think it is unrelated. This bug is for the file which is zero size.

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

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.

Comment 7

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: returning empty object instead of null when decoding mms attachment

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: 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

Comment 9

[Vicamo Yang [:vicamo][:vyang]]

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.

Comment 10

[Wayne Chang [:wchang]]

Triage - as easily reproduced this could be a potential attack on all firefox phones. Blocking.

Comment 11

[Gene Lian (I already quit Mozilla)]

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!

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(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.

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/projects/birch/rev/9a3ee53f3421

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Backed out for no reviewer on patch comment. https://hg.mozilla.org/projects/birch/rev/0664d639c478

Comment 15

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/projects/birch/rev/1b659b9958cb

Comment 16

[Ed Morley [:emorley]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/8be1c897c934

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/8be1c897c934

Comment 20

[Paul Yang [:pyang] (away)]

Verified by pvt build 20130715230201
Issue resolved without crash