Closed Bug 823816 Opened 7 years ago Closed 7 years ago

B2G MMS: WSP Text decode check the wrong range of ASCII code.

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-b2g leo+
Tracking Status
firefox20 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: ctai, Assigned: ctai)

References

Details

Attachments

(2 files, 2 obsolete files)

WspPduHelper parse the decodeConstrainedMedia like 0x9d813100 will cause wrong result=>WspPduHelper: contentType = {"media":"Ž1","params":null}

The root cause is the wrong range of ASCII in Text.decode.
After change the range check statement from "if ((code >= CTLS) && (code != DEL)) {" to "if ((code >= CTLS) && (code <= DEL)) {", I can fix the bug.

It shows the correct result like "WspPduHelper: contentType = {"media":"image/gif","params":null}".
Attached patch Fix the wrong ASCII range (obsolete) — Splinter Review
Attachment #694690 - Flags: review?(vyang)
Comment on attachment 694690 [details] [diff] [review]
Fix the wrong ASCII range

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

It's not. RFC-2616 has following definitions clearly:

  OCTET = <any 8-bit sequence of data>
  TEXT  = <any OCTET except CTLs, but including LWS>

So TEXT can definitely be non-ASCII, 8-bit characters. Instead, BNF of Extension-media should be:

  Extension-media = Text-string

to solve this case perfectly. However, I can't find any update to WAP-230-WSP-20010705-a now. Another solution would be try decoding ShortInteger before NullTerminatedTexts as Android does. Again, that's not specified in WSP spec.
Attachment #694690 - Flags: review?(vyang) → review-
I don't think this issue should ever block bug 801632.
Blocks: b2g-mms
No longer blocks: 801632
Please see below codes for how Android dealing this issue for reference.
In function "parseContentType" in frameworks/opt/mms/src/java/com/google/android/mms/pdu/PduParser.java shows 
            if ((first >= TEXT_MIN) && (first <= TEXT_MAX)) {
                contentType = parseWapString(pduDataStream, TYPE_TEXT_STRING);
            } else if (first > TEXT_MAX) {
                int index = parseShortInteger(pduDataStream);
            ....
            }
Android check the first char to decide taking as WapString or ShortInteger. Maybe we can consider this method.
Attachment #694690 - Attachment is obsolete: true
Attachment #694732 - Flags: review?(vyang)
Comment on attachment 694732 [details] [diff] [review]
Replace NullTerminatedTexts witn TextString

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

Thank you! But since this is a non-standard solution, we should also update comments for |this.ConstrainedEncoding|.

::: dom/mms/src/ril/WspPduHelper.jsm
@@ +881,5 @@
>     *
>     * @return Decode integer value or string.
>     */
>    decode: function decode(data) {
> +    return decodeAlternatives(data, null, TextString, ShortInteger);

Could you also updates encode()? TypeValue.encode() should call ConstrainedEncoding.encode(), and ConstrainedEncoding.encode() should either call ShortInteger.encode() or TextString.encode().
Attachment #694732 - Flags: review?(vyang)
Attachment #695408 - Flags: review?(vyang)
Comment on attachment 695408 [details] [diff] [review]
Replace NullTerminatedTexts witn TextString

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

\(^^)/
Attachment #695408 - Flags: review?(vyang) → review+
https://hg.mozilla.org/mozilla-central/rev/b86cca9cd8d2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
For mms merge to b2g18.
blocking-b2g: --- → leo?
blocking-b2g: leo? → leo+
The entire set of clian's pushes was backed out for multiple reasons.
https://tbpl.mozilla.org/?tree=Mozilla-B2g18&rev=a0b06192f882

1.) The tree rules are clear that you are not to land on top of bustage. At the time you pushed, both B2G Mn and B2G xpcshell had bustage from prior commits that hadn't been backed out yet.
2.) The tree rules are also clear that you are to watch your pushes for any bustage and handle them accordingly. mozilla-inbound is the ONLY tree where this rule does not apply.
3.) Even after the earlier bustage was backed out, something in one of your many pushes was causing further B2G Mn failures as shown in the log below.
https://tbpl.mozilla.org/php/getParsedLog.php?id=20424173&tree=Mozilla-B2g18
4.) This isn't cause for backout by itself, but it is also strongly preferred to not push each commit individually as our build and testing resources are limited and doing so stretches them even thinner. Please limit your number of pushes as much as possible unless you have good reason for keeping them separate.
Flags: in-moztrap-
You need to log in before you can comment on or make changes to this bug.