Last Comment Bug 317216 - UTF16 and UTF32 decoders allow invalid UTF16 into the core
: UTF16 and UTF32 decoders allow invalid UTF16 into the core
Status: VERIFIED FIXED
[sg:want P1]
: fixed1.9.1, verified1.8.1.22, verified1.9.0.9
Product: Core
Classification: Components
Component: Internationalization (show other bugs)
: Trunk
: x86 Linux
: P2 normal with 2 votes (vote)
: ---
Assigned To: Simon Montagu :smontagu
:
:
Mentors:
Depends on:
Blocks: 316338
  Show dependency treegraph
 
Reported: 2005-11-20 14:32 PST by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2009-06-04 14:27 PDT (History)
17 users (show)
mbeltzner: wanted‑next+
benjamin: blocking1.9.1+
dveditz: blocking1.9.0.9+
samuel.sidler+old: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
UTF-32 patch (3.21 KB, patch)
2005-11-21 06:41 PST, Jungshik Shin
smontagu: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
UTF-16 patch (13.83 KB, patch)
2007-11-14 03:37 PST, Simon Montagu :smontagu
jshin1987: review+
dveditz: superreview+
Details | Diff | Splinter Review
UTF-16 patch v.3 (15.09 KB, patch)
2009-02-19 07:13 PST, Simon Montagu :smontagu
jag-mozilla: review+
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
Patch for 1.8 branch (14.84 KB, patch)
2009-04-05 23:37 PDT, Simon Montagu :smontagu
dveditz: approval1.8.1.next+
smontagu: approval1.8.0.next?
Details | Diff | Splinter Review
Testcase (6.58 KB, text/html)
2009-04-07 01:57 PDT, Simon Montagu :smontagu
no flags Details

Description Boris Zbarsky [:bz] (still a bit busy) 2005-11-20 14:32:02 PST
See bug 316338.  The relevant parts are:

> There's also the following comment in our "UCS216ToUnicode" converter:

> // XXX : illegal surrogate code points are just passed through !!

and

> and also in UTF32ToUnicode:

> // XXX Do we have to convert surrogate code points to the replacement
> // character (0xfffd)? 

The answer to the latter question is "Yes, please!", of course.
Comment 1 Simon Montagu :smontagu 2005-11-20 23:41:31 PST
(In reply to comment #0)
> > // XXX Do we have to convert surrogate code points to the replacement
> > // character (0xfffd)? 
> 
> The answer to the latter question is "Yes, please!", of course.

It was discussed in bug 184120 comment 15:

> > Should you check that the input isn't in the surrogate ranges here (and > replace
> > with 0xfffd, perhaps?), since failing to check could allow an 
> > incorrectly
> > encoded thing that looks like a surrogate into the data?  
> 
>   It's not very clear what to do in such a case. (UAX #19 doesn't
> say anything about what to do when converting UTF-32 to other
> encoding forms). I'll ask around. In the meantime, I added a comment.
> 
> > Or do all our
> > routines that handle UTF-16 correctly handle such errors?
> 
>   They're supposed to check for unpaired surrogate code points.
> In all cases I'm aware of, our routines do the 'right thing'.
> Therefore, I guess we can get away with just passing
> thru all codepoints below 0x10000 for the time being. 

I think we've learned from bug 316338 and its offshoots that that's not the case. The definitions and conformance requirements have also been clarified in the current version of Unicode[1]:

D31: Because surrogate code points are not included in the set of Unicode scalar values, UTF-32 code units in the range 0000D80016..0000DFFF16 are ill-formed.

D35: Because surrogate code points are not Unicode scalar values, isolated UTF-16 code units in the range D80016..DFFF16 are ill-formed.

C12a When a process interprets a code unit sequence which purports to be in a Unicode character encoding form, it shall treat ill-formed code unit sequences as an error condition, and shall not interpret such sequences as characters.

[1] http://www.unicode.org/versions/Unicode4.0.0/ch03.pdf
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2005-11-21 05:52:52 PST
>   They're supposed to check for unpaired surrogate code points.
> In all cases I'm aware of, our routines do the 'right thing'.

Er..  "our routines" would be the whole friggin' codebase, plus any system APIs that we pass UTF16 to.  There's no way they could all possibly be doing "the right thing", and imposing such a burden on all of it is unreasonable.
Comment 3 Jungshik Shin 2005-11-21 06:41:29 PST
Created attachment 203790 [details] [diff] [review]
UTF-32 patch

As I'm to blame for that XXX  comments and oversight, I'm taking it. This is the easier of two cases.
Comment 4 Simon Montagu :smontagu 2005-11-21 07:23:48 PST
Comment on attachment 203790 [details] [diff] [review]
UTF-32 patch

The same code is repeated later in the source file. Make the same changes there and r=me.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2005-11-21 11:44:48 PST
Comment on attachment 203790 [details] [diff] [review]
UTF-32 patch

