3.21 KB, patch
|Details | Diff | Splinter Review|
15.09 KB, patch
jag (Peter Annema): review+
|Details | Diff | Splinter Review|
14.84 KB, patch
|Details | Diff | Splinter Review|
6.58 KB, text/html
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.
(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: 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.  http://www.unicode.org/versions/Unicode4.0.0/ch03.pdf
> 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.
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 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 on attachment 203790 [details] [diff] [review] UTF-32 patch Excellent!
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..
(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.
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.
I believe it was nominated because invalid UTF-16 in core has caused memory safety bugs in the past.
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.
OK. I'll plus this, set the priority to P3. Please reset the priority if need be.
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.
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.
Simon, could you find someone else to sr this, please?
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.
I think this should be at least wanted1.9.0.x....
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 on attachment 288642 [details] [diff] [review] UTF-16 patch sr=dveditz
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 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 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 on attachment 363113 [details] [diff] [review] UTF-16 patch v.3 Approved for 18.104.22.168, a=dveditz for release-drivers
This will probably apply to the 1.8 branch, right?
Probably not, actually, unless we combine it with the patch from bug 340714.
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.
Verified for 22.214.171.124 based on checked in tests with the patch.
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 on attachment 371199 [details] [diff] [review] Patch for 1.8 branch Approved for 126.96.36.199, a=dveditz for release-drivers
Created attachment 371409 [details] Testcase Stand-alone version of the unit test for testing on 1.8 branch
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)
Verified for 188.8.131.52 using Seamonkey and stand-alone testcase: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:184.108.40.206pre) 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.