814761 - B2G SMS: Exception in processParcel() at receiving multipart SMS messages (>= 379 chars) sent from emulator console

Status: RESOLVED FIXED

Description

[Robert Wood [:rwood]]

On the b2g device emulator:

Simulate an incoming SMS (via emulator's 'sms send' command) and make the SMS text length to be 379 characters or greater.  The first time, the incoming message is received fine.  Now repeat the same thing, use the emulator command and simulate a second incoming SMS of 379 or greater - notice this time the SMS is not received. The emulator callback works / the emulator returns 'OK' after issuing the 'sms send' command but the 'sms.onreceived' event is never fired.

Having two incoming SMS messages of 378 characters or less works fine, but two incoming messages of 379 characters or above will reproduce the problem on the second message.  Also note that simulating an incoming SMS with a larger number of characters (i.e. 1600) works fine on the first message.

Reproduce by running the attached marionette WebSMS script.  Note that it can also be reproduced manually via telnet to the emulator, so it is not a marionette issue.

Comment 1

[Robert Wood [:rwood]]

Attachment: Marionette WebSMS test script

Comment 2

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

Attachment: Exception in ril_worker, function processParcel

I found ril_worker throws an exception at receiving the third segment. `TypeError: options is undefined in line 589`.

 579   processParcel: function processParcel() {
 ...
 587       options = this.tokenRequestMap[token];
 588       delete this.tokenRequestMap[token];
 589       request_type = options.rilRequestType;

Comment 3

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

It's funny that I got my own tests, too.

addr_1: 5551110000
addr_2: 5554
body_1: "a" * 380
body_2: "1" * 10 + "2" * 10 + ... + "0" * 10

Only addr_2 with body_1 triggers this issue.

Comment 4

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #2)
> Created attachment 685538 [details]
> I/Gecko   (   42): RIL Worker: New incoming parcel of size 656
> I/Gecko   (   42): RIL Worker: Read 360, but parcel size is 656. Going to sleep.
> I/Gecko   (   42): RIL Worker: Received 668 bytes.
> I/Gecko   (   42): RIL Worker: Already read 360
> I/Gecko   (   42): RIL Worker: Parcel (size 656): 0,0,0,0,235,3,0,0,64,1,0,0,48,0, ....

Something wrong in emulator or hardware/ril. Incoming SMS should be a unsolicited request, so we should have had "1,0,0,0,235,3,0,0,..." here.

> I/Gecko   (   42): RIL Worker: We have at least one complete parcel.
> I/Gecko   (   42): RIL Worker: Parcel handling threw TypeError: options is undefined
> I/Gecko   (   42): processParcel@resource://gre/modules/ril_worker.js:589

And parcel handler should not failed here for a unmatched solicited request as well.

Comment 5

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

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #3)
> It's funny that I got my own tests, too. Only addr_2 with body_1 triggers this issue.

Actually other combinations also cause wrong parcel data read, but their request_type is some arbitrary number and is therefore recognized as an unsolicited request.

Comment 6

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

When we try to grow incoming buffer in function writeToIncoming(), the minimum size is currently set to incoming.length. It's incorrect and should be set to (incoming.length + this.readIncoming) so that we can prevent previous read but not consumed yet data being overwritten.

Comment 7

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

Attachment: Part 1: fix ril worker exception due to buffer overflow

Yoshi, would you like to review this patch?

Comment 8

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

Attachment: Part 2: add test case

Hi Rob,

This patch is based on you attachment #684763 [details] with a small fix for pending emulator response. I still list you as the author.

Comment 9

[Robert Wood [:rwood]]

Comment on attachment 688242 [details] [diff] [review]
Part 2: add test case

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

Looks good, thanks Vicamo.

Comment 10

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

Attachment: Part 1: fix ril worker exception due to buffer overwritten: v2

Add r=yoshi. Already r+.

Comment 11

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

Attachment: Part 2: add test cases

Hi Rob, I add xpcshell test case as well because the marionette test depends on SMS functions and this issue should be a generic Buf operation bug. Please help reviewing this patch again. Thank you :)

Comment 12

[Robert Wood [:rwood]]

Comment on attachment 688650 [details] [diff] [review]
Part 2: add test cases

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

Hi Vicamo, I have never written an xpcshell test and am not familiar with them at all, so I am not qualified to review that particular test. I am unsure whom to suggest instead, maybe one of the B2G peers?  https://wiki.mozilla.org/Modules/Boot2Gecko

Comment 13

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

Comment on attachment 688650 [details] [diff] [review]
Part 2: add test cases

Hi Yoshi, would you like to review the xpcshell test cases?

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh]]

Comment on attachment 688650 [details] [diff] [review]
Part 2: add test cases

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

LGTM,
Can you explain and add comment for the +4 in parcel size?

Thanks

::: dom/system/gonk/tests/test_ril_worker_buf.js
@@ +111,5 @@
> +
> +  // Prepare two parcels, whose size are both smaller than the incoming buffer
> +  // size but larger when combined, to trigger the bug.
> +  let pA_dataLength = buf.INCOMING_BUFFER_LENGTH / 2;
> +  let pA_parcelSize = pA_dataLength + worker.PARCEL_SIZE_SIZE + 4;

Could you explain why +4 here?

@@ +117,5 @@
> +                             worker.RESPONSE_TYPE_UNSOLICITED,
> +                             request,
> +                             calloc(pA_dataLength, 1));
> +
> +  let pB_dataLength = buf.INCOMING_BUFFER_LENGTH *3 / 4;

nit, space

Comment 15

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

Attachment: Part 2: add test cases : v2

Address review comment #14. Actually, the expression for parcel size is semantically wrong. They should have been what they look like now.

Comment 16

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/4130f20282de
http://hg.mozilla.org/integration/mozilla-inbound/rev/957e9d5fe527

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4130f20282de
https://hg.mozilla.org/mozilla-central/rev/957e9d5fe527

Comment 18

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

Comment on attachment 688647 [details] [diff] [review]
Part 1: fix ril worker exception due to buffer overwritten: v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 699256
User impact if declined: might not receive multipart SMS message correctly
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?rev=00e26dece6ea
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 19

[Alex Keybl [:akeybl]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> Risk to taking this patch (and alternatives if risky): 

To be able to make a call on this non-b-b+ bug, we need a risk evaluation.

Comment 20

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

(In reply to Alex Keybl [:akeybl] from comment #19)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> > Risk to taking this patch (and alternatives if risky): 
> 
> To be able to make a call on this non-b-b+ bug, we need a
> risk evaluation.

This is actually a bug that had existed since very early RIL implementation. It's not some new feature, workaround or the like. Without this, transactions of two huge, successional parcels may fail with exceptions shown comment #2. The only known victim is SMS for now.

IMHO, there is no risk approving uplift of this patch.

Comment 21

[Alex Keybl [:akeybl]]

Comment on attachment 688647 [details] [diff] [review]
Part 1: fix ril worker exception due to buffer overwritten: v2

Low or no risk, and impact automated testing. Approving for branches.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fbaa50688290
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0dd7c6613c8

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3dc7f40480de
https://hg.mozilla.org/releases/mozilla-b2g18/rev/e09d309b5145