Excellent!
Comment 6 Jungshik Shin 2005-11-23 21:03:46 PST
Thanks for your review and reference.
UTF-32 patch checked into the trunk a couple of days ago. As for UTF-16, I quickly made a patch, but that's not perfect in that it doesn't handle cases where a surrogate pair is splitted into two separate chunks. Btw, Simon, do you think we just have to replace an isolated surrogate by the replacement character? That's what's done in UTF-32 patch..
Comment 7 Simon Montagu :smontagu 2005-11-24 00:03:33 PST
(In reply to comment #6)
> Btw, Simon, do you
> think we just have to replace an isolated surrogate by the replacement
> character? That's what's done in UTF-32 patch..

Yes, I think so, based on D35 that I quoted in comment 1.
Comment 8 Damon Sicore (:damons) 2007-11-08 17:36:59 PST
If this has been checked in and is really fixed/resolved, please mark as so.  This is old, and I'm not sure why this should block 1.9.  Please re-nom if necessary.
Comment 9 Jesse Ruderman 2007-11-08 17:57:45 PST
I believe it was nominated because invalid UTF-16 in core has caused memory safety bugs in the past.
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2007-11-08 18:06:40 PST
What Jesse says.  This bug was part of a series of bugs filed once we realized we were running into security issues with this...

We still need to fix the UTF16 decoder.
Comment 11 Damon Sicore (:damons) 2007-11-09 13:26:24 PST
OK.  I'll plus this, set the priority to P3.  Please reset the priority if need be.
Comment 12 Simon Montagu :smontagu 2007-11-10 10:46:08 PST
It shouldn't be hard to allow for surrogate pairs split into two separate chunks as mentioned in comment 6 -- we handle the same situation in other multi-byte encodings.
Comment 13 Simon Montagu :smontagu 2007-11-14 03:37:36 PST
Created attachment 288642 [details] [diff] [review]
UTF-16 patch

This turned out to be a little trickier than I expected, because of the possibility of either odd bytes or odd surrogates between buffers, combined with the possibility of swapping endianness. I moved some things around to make the new code less tangled.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2007-11-14 08:49:47 PST
Simon, could you find someone else to sr this, please?
Comment 15 Mike Beltzner [:beltzner, not reading bugmail] 2008-02-29 14:16:51 PST
bz: moving off the hard blocker list for this release, renom if you think it MUST be fixed before we ship Firefox 3.

We'll take a reviewed patch, of course.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2008-02-29 14:50:46 PST
I think this should be at least wanted1.9.0.x....
Comment 17 Simon Montagu :smontagu 2008-03-01 14:03:33 PST
biesi: ping?
Comment 18 Daniel Veditz [:dveditz] 2008-12-23 10:44:31 PST
This got bumped out of 3.0 in favor of a later 3.0.x release and then died. All it needs is an sr= -- does it really have to be biesi?
Comment 19 Daniel Veditz [:dveditz] 2009-02-18 09:58:05 PST
Comment on attachment 288642 [details] [diff] [review]
UTF-16 patch

sr=dveditz
Comment 20 Simon Montagu :smontagu 2009-02-19 07:13:51 PST
Created attachment 363113 [details] [diff] [review]
UTF-16 patch v.3

This is a co-production from jag and me. r=me for his parts.
Comment 21 jag (Peter Annema) 2009-02-20 18:55:10 PST
Comment on attachment 363113 [details] [diff] [review]
UTF-16 patch v.3

r=jag on the tests and the changes outside UTF16ConvertToUnicode.

You could create the |IOService| and |ConverterInputStream| constructors and |ios| outside testCase, and use |const| instead of |var|.
Comment 22 Simon Montagu :smontagu 2009-02-22 02:13:24 PST
http://hg.mozilla.org/mozilla-central/rev/1abd98b9df00
Comment 23 Simon Montagu :smontagu 2009-03-01 01:51:47 PST
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bd90eb7a73ee
Comment 24 Simon Montagu :smontagu 2009-03-01 02:01:53 PST
Comment on attachment 363113 [details] [diff] [review]
UTF-16 patch v.3

Requesting branch approval after a week baking on trunk. We have good test coverage of UTF-16 decoding with the tests from bug 335531, bug 340714 and bug 396637, plus the new test in this patch, so I am confident that it shouldn't cause any regressions.
Comment 25 Daniel Veditz [:dveditz] 2009-03-06 11:36:24 PST
Comment on attachment 363113 [details] [diff] [review]
UTF-16 patch v.3

Approved for 1.9.0.8, a=dveditz for release-drivers
Comment 26 Daniel Veditz [:dveditz] 2009-04-01 15:54:18 PDT
This will probably apply to the 1.8 branch, right?
Comment 27 Simon Montagu :smontagu 2009-04-01 21:01:32 PDT
Probably not, actually, unless we combine it with the patch from bug 340714.
Comment 28 Daniel Veditz [:dveditz] 2009-04-03 10:23:33 PDT
Which we probably want to do, sorry. We're still supporting Thunderbird 2 and it can potentially get a lot of bad data pushed down to it.
Comment 29 Al Billings [:abillings] 2009-04-03 12:22:58 PDT
Verified for 1.9.0.9 based on checked in tests with the patch.
Comment 30 Simon Montagu :smontagu 2009-04-05 23:37:46 PDT
Created attachment 371199 [details] [diff] [review]
Patch for 1.8 branch

This includes both patches from this bug, and also the patches from bug 396637, bug 340714 and bug 376636.
Comment 31 Daniel Veditz [:dveditz] 2009-04-06 14:36:22 PDT
Comment on attachment 371199 [details] [diff] [review]
Patch for 1.8 branch

Approved for 1.8.1.22, a=dveditz for release-drivers
Comment 32 Simon Montagu :smontagu 2009-04-07 01:57:25 PDT
Created attachment 371409 [details]
Testcase

Stand-alone version of the unit test for testing on 1.8 branch
Comment 33 Simon Montagu :smontagu 2009-04-07 02:03:40 PDT
N.B. attachment 371409 [details] request enhanced privileges, so will need to be saved and run locally (or with signed.applets.codebase_principal_support set to true)
Comment 34 Al Billings [:abillings] 2009-06-04 14:27:30 PDT
Verified for 1.8.1.22 using Seamonkey and stand-alone testcase: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.22pre) Gecko/20090604 SeaMonkey/1.1.17pre. 

Verified for Trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090604 Minefield/3.6a1pre.

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