If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Regression: String operations on a string that ends with a partial surrogate character can result in a crash

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Werner Sharp, Assigned: Werner Sharp)

Tracking

unspecified
flash10.1
Bug Flags:
in-testsuite +
flashplayer-qrb +
flashplayer-triage +

Details

(Whiteboard: Has patch)

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
Constructing a StUTF8String object from a UTF-16 character that ends in a partial surrogate character will result in a call to GC::Alloc with a size of zero. This will crash on release builds.

This is a regression -- crashes in 10.1 but not in 10.0.45.

At line 2697 of StUTF8String::StUTF8String, when it calls UnicodeUtils::Utf16ToUtf8, it does not handle the case where UnicodeUtils::Utf16ToUtf8 returns -1. It will do so if the str parameter ends with half of a surrogate pair.

Partial surrogate pairs may be produced when deleting characters. See bug 2522195.

Bug Group bug 2562110 contains a simple test app that will allow this bug to be reproduced. It also has instructions on how to enter surrogate characters and generate the right conditions for this bug to occur.  See also sample code below for a programmatic crasher case.

var s:String = "abc";
var badChar:String = String.fromCharCode(0xD801);
s += badChar;
s.replace(/\uD801/, "\\uD801");

player bug #2564537
(Assignee)

Updated

8 years ago
Assignee: nobody → wsharp
OS: Windows Vista → All
Hardware: x86 → All
Target Milestone: --- → flash10.1

Comment 1

8 years ago
(In reply to comment #0)
> Constructing a StUTF8String object from a UTF-16 character that ends in a
> partial surrogate character will result in a call to GC::Alloc with a size of
> zero. This will crash on release builds.

I have suspected for some time that making zero-size allocations illegal - which is an explicit choice - is probably an API bug in MMgc, but I don't know if we want to fix that API right now or just work around this problem.
(Assignee)

Comment 2

8 years ago
Prior implementation just converted the -1 length to 0.

		if( utf8len < 0 )
		{
			utf8len = 0;
		}

I was just planning to do the same code in the current code:

	len = UnicodeUtils::Utf16ToUtf8(String::Pointers(str).p16, str->length(), NULL, 0);
	if (len < 0)
		len = 0;
	char* dstBuf = (char*) gc->Alloc(uint32_t(len)+1, 0);
(Assignee)

Comment 3

8 years ago
Created attachment 429831 [details] [diff] [review]
len < 0 patch
Attachment #429831 - Flags: review?(stejohns)

Updated

8 years ago
Attachment #429831 - Flags: review?(stejohns) → review+

Comment 4

8 years ago
Comment on attachment 429831 [details] [diff] [review]
len < 0 patch

Have we checked all the other calls to Utf16toUtf8 to see if they also check for negative results?
(Assignee)

Comment 5

8 years ago
This is the only case.   We call it to get the length then we call it again with our buffer.

Updated

8 years ago
Flags: flashplayer-qrb+

Updated

8 years ago
Status: NEW → ASSIGNED
Priority: -- → P2

Comment 6

8 years ago
can this land?

Updated

8 years ago
Whiteboard: Has patch

Comment 7

8 years ago
tamarin-redux changeset 4015:03d093d2cf20
tr-argo changeset 3814:b28ad909f055
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 8

8 years ago
Created attachment 433580 [details] [diff] [review]
testcase
Attachment #433580 - Flags: review?(dschaffe)

Updated

8 years ago
Attachment #433580 - Flags: review?(dschaffe) → review+

Comment 9

8 years ago
Comment on attachment 433580 [details] [diff] [review]
testcase

testcase pushed
tr-argo -> 3873:8265b24fbef7
tr -> 4179:8265b24fbef7

Updated

8 years ago
Status: RESOLVED → VERIFIED
Flags: in-testsuite+
Flags: flashplayer-triage+
You need to log in before you can comment on or make changes to this bug.