Bug 20069 – CSSPrimitiveValue::parserValue() returns deleted memory

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Alexey Proskuryakov]

<rdar://problem/7725534>

Created attachment 50602 [details]
Change valueOrPropertyName() to return an AtomicString from a static table

[Alexey Proskuryakov]

You probably could have added a test case with a "checkConsistency"-style assertion.

Fixed in <http://trac.webkit.org/projects/webkit/changeset/55914>.