Bug 11555 - StringImpl can corrupt the static empty string
Summary: StringImpl can corrupt the static empty string
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 420+
Hardware: PC OS X 10.4
: P2 Normal
Assignee: Michelle Bell
Depends on:
Reported: 2006-11-09 10:47 PST by Michelle Bell
Modified: 2007-12-24 17:06 PST (History)
3 users (show)

See Also:

patch to String.cpp (681 bytes, patch)
2006-11-09 10:48 PST, Michelle Bell
ap: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michelle Bell 2006-11-09 10:47:33 PST
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.
Comment 1 Michelle Bell 2006-11-09 10:48:44 PST
Created attachment 11442 [details]
patch to String.cpp
Comment 2 Michelle Bell 2006-11-09 10:50:21 PST
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.
Comment 3 Don Gibson 2006-11-09 11:21:10 PST
Comment on attachment 11442 [details]
patch to String.cpp

Marking r? for attention
Comment 4 Don Gibson 2006-11-09 11:23:37 PST
Confirming.  Another failing code sequence (as alluded to in comment 2) is

String s1 = "";
s1.insert("corrupted", 0);
String s2 = "";
Comment 5 Alexey Proskuryakov 2006-11-09 11:43:35 PST
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...
Comment 6 Maciej Stachowiak 2006-11-10 10:18:06 PST
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.
Comment 7 Mark Rowe (bdash) 2007-12-22 02:31:31 PST
I think this will be fixed by the patch in bug 16550.
Comment 8 Darin Adler 2007-12-24 17:06:41 PST
Fixed by r28976.