B2G SMS: readSwappedNibbleBCD may discard leading zeros

RESOLVED FIXED in mozilla15

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 709564, 751052
Blocks: 754018
No longer blocks: 751052
(Assignee)

Comment 1

5 years ago
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
Assignee: nobody → vyang
(Assignee)

Comment 2

5 years ago
Created attachment 623925 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address
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);
       ^^^^^
(Assignee)

Comment 4

5 years ago
Created attachment 624271 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V2

address review comment #3
Attachment #623925 - Attachment is obsolete: true
Attachment #624271 - Flags: review?(yhuang)
Attachment #624271 - Flags: review?(philipp)
(Assignee)

Comment 5

5 years ago
Created attachment 624275 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V3

Just don't use magic number
Attachment #624271 - Attachment is obsolete: true
Attachment #624271 - Flags: review?(yhuang)
Attachment #624271 - Flags: review?(philipp)
Attachment #624275 - Flags: review?(yhuang)
Attachment #624275 - Flags: review?(philipp)
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.
Attachment #624275 - Flags: review?(yhuang) → feedback+
(Assignee)

Comment 7

5 years ago
Created attachment 624343 [details] [diff] [review]
add readSwappedNibbleBcdString for complex GSM address : V4

Rebase to dev tip
Attachment #624275 - Attachment is obsolete: true
Attachment #624275 - Flags: review?(philipp)
Attachment #624343 - Flags: review?(philipp)
Attachment #624343 - Flags: feedback?(yhuang)
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.
Attachment #624343 - Flags: review?(philipp) → review+
No longer blocks: 754018
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.
(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... :)
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.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a089d72876e
https://hg.mozilla.org/mozilla-central/rev/8a089d72876e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Assignee)

Updated

5 years ago
Depends on: 758658
Attachment #624343 - Flags: feedback?(yhuang)
(Assignee)

Updated

5 years ago
Blocks: 758658
No longer depends on: 758658
You need to log in before you can comment on or make changes to this bug.