813042 - STK Profile Download makes Nexus S/Galaxy S2' RIL reset

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[:gerard-majax]

Attachment: Modem dump when resetting after profile download

STK Profile Download from bug 791934 makes the Nexus S modem rebooting, effectively breaking the main functionnality of a phone.

Commenting out the STK profile download in ril_worker.js worksaround the issue. Attached is the dump from the modem when it crashes and resets.

Comment 1

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

Hi Lissy
I just try my latest m-c on Nexus S and it's still crashing even I commented out the profile download code.

my last m-c commit is 

commit 40c9aec14b80a697dd3f2f368a5513d7d1b54a8d
Author: Alex Keybl <[e-mail]>
Date:   Mon Nov 19 12:03:27 2012 -0800

    Merging in version bump NO BUG


Can you show me your m-c version so I can give it a try ?

Thanks

Comment 2

[:gerard-majax]

$ git describe --always master
b373a99

And the change to ril_worker.js that makes me happy:
diff --git a/dom/system/gonk/ril_worker.js b/dom/system/gonk/ril_worker.js
index f14099b..314034b 100644
--- a/dom/system/gonk/ril_worker.js
+++ b/dom/system/gonk/ril_worker.js
@@ -3030,11 +3030,13 @@ let RIL = {
     if (newCardState == GECKO_CARDSTATE_READY) {
       // For type SIM, we need to check EF_phase first.
       // Other types of ICC we can send Terminal_Profile immediately.
+      /*
       if (this.appType == CARD_APPTYPE_SIM) {
         this.getICCPhase();
       } else {
         this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE);
       }
+      */
       this.fetchICCRecords();
       this.reportStkServiceIsRunning();
     }

Comment 3

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

sorry for my late reply
I tried to reproduce it but I can't.

I just try my latest gecko
commit f29aa5cb0067b628348d53098224f811e8aa7771
Author: Ehsan Akhgari <[e-mail]>
Date:   Wed Nov 28 17:13:43 2012 -0500

and gaia
commit 7c3bd603b482676da3214e2a54a697e5089932bc
Merge: 47823b5 2cc7345
Author: Timothy Guan-tin Chien <[e-mail]>
Date:   Wed Nov 28 18:51:37 2012 -0800

and the nexus won't reboot now.

Lissy, can you check again ?

Comment 4

[:gerard-majax]

I'll try as soon as I get the device back. Do not hesitate to ping me if I forget.

Comment 5

[Juan Gomez [:_AtilA_] (CET/CEST)]

I was experiencing the same issues on SGS2.
Workaround in Comment #2 worked for me.

Comment 6

[:gerard-majax]

Got my Nexus S back, flashed it with brand new code (completely wiped ccache, out/ and objdir-gecko), still reproducing.

Gecko is at :
$ git describe --always 
acf076f

The workaround "fix" I proposed earlier still works.

Comment 7

[:gerard-majax]

According to TS 101.127 section 5.2 [http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/ts_101267v081800p.pdf], encoding of the profile downloaded to SIM is composed of 19 bytes plus one subsequent byte at zero.

in ril_consts.js, this.STK_SUPPORTED_TERMINAL_PROFILE is defined with 20 bytes, and sendStkTerminalProfile() has the following code:

  sendStkTerminalProfile: function sendStkTerminalProfile(profile) {
    Buf.newParcel(REQUEST_STK_SET_PROFILE);
    Buf.writeUint32(profile.length * 2);
    for (let i = 0; i < profile.length; i++) {
      GsmPDUHelper.writeHexOctet(profile[i]);
    }    
    Buf.writeUint32(0);
    Buf.sendParcel();
  },

So we copy the 20 bytes then add another one. Which makes 21 bytes to me. Commenting the last "Buf.writeUint32(0)" fixes the issue.

Comment 8

[:gerard-majax]

Looks like "fixed the issue" means in this case "makes no profile being downloaded, therefore worksaround the bug".

Comment 9

[:gerard-majax]

I did some testing, re-enabling stk profile download and below is the result.

-----------------------------------------------------------------------------
Working diff :
-----------------------------------------------------------------------------
diff --git a/dom/system/gonk/ril_consts.js b/dom/system/gonk/ril_consts.js
index 653c6c0..a2151bc 100644
--- a/dom/system/gonk/ril_consts.js
+++ b/dom/system/gonk/ril_consts.js
@@ -893,6 +893,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // Proactive Commands
   0x00, // Proactive Commands
   0x00, // Softkey support
+  /*
   0x00, // Softkey information
   0x00, // BIP proactive commands
   0x00, // BIP supported bearers
@@ -903,6 +904,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // 18, RFU
   0x00, // 19, RFU
   0x00, // 20, RFU
+  */
 ];
 
 /**
 
 
-----------------------------------------------------------------------------
Does not work :
-----------------------------------------------------------------------------
diff --git a/dom/system/gonk/ril_consts.js b/dom/system/gonk/ril_consts.js
index 653c6c0..99e1ba8 100644
--- a/dom/system/gonk/ril_consts.js
+++ b/dom/system/gonk/ril_consts.js
@@ -894,6 +894,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // Proactive Commands
   0x00, // Softkey support
   0x00, // Softkey information
+  /*
   0x00, // BIP proactive commands
   0x00, // BIP supported bearers
   0x00, // Screen height
@@ -903,6 +904,7 @@ this.STK_SUPPORTED_TERMINAL_PROFILE = [
   0x00, // 18, RFU
   0x00, // 19, RFU
   0x00, // 20, RFU
+  */
 ];
 
 /**

Comment 10

[:gerard-majax]

One will notice that it works well with 10, and that it does not work when going to 11.

Comment 11

[:gerard-majax]

Some other results of logcat on the radio. I can't decide if in the second case we send the correct profile. But it seems we get the same answer in both case.

Half-length profile (cut in the middle), Buf.writeUint32(profile.length * 2);
E/RIL(s)  (   81): TX: (M)IPC_SAT_CMD  (S)IPC_SAT_PROFILE_DOWNLOAD  (T)IPC_CMD_SET (len:0x1C mseq:0x55 aseq:0x00) [ 01 30 42 45 33 46 46 33 46 31 46 30 30 30 30 30 30 ... ]
E/RIL(s)  (   81): RX: (M)IPC_GEN_CMD  (S)IPC_GEN_PHONE_RES  (T)IPC_CMD_RESP (len:0x0C mseq:0xFF aseq:0x55) [ 0E 01 03 01 80 ]

Full-length profile, Buf.writeUint32(profile.length * 4);
E/RIL(s)  (   81): TX: (M)IPC_SAT_CMD  (S)IPC_SAT_PROFILE_DOWNLOAD  (T)IPC_CMD_SET (len:0x1C mseq:0x52 aseq:0x00) [ 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ... ]
E/RIL(s)  (   81): RX: (M)IPC_GEN_CMD  (S)IPC_GEN_PHONE_RES  (T)IPC_CMD_RESP (len:0x0C mseq:0xFF aseq:0x52) [ 0E 01 03 01 80 ]

Comment 12

[:gerard-majax]

And now a small dump of sent parcel in both cases:

Full-length profile, Buf.writeUint32(profile.length * 4);
I/Gecko   (   77): RIL Worker: New outgoing parcel of type 68
I/Gecko   (   77): RIL Worker: Outgoing parcel: 0,0,0,96,68,0,0,0,44,0,0,0,80,0,0,0,48,0,66,0,69,0,51,0,70,0,70,0,51,0,70,0,49,0,70,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,0,0,0,0

Half-length profile (cut in the middle), Buf.writeUint32(profile.length * 2);
I/Gecko   (   78): RIL Worker: New outgoing parcel of type 68
I/Gecko   (   78): RIL Worker: Outgoing parcel: 0,0,0,56,68,0,0,0,44,0,0,0,20,0,0,0,48,0,66,0,69,0,51,0,70,0,70,0,51,0,70,0,49,0,70,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,48,0,0,0,0,0

Comment 13

[:gerard-majax]

Please note that (profile.length*4) seems to be consistent with bug 819790 from what I can infer in both cases.

Comment 14

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

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> According to TS 101.127 section 5.2
> [http://www.etsi.org/deliver/etsi_ts/101200_101299/101267/08.18.00_60/
> ts_101267v081800p.pdf], encoding of the profile downloaded to SIM is
> composed of 19 bytes plus one subsequent byte at zero.
> 
> in ril_consts.js, this.STK_SUPPORTED_TERMINAL_PROFILE is defined with 20
> bytes, and sendStkTerminalProfile() has the following code:
> 
>   sendStkTerminalProfile: function sendStkTerminalProfile(profile) {
>     Buf.newParcel(REQUEST_STK_SET_PROFILE);
>     Buf.writeUint32(profile.length * 2);
This line writes 'String length'

>     for (let i = 0; i < profile.length; i++) {
>       GsmPDUHelper.writeHexOctet(profile[i]);
>     }    
>     Buf.writeUint32(0);
This line writes String delimeter.

Comment 15

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

(In reply to Alexandre LISSY :gerard-majax from comment #13)
> Please note that (profile.length*4) seems to be consistent with bug 819790
> from what I can infer in both cases.

Hi Lissy
I think this bug and bug 819790 are different

From Bug 819790, the length is read from GsmPDUHelper.readHexOctet
so /3 is correct, we don't have to do /2 again (become /6 now) to get the length of octets.

But in STK Profile Download, the length is written for string length.
Each octet will be encoded to two chars, said 0xff -> 'ff'
1 byte -> two chars
so the length should be * 2.

Comment 16

[Juan Gomez [:_AtilA_] (CET/CEST)]

Ok, I was following another path that consisted of sending the command without encoding it with PDU format, and using profile.length as Parecel length. Yoshi saw me that I was wrong cause PDU format is a requierement of ril (as inferred from ril.h). This approach overcomes the crash problem with ril, but I saw that I was receiving this response after sending the Terminal Profile Download command:

I/Gecko   ( 1914): RIL Worker: New incoming parcel of size 12
I/Gecko   ( 1914): RIL Worker: Parcel (size 12): 0,0,0,0,15,0,0,0,2,0,0,0
I/Gecko   ( 1914): RIL Worker: We have at least one complete parcel.
I/Gecko   ( 1914): RIL Worker: Solicited response for request type 68, token 15, error 2

I'm investigating what that response means (error 2?). But I'm receiving the same response when I implement Alexandre approach (using profile.length * 4 with PDU encoding).

Well, at least rild is not crashing anymore, but it looks like the terminal profile message that we are sending is not what Samsung's RIL is expecting.

Comment 17

[:gerard-majax]

I've also found this: https://android.googlesource.com/device/samsung/crespo/+/0e70776ea20206519aebf48e33483cb53cd452ab%5E!/

The interesting part is:
	[ Problem ]
	 - When we power on Crespo with SKT 3G USIM inserted.
	   Crespo does not recognize USIM and causing infinite sim reset process.

	[ SW Modification ]
	 - We enabled major sim toolkit command like Send SMS, display text, refresh etc.
	   at Terminal Profile which notifing Mobile capability to SIM.

	[ Reason ]
	 - Basically, STK USIM is not following 3GPP 31.101 standard.
	 - When STK USIM receive Terminal Profile which basic sim toolkit command disabled from Mobile,
	   It is not returning any status word 1,2 whcih have the result of current command processed.
	   Because mobile didn't received any sw1, sw2 from SIM, It is trying to recover this invalid situation causing
	   Cold reset to SIM. after that same Terminal Profile fall down to SIM and no sw1, sw2 again.
	   this is repeated.

Comment 18

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

For the STK_SET_PROFILE request, it seems Samsung RIL doesn't implement this (so the TERMINAL PROFILE is implemented in modem/baseband)

Currently I found some builds of my Nexus-S will crash and keeps rebooting,
but even I remove the those profile download related code, it's still crashing.
So I am thinking maybe it's caused by other stuff.

Comment 19

[:gerard-majax]

I've updated the code a couple of minutes ago, and once stk profile download commented, no crash.

Comment 20

[cajbir (:cajbir)]

I'm also getting a hang on boot. This is my logcat:

E/Profiler(  193): BEGIN mozilla_sampler_init
E/Profiler(  193): ----------------- MOZ_PROFILER_NEW not set -----------------
E/Profiler(  193): Registering start signal
E/GeckoConsole(  193): Could not read chrome manifest 'file:///system/b2g/chrome.manifest'.
F/<unknown>(   80): stack corruption detected: aborted
I/Gonk    (   76): OnDisconnect

Note that "stack corruption detected: aborted".

I can confirm the workaround in comment 2 enables my Nexus S to boot and the phone works.

Comment 21

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

Hi Vincent, 
The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S
(Actually it doesn't work on otoro/unagi either)

Could you add a RIL quirk for this?

Comment 22

[Vincent Liu[:vliu]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21)
> Hi Vincent, 
> The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S
> (Actually it doesn't work on otoro/unagi either)
> 
> Could you add a RIL quirk for this?

Sure! I will try adding it.

Comment 23

[Vincent Liu[:vliu]]

Attachment: WIP V1

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21)
> Hi Vincent, 
> The REQUEST_STK_SEND_PROFILE doesn't work on Nexus-S
> (Actually it doesn't work on otoro/unagi either)
> 
> Could you add a RIL quirk for this?

Hi Yoshi,

Can you give some feedbacks for the WIP? Thanks!

Comment 24

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

Comment on attachment 741664 [details] [diff] [review]
WIP V1

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

Good!

Thank you :)

::: dom/system/gonk/ril_worker.js
@@ +76,5 @@
>  let RILQUIRKS_SIM_APP_STATE_EXTRA_FIELDS = libcutils.property_get("ro.moz.ril.simstate_extra_field", "false") === "true";
>  // Needed for call-waiting on Peak device
>  let RILQUIRKS_EXTRA_UINT32_2ND_CALL = libcutils.property_get("ro.moz.ril.extra_int_2nd_call", "false") == "true";
> +let RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD = libcutils.property_get("ro.moz.ril.send_stk_profile_dl", "false") == "true";
> +// Ril quirk to Send STK Profile Download

Comment on wrong line?

Comment 25

[Vincent Liu[:vliu]]

Attachment: WIP V2

(In reply to Alexandre LISSY :gerard-majax from comment #19)
> I've updated the code a couple of minutes ago, and once stk profile download
> commented, no crash.

Can you please add the patch and try again? Thanks

Comment 26

[:gerard-majax]

(In reply to Vincent Liu[:vliu] from comment #25)
> Created attachment 741670 [details] [diff] [review]
> WIP V2
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > I've updated the code a couple of minutes ago, and once stk profile download
> > commented, no crash.
> 
> Can you please add the patch and try again? Thanks

I'm sorry, I don't have the time right now :(

Comment 27

[:gerard-majax]

(In reply to Vincent Liu[:vliu] from comment #25)
> Created attachment 741670 [details] [diff] [review]
> WIP V2
> 
> (In reply to Alexandre LISSY :gerard-majax from comment #19)
> > I've updated the code a couple of minutes ago, and once stk profile download
> > commented, no crash.
> 
> Can you please add the patch and try again? Thanks

I've retried working on this bug and I can still confirm that changing the length to "(profile.length + 1) * 2" OR removing the leading '0' written fixes the crash. I'm currently in the process of cross checking on Inari, but I don't see why this would not be a proper fix.

Comment 28

[:gerard-majax]

Attachment: Send correct STK TERMINAL PROFILE length

Please find attached a patch that fixes the issue by setting the correct amount of bytes that we will be sending to the modem. As far as I could test, it does not breaks Inari device.

Comment 29

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

Hi Alexandre
Can you explain why +1 is needed?

The line 
Buf.writeUint32(profile.length * 2);
is to write the string length.

Each octet 0x12 will be represeneted '12' in hex format string.
And '12' is string of length 2
So that's why *2 is needed.

The following 
Buf.writeUint32(0) is to write string delimiter.
It serves the same purpose as 
Buf.writeStringDelmiter(profile.length * 2)
which is to write 4 octets of zero.

Thanks

Comment 30

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

Also if you insist +1 could work on Nexus S, 
can you also check GET_STK_PROFILE could return the same profile you set when calling SET_STK_PROFILE?

Comment 31

[:gerard-majax]

Comment on attachment 741670 [details] [diff] [review]
WIP V2

Actually this is working.

Comment 32

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

Can vliu or alexandre update the title in the patch?
It seems this patch is malformed I cannot review it entirely on Bugzilla.

Comment 33

[:gerard-majax]

Attachment: Enable STK Profile Download on a per device basis

Same patch, reformated.

Comment 34

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

Comment on attachment 785710 [details] [diff] [review]
Enable STK Profile Download on a per device basis

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

This patch still looks weird to me,
when I do the review,
I only see line 93~95 are adding,
but not the changes in line 3261, 3269, and 10762. 

Also the patch here doesn't contain 8 lines of context.

Comment 35

[:gerard-majax]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #34)
> Comment on attachment 785710 [details] [diff] [review]
> Enable STK Profile Download on a per device basis
> 
> Review of attachment 785710 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch still looks weird to me,
> when I do the review,
> I only see line 93~95 are adding,
> but not the changes in line 3261, 3269, and 10762. 
> 
> Also the patch here doesn't contain 8 lines of context.

I'm getting the same behavior :(

Yet it's just a git format-patch -1

Comment 36

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

Attachment: HG patch

Comment 37

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

Comment on attachment 788050 [details] [diff] [review]
HG patch

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

Hi, Lissy
can you help to update this HG patch?
And add r=me.

::: dom/system/gonk/ril_worker.js
@@ +89,5 @@
>  let RILQUIRKS_EXTRA_UINT32_2ND_CALL = libcutils.property_get("ro.moz.ril.extra_int_2nd_call", "false") == "true";
>  // On the emulator we support querying the number of lock retries
>  let RILQUIRKS_HAVE_QUERY_ICC_LOCK_RETRY_COUNT = libcutils.property_get("ro.moz.ril.query_icc_count", "false") == "true";
>  
> +// Ril quirk to Send STK Profile Download 

trailing space

@@ +3295,5 @@
>          ICCRecordHelper.fetchICCRecords();
>        } else if (this.appType == CARD_APPTYPE_USIM) {
> +        if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) {
> +          this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE);
> +	}

a Tab here.

@@ +3300,5 @@
>          ICCRecordHelper.fetchICCRecords();
>        } else if (this.appType == CARD_APPTYPE_RUIM) {
> +        if (RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD) {
> +          this.sendStkTerminalProfile(STK_SUPPORTED_TERMINAL_PROFILE);
> +	}

here to.

Comment 38

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

Comment on attachment 785710 [details] [diff] [review]
Enable STK Profile Download on a per device basis

Please try my hg patch.

Comment 40

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

Discussed with Lissy on irc, will assign to Georgia to land this patch.

Comment 41

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #38)
> Please try my hg patch.

Tried this patch on my nexus-s and call functionality was properly restored. Please check this in!

Comment 42

[Georgia Wang]

I've also try the patch in comment #38, there's no aborted in log and device works fine. 

Gaia: 9ebfaca574dc70893494ef445880ff897a7cb03b
Gecko: 6a9509a8d7d1fde2f6c77c6b459191683cc23dbe

Comment 43

[Georgia Wang]

Attachment: Part1: Add Ril quirk for REQUEST_STK_SEND_PROFILE. v1

Comment 44

[Georgia Wang]

Attachment: Part2: xpcshell test for RILQUIRKS_SEND_STK_PROFILE_DOWNLOAD v.1

Comment 45

[Georgia Wang]

Try result: https://tbpl.mozilla.org/?tree=Try&rev=9c888c64cbc5

Comment 46

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

https://hg.mozilla.org/integration/b2g-inbound/rev/8f9fac2ade0f
https://hg.mozilla.org/integration/b2g-inbound/rev/0024361780c4

Comment 47

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8f9fac2ade0f
https://hg.mozilla.org/mozilla-central/rev/0024361780c4