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

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P2)

Description

[Werner Sharp]

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

Comment 1

[Lars T Hansen]

(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.

Comment 2

[Werner Sharp]

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

Comment 3

[Werner Sharp]

Attachment: len < 0 patch

Comment 4

[Steven Johnson]

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?

Comment 5

[Werner Sharp]

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

Comment 6

[Trevor Baker]

can this land?

Comment 7

[Rob Winchell]

tamarin-redux changeset 4015:03d093d2cf20
tr-argo changeset 3814:b28ad909f055

Comment 8

[Brent Baker]

Attachment: testcase

Comment 9

[Brent Baker]

Comment on attachment 433580 [details] [diff] [review]
testcase

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