String::createUTF16() always fails if the UTF-16 data does not contain well-formed surrogate pairs. It should have a non-strict mode such as String::createUTF8() has. AvmCore::newStringUTF16() should also pass in this flag. The header documentation of String::createUTF16() does not reflect this behavior. Implementers could suffer from a crash, because the method returns NULL for a malformed UTF-16 sequence.

This should be fixed if it is an injection since FP10; but it should also be fixed if it is not, since it's a crash.

The AvmCore::newStringUTF16() was introduced with the new StringObject code. I don't know if the change already affected FP10. The patch adds a "strict" argument to String::createUTF16(), AvmCore::newStringUTF16() and AvmCore::NewStringEndianUTF16(). String::createUTF16() has a default strict value of true to match String::createUTF8(), and the AvmCore methods have a default strict value of false to make the behavior compatible with AS3, where surrogate pairs are treated as ordinary characters. Before, the methods always assumed strict behavior, leading to a return value of NULL in case of malformed surrogate pairs.

Comment on attachment 420507 [details] [diff] [review] Add strict argument to UTF-16 string creation I must confess I am reluctant to have "strict=false" be the default, as it feels wrong somehow... I presume that "strict=true" is demonstrably leading to failures in existing AS3 code?

strict == false just manifests the current AS3 behavior, where bad surrogate pairs are OK. If the default strict == true, using the method carelessly would return NULL, and could result in a crash. For safety reasons, I believe that it is better to have strict == false. Also, it implementes the same behavoir as in AvmCore::newStringUTF8(). It would be confusing to choose a different default value for strict.

Path pushed as changeset: 3532:86fb23ff850d

Are there as testcases that can be written to verify this bug (and add to the testsuite)?

I created a test case, just to find out that the fix is wrong in one place. The global functions decodeURI() and decodeURIComponent() actually check for a malformed UTF-16 sequence by checking the return value of newStringUTF16() to be NULL, and throw an error if newStringUTF16() (via Toplevel::decode()) returned NULL. I checked the usage of newStringUTF16() in the Player code, and on first sight, I could not identify any place that checked for a NULL return value in case of a malformed sequence (the search produced 58 hits). IMHO, there is plenty of room for a Player crash there. There are two ways to fix this problem: 1) Make Toplevel::decode() use strict mode, and return NULL so decodeURI() and decodeURIComponent() can throw exceptions as before. Player code will not crash anymore, but return the malformed sequence as two separate characters, which would be the same behavior as if the characters were well-formed (but not crashing). 2) Make the default value for AvmCore::newStringUTF16()'s strict argument true (as it was implicitly before), and have all Player code reviewed for potential crashers because of the possible NULL return value. The pattern that I tried was: // ED B0 80 = UTF-8 for 0xDC00 // ED A0 80 = UTF-8 for 0xD800 // 0xDC00 0xD800 is an invalid UTF-16 sequence var s = "%ED%B0%80%ED%A0%80"; decodeURIComponent(s); Please comment on which solution I should provide a patch for. I have reopened the bug for a patch.

