317216 - UTF16 and UTF32 decoders allow invalid UTF16 into the core

Status: VERIFIED FIXED

(Core :: Internationalization, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

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]

(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 [:bzbarsky]]

>   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]

Attachment: 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]

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 [:bzbarsky]]

Comment on attachment 203790 [details] [diff] [review]
UTF-32 patch

Excellent!

Comment 6

[Jungshik Shin]

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]

(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)]

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]

I believe it was nominated because invalid UTF-16 in core has caused memory safety bugs in the past.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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)]

OK.  I'll plus this, set the priority to P3.  Please reset the priority if need be.

Comment 12

[Simon Montagu :smontagu]

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]

Attachment: 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 [:bzbarsky]]

Simon, could you find someone else to sr this, please?

Comment 15

[Mike Beltzner [:beltzner, not reading bugmail]]

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 [:bzbarsky]]

I think this should be at least wanted1.9.0.x....

Comment 17

[Simon Montagu :smontagu]

biesi: ping?

Comment 18

[Daniel Veditz [:dveditz]]

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]]

Comment on attachment 288642 [details] [diff] [review]
UTF-16 patch

sr=dveditz

Comment 20

[Simon Montagu :smontagu]

Attachment: UTF-16 patch v.3

This is a co-production from jag and me. r=me for his parts.

Comment 21

[jag (Peter Annema)]

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]

http://hg.mozilla.org/mozilla-central/rev/1abd98b9df00

Comment 23

[Simon Montagu :smontagu]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/bd90eb7a73ee

Comment 24

[Simon Montagu :smontagu]

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]]

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]]

This will probably apply to the 1.8 branch, right?

Comment 27

[Simon Montagu :smontagu]

Probably not, actually, unless we combine it with the patch from bug 340714.

Comment 28

[Daniel Veditz [:dveditz]]

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 - ex-MoCo]]

Verified for 1.9.0.9 based on checked in tests with the patch.

Comment 30

[Simon Montagu :smontagu]

Attachment: 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]]

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]

Attachment: Testcase

Stand-alone version of the unit test for testing on 1.8 branch

Comment 33

[Simon Montagu :smontagu]

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 - ex-MoCo]]

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.