473480 - String::createDynamic can attempt a zero-byte GC allocation, which is not legal

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Description

[Steven Johnson]

Attachment: Patch

This can happen if you call toLowerCase() or toUpperCase() on an empty string; it might be possible in other situations as well. Enclosed is a patch for the known case. Michael, you should probably examine the other possible cases to verify other misbehavior isn't present.

Comment 1

[Michael Daumling]

Comment on attachment 356849 [details] [diff] [review]
Patch

I would remove the assert. It may be legal to attempt to allocate zero bytes - there are tons of possibilities where this may happen, and you fix the problem later in a correct way anyway.

Comment 2

[Steven Johnson]

It is never legal to request a zero-byte allocation from MMGC; it will assert as well, further down. This just pushes the assertion further up the well. (That's why I requested you examine the other uses to avoid zero-byte allocations)

Comment 3

[Michael Daumling]

It is legal to request a zero-length dynamic string. There are numerous occasions where this may happen, for example if someone calls getFixedWidthString() on core->kEmptyString with a width of 16 bits. Since such a string may be created and later appended to, the fix that you entered (if # of bytes == 0, make it 1), makes perfect sense, but the assert does not. There will not be any assert in MMgc because of your fix.

Even if I made sure that no other code is able to enter createDynamic() with a zero length, I do not think that it makes sense to add that much housekeeping code if the solution is to simply allocate 1 character.

Comment 4

[Steven Johnson]

Ah, ok, I misunderstood. 0-len dynamic string == fine, 0-len gc allocation == not fine. I'll revisit the patch.

Comment 5

[Steven Johnson]

Attachment: Patch #2

Updated patch per Michael's suggestion.

Comment 6

[Michael Daumling]

Comment on attachment 357000 [details] [diff] [review]
Patch #2

Go for it.

Comment 7

[Steven Johnson]

pushed to redux as changeset:   1298:69526ecaf558

Comment 8

[Chris Peyer]

Resolved fixed engineering / work item that has been pushed.  Setting status to verified.