Last Comment Bug 753034 - B2G SMS: readSwappedNibbleBCD may discard leading zeros
: B2G SMS: readSwappedNibbleBCD may discard leading zeros
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Vicamo Yang [:vicamo][:vyang]
:
Mentors:
Depends on:
Blocks: b2g-sms 758658
  Show dependency treegraph
 
Reported: 2012-05-08 11:54 PDT by Vicamo Yang [:vicamo][:vyang]
Modified: 2012-09-12 03:14 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
add readSwappedNibbleBcdString for complex GSM address (5.88 KB, patch)
2012-05-14 20:20 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
add readSwappedNibbleBcdString for complex GSM address : V2 (5.89 KB, patch)
2012-05-15 18:57 PDT, Vicamo Yang [:vicamo][:vyang]
no flags Details | Diff | Splinter Review
add readSwappedNibbleBcdString for complex GSM address : V3 (5.90 KB, patch)
2012-05-15 19:04 PDT, Vicamo Yang [:vicamo][:vyang]
allstars.chh: feedback+
Details | Diff | Splinter Review
add readSwappedNibbleBcdString for complex GSM address : V4 (5.60 KB, patch)
2012-05-16 04:37 PDT, Vicamo Yang [:vicamo][:vyang]
philipp: review+
Details | Diff | Splinter Review

Description Vicamo Yang [:vicamo][:vyang] 2012-05-08 11:54:11 PDT
The `readSwappedNibbleBCD` assumes all numeric BCDs in its input and returns a calculated number accordingly. However, for a national number, which may be prefixed with zeros, the decimal numeric string won't preserve them and results in a wrong number.
Comment 1 Vicamo Yang [:vicamo][:vyang] 2012-05-11 12:02:51 PDT
We supports only one(and incomplete) type of SMS addressing for now. The missing zeros problem could be solved by turning parsed numeric result into a string one. This is also a must especially when we'll going to support different TOAs(types of address). See 3GPP TS 24.011[1], 3GPP 29.002[2] and 3GPP TS 23.040[3] as well.

[1]: http://www.3gpp.org/ftp/Specs/html-info/24011.htm
[2]: http://www.3gpp.org/ftp/Specs/html-info/29002.htm
[3]: http://www.3gpp.org/ftp/Specs/html-info/23040.htm
Comment 2 Vicamo Yang [:vicamo][:vyang] 2012-05-14 20:20:14 PDT
Created attachment 623925 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address
Comment 3 Yoshi Huang[:allstars.chh] 2012-05-15 02:46:19 PDT
Comment on attachment 623925 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address

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

::: dom/system/gonk/ril_worker.js
@@ +3034,5 @@
> +      }
> +
> +      str += this.semiOctetToBcdChar(nibbleL);
> +      if (nibbleH != 0x0F) {
> +        str += semiOctetToBcdChar(nibbleH);

str += this.semiOctetToBcdChar(nibbleH);
       ^^^^^
Comment 4 Vicamo Yang [:vicamo][:vyang] 2012-05-15 18:57:43 PDT
Created attachment 624271 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V2

address review comment #3
Comment 5 Vicamo Yang [:vicamo][:vyang] 2012-05-15 19:04:30 PDT
Created attachment 624275 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V3

Just don't use magic number
Comment 6 Yoshi Huang[:allstars.chh] 2012-05-15 23:22:44 PDT
Comment on attachment 624275 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V3

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

I've helped to verify this patch on SIM contacts part, and it works well.
Thanks for that patch. Remember to rebase again.

I might not have privilege to review that, so I just change it to feedback+.

::: dom/system/gonk/ril_worker.js
@@ +3028,5 @@
> +    let str = "";
> +    for (let i = 0; i < pairs; i++) {
> +      let nibbleH = this.readHexNibble();
> +      let nibbleL = this.readHexNibble();
> +      if (nibbleL == 0x0F) {

maybe 0x0f? I see all other code uses 0x0f

@@ +3048,5 @@
>     *  @return the decimal as a number.
>     */ 
>    readStringAsBCD: function readStringAsBCD() {
>      let length = Buf.readUint32();
> +    let bcd = this.readSwappedNibbleBcdString(length / 2);

This function readStringAsBCD is removed in Bug 754777, you need to rebase for that.
Comment 7 Vicamo Yang [:vicamo][:vyang] 2012-05-16 04:37:57 PDT
Created attachment 624343 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V4

Rebase to dev tip
Comment 8 Philipp von Weitershausen [:philikon] 2012-05-17 17:41:58 PDT
Comment on attachment 624343 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V4

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

Looks great, but it should have unit tests.
Comment 9 Gregor Wagner [:gwagner] 2012-05-21 10:22:21 PDT
Comment on attachment 624343 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V4

=
>+
>+      str += this.semiOctetToBcdChar(nibbleL);
>+      if (nibbleH != 0x0F) {
>+        str += semiOctetToBcdChar(nibbleH);


I think there is another |this.| for semiOctetToBcdChar missing.
Comment 10 Philipp von Weitershausen [:philikon] 2012-05-21 13:20:49 PDT
(In reply to Gregor Wagner [:gwagner] from comment #9)
> >+
> >+      str += this.semiOctetToBcdChar(nibbleL);
> >+      if (nibbleH != 0x0F) {
> >+        str += semiOctetToBcdChar(nibbleH);
> 
> 
> I think there is another |this.| for semiOctetToBcdChar missing.

... which is why it needs to have tests... :)
Comment 11 Gregor Wagner [:gwagner] 2012-05-24 15:01:37 PDT
Vicamo, please file a separate bug for the tests and promise to fix it soon :)
The contacts-import patch that relies on this patch already landed so I will land this now.
Comment 13 Ed Morley [:emorley] 2012-05-25 08:27:07 PDT
https://hg.mozilla.org/mozilla-central/rev/8a089d72876e

Note You need to log in before you can comment on or make changes to this bug.