Bug 20069 - CSSPrimitiveValue::parserValue() returns deleted memory
Summary: CSSPrimitiveValue::parserValue() returns deleted memory
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2008-07-16 20:11 PDT by Eric Seidel (no email)
Modified: 2010-03-12 10:29 PST (History)
3 users (show)

See Also:


Attachments
Change valueOrPropertyName() to return an AtomicString from a static table (3.12 KB, patch)
2010-03-12 09:12 PST, mitz
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2008-07-16 20:11:20 PDT
CSSParserValue CSSPrimitiveValue::parserValue() const:

        case CSS_IDENT: {
            value.id = m_value.ident;
            String name = valueOrPropertyName(m_value.ident);
            value.string.characters = const_cast<UChar*>(name.characters());
            value.string.length = name.length();
            break;
        }

This function returns the resulting "value" which has a weak pointer to the UChar buffer which must have been freed when the String went out of scope.

I discovered this will trying to remove callers of StringImpl::characters() (see bug 20065) so that we can play around with using different storage techniques for StringImpl's data.


The only code which ever uses this is:
void CSSStyleSelector::addMatchedDeclaration(CSSMutableStyleDeclaration* decl)

Which I'm not sure what it even does.  It seems to be used for variable resolution?  Maybe for dealing with inline styles?  Unclear.

Perhaps Hyatt can explain.  Once we know how this code is used, it should be easy to produce a test case which will crash under MallocScribble.
Comment 1 Eric Seidel (no email) 2008-07-16 20:12:36 PDT
In case you're wondering, valueOrPropertyName returns a char* which is transparently copied to create a String:

static const char* valueOrPropertyName(int valueOrPropertyID)
{
    if (const char* valueName = getValueName(valueOrPropertyID))
        return valueName;
    return getPropertyName(static_cast<CSSPropertyID>(valueOrPropertyID));
}

Seems sad that it would be copied. :(
Comment 2 Alexey Proskuryakov 2010-03-06 16:05:15 PST
<rdar://problem/7725534>
Comment 3 mitz 2010-03-12 09:12:53 PST
Created attachment 50602 [details]
Change valueOrPropertyName() to return an AtomicString from a static table
Comment 4 Alexey Proskuryakov 2010-03-12 10:20:20 PST
You probably could have added a test case with a "checkConsistency"-style assertion.
Comment 5 mitz 2010-03-12 10:29:45 PST
Fixed in <http://trac.webkit.org/projects/webkit/changeset/55914>.