StringImpl can corrupt the static empty string

[Michelle Bell]

The following code pattern exhibits exhibits unexpected behavior:
    String s1 = "";   // internally, this gets assigned a static empty string
    String s2 = "";   // internally, this gets the same StringImpl ptr
    s1.insert("corruptme!", 0);  // this modifies the static empty StringImpl
    assert(s1 != s2);     // this assert will trip; both s1 and s2 were modified

The fix appears to be simple; the StringImpl::append() methods already do this, so it is just a matter of applying to the insert() methods.  See attached proposed patch.

[Michelle Bell]

Created attachment 11442 [details]
patch to String.cpp

[Michelle Bell]

BTW - it should be noted that once the static empty string is corrupted, any users of the empty string get the corrupted string.  Basically, everything starts failing.

[Don Gibson]

Comment on attachment 11442 [details]
patch to String.cpp

Marking r? for attention

[Don Gibson]

Confirming.  Another failing code sequence (as alluded to in comment 2) is

String s1 = "";
s1.insert("corrupted", 0);
String s2 = "";
assert(s2.empty());

[Alexey Proskuryakov]

Comment on attachment 11442 [details]
patch to String.cpp

This looks like a design problem to me. According to a comment above the class declaration, Strings are supposed to be shared - so, the fact that append() implicitly copies (unlike insert(), replace() and truncate()) is formally a bug, and the constructors that initialize m_impl to StringImpl::empty() are incorrect...

[Maciej Stachowiak]

Originally the class that used to be String was used in a very inconsistent way, but our current intent is that StringImpls should be shareable and effectively immutable so that two Strings with the same value will not both be modified if one is mutated. However, it's uncertain whether any code is still dependent on the old semantics of insert.

[Mark Rowe (bdash)]

I think this will be fixed by the patch in bug 16550.

[Darin Adler]

Fixed by r28976.