(In reply to comment #7) > Please comment on which solution I should provide a patch for. I have reopened > the bug for a patch. Which option mimics FP10's behavior? That's the one we must choose.

This is setting the default value of strict to true. Doing so preserves the potential crasher in Player code, though (it would be option #2).

OK then, number 2 it is.

Patch for option #2: setting strict to true for AvmCore::newStringUTF16() and AvmCore::newStringEndianUTF16(), plus warnings in the API documentation.

Comment on attachment 422358 [details] [diff] [review] Warning as comment plus strict=true for UTF-16 methods Obviously, we'll have to vet all existing calls to this when it's merged into Flash/AIR, but that's a good idea anyway.

Patch pushed as changeset: 3566:af872051a045 Since the change is very small, I think that Steven's sr+ is enough. Lars, plese respond if you disagree.

(In reply to comment #13) > Patch pushed as changeset: 3566:af872051a045 > > Since the change is very small, I think that Steven's sr+ is enough. Lars, > plese respond if you disagree. I'm happy to defer to Steven.

Comment on attachment 422358 [details] [diff] [review] Warning as comment plus strict=true for UTF-16 methods Rubber stamp.

verified that previous shells would throw a rte about an invalid URI

Comment on attachment 433605 [details] [diff] [review] testcase please move file to regress/security directory.

seems it is not a security bug I thought I saw the vm crash before checkin but I was mistaken.

Comment on attachment 433605 [details] [diff] [review] testcase testcase pushed: tr-argo -> 3873:8265b24fbef7 tr -> 4179:8265b24fbef7

(In reply to comment #13) > Patch pushed as changeset: 3566:af872051a045 > Ok looks like redux and argo are not in sync with this bug. I added a testcase based on comment #7, option #2 and the testcase is passing in argo but failing in redux. It appears that the bug was initially fixed via http://hg.mozilla.org/tamarin-redux/rev/3532 and then modified via http://hg.mozilla.org/tamarin-redux/rev/3566. Rev 3532 is in tamarin-redux-argo but 3566 is not. Wondering what the expected behavior is for the testcase? Is argo or redux correct?

Marking as REOPENED and setting QRB? as this is marked as a 10.1 target but /may/ not be fixed in argo.

It appears that from comment #9 and #11 the expected behavior for the testcase that I added is to produce a RTE, which is what is happening in tamarin-redux. I will update the testcase accordingly. Still open issue is that this change is NOT in argo and probably has NOT been vetted in the playercode per comment #12

after examining the comments in the bug, the testcases SHOULD be producing errors that the URI passed into decodeURI* is invalid. This patch changes the testcase to capture and confirm the RTE that is produced.

Comment on attachment 435197 [details] [diff] [review] updated testcase Pushed patch ahead of review in order to get the build system green tr-> 4199:64c6d9104774

-----Original Message----- From: Steven Johnson Sent: Tuesday, April 06, 2010 1:25 PM To: Dan Smith Cc: Brent Baker Subject: Re: Update on Bug 538107 - String::createUTF16() is always strict um, not clear to mne either -- the fix was pushed a while back. the goal was that argo would accept "wrong" utf16, just as coral did -- brent, what is the situation? (obviously a case we need to add versioning in the future...)

This does mesh up with what I am seeing: argo -> accepts "wrong" utf16 sequence to decodeURI() and decodeURIComponent() redux -> throws an error that the string is invalid So it looks like redux has a change that needs to be reverted until it can be properly versioned?

OK, to be sure I'm clear: -- the patch we pushed caused redux to reject invalid UTF16 pairs. -- this patch hasn't made it into Argo, which is still accepting invalid UTF16 pairs. -- despite earlier comments to the contrary, it seems we want Argo to continue the accept-invalid-pairs behavior to maintain backward compatibility (until a versioning check can be added). -- thus the resolution needed here is to change redux to match argo (strict=false by default), and flag this bug as needing attention for future versioning. is this right?

sounds right to me

pushed as http://hg.mozilla.org/tamarin-redux/rev/666ee6f9e2b0 (Edwin, I credited you with an r+)

Test case needs to be flagged as expected to fail in TR.

flagged test as skip: tamarin-redux changeset: 4361:4c91cd0bf5e6

Should this bug actually be marked as "RESOLVED/FIXED"? I thought that the idea was that we WANTED to make this change but could NOT because it requires versioning which we will not have until the next release. flag flashplayer‑needsversioning is set to "+" so I would assume that this needs to stay OPEN and should now be targeted to "future"

Updated testcase to match current shell behavior, accept invalid UTF16 strings to decodeURI and decodeURIComponent and removed skip from testconfig.txt tr: 4481:716b92468594

Steven, this appears closed. Pls confirm.

I think it was open on the assumption we may want to make it always-strict in a future swf version. No clear call for that though.

Retargeting for "future".

(In reply to Steven Johnson from comment #35) > I think it was open on the assumption we may want to make it always-strict > in a future swf version. No clear call for that though. Closing as Resolved/Fixed. If we wants to be always strict, a new bug will be opened.