B2G SMS: potential errors in GsmPDUHelper.calculateUserDataLength()

RESOLVED FIXED in mozilla13

Status

()

--
minor
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: vicamo, Assigned: vicamo)

Tracking

unspecified
mozilla13
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

7 years ago
There might be other potential errors in GsmPDUHelper.calculateUserDataLength():

01  calculateUserDataLength: function calculateUserDataLength(options) {
02    ...
03    for (...) {
04      ...
05      // Calculate full user data length, note the extra byte is for header len
06      let userDataLength = length + (headerLen ? headerLen + 1 : 0);
07      ...
08      if (userDataLength <= options.body.length) {
09        // Found minimum user data length already
10        return;
11      }
12    }
13    ...
14  }

In line 6, the unit of `headerLen` is octet, while that of `length` is septet. Under some situations, the result may vary.

In line 8~10, when it comes with a string with numerous ineffective characters like <escape>, the calculated encoded string length may be much less than options.body.length, and it is therefore possible for another encoding to have an even shorter result. For example, an input string of length 10 might get calculated cost 8 with several characters encoded with single shift mechanism. But another encoding might get cost 5 with all characters encoded with locking shift mechanism. This should be a minor issue because users cannot key-in \uffff and \ufffe, which are internally assigned to ineffective characters <escape> and <resctl>.
(Assignee)

Updated

7 years ago
Assignee: nobody → vyang
(Assignee)

Comment 1

7 years ago
This issue was first addressed in https://bugzilla.mozilla.org/show_bug.cgi?id=733300#c4 , and had already fixes in attachment 603660 [details] [diff] [review]. Should also add test cases for them.
(Assignee)

Comment 2

7 years ago
Created attachment 603972 [details] [diff] [review]
Part 1: headerLen is in octets, not septets

1) rename related variables, functions to have more clear meaning.
2) add test case
Attachment #603972 - Flags: review?(philipp)
(Assignee)

Comment 3

7 years ago
Created attachment 603973 [details] [diff] [review]
Part 2: possible early return non-best choice

1) remove early return condition
2) add test case
Attachment #603973 - Flags: review?(philipp)
Attachment #603972 - Flags: review?(philipp) → review+
Attachment #603973 - Flags: review?(philipp) → review+
https://hg.mozilla.org/mozilla-central/rev/f20ede6f964a
https://hg.mozilla.org/mozilla-central/rev/c2d1c97a8a2f
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.