Bug 230929 – Nullptr deref when accessing m_value.calc in CSSPrimitiveValue::primitiveType()

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Gabriel Nava Marino]

We should check for m_value.calc existence before dereferencing, as is currently done in other places in CSSPrimitiveValue class.

[Gabriel Nava Marino]

Created attachment 439616 [details]
Patch

[Darin Adler]

Comment on attachment 439616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439616&action=review

> Source/WebCore/ChangeLog:4
> +        Nullptr deref when accessing m_value.calc in CSSPrimitiveValue::primitiveType()
> +        https://bugs.webkit.org/show_bug.cgi?id=230929

Overall this doesn’t look right. Type set to CSS_CALC, but calc pointer set to nullptr is an invalid state, and changing all the functions to support that isn’t the correct solution.

Instead I suggest we focus on how we end up in this state and preventing us from doing so.

> Source/WebCore/css/CSSPrimitiveValue.cpp:436
> -    m_value.calc = c.leakRef();
> +    if (c)
> +        m_value.calc = c.leakRef();

This specific change is either is a no-op or seems pretty dangerous. It will leave m_value.calc uninitialized if this is called with nullptr. That’s not *better* than setting it to nullptr.

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #2)
> Comment on attachment 439616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439616&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        Nullptr deref when accessing m_value.calc in CSSPrimitiveValue::primitiveType()
> > +        https://bugs.webkit.org/show_bug.cgi?id=230929
> 
> Overall this doesn’t look right. Type set to CSS_CALC, but calc pointer set
> to nullptr is an invalid state, and changing all the functions to support
> that isn’t the correct solution.

Thank you for pointing this out. After taking a closer look, I believe we can prevent this state (see below) by how we handle blending from LengthType::FitContent.

> 
> Instead I suggest we focus on how we end up in this state and preventing us
> from doing so.

We end up in this bad state as a result of trying to blend from a FitContent LengthType to a Calculated LengthType as a result of the div animation.

The attempt to create a CSSCalcExpressionNode for LengthType::FitContent is not supported as evidenced by the switch statement falling through to a ASSERT_NOT_REACHED().

I will see if we can prevent this blending action much higher upstream before reaching this code.

> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:436
> > -    m_value.calc = c.leakRef();
> > +    if (c)
> > +        m_value.calc = c.leakRef();
> 
> This specific change is either is a no-op or seems pretty dangerous. It will
> leave m_value.calc uninitialized if this is called with nullptr. That’s not
> *better* than setting it to nullptr.

[Darin Adler]

Comment on attachment 439616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439616&action=review

>>> Source/WebCore/css/CSSPrimitiveValue.cpp:436
>>> +        m_value.calc = c.leakRef();
>> 
>> This specific change is either is a no-op or seems pretty dangerous. It will leave m_value.calc uninitialized if this is called with nullptr. That’s not *better* than setting it to nullptr.
> 
> 

Thinking further about this, even if we fix the true cause of the null CSSCalcValue, I suggest we also change this init function to either take Ref<CSSCalcValue> instead of RefPtr<CSSCalcValue>, like all the other init functions, or change it so that if it’s passed nullptr it returns early and does nothing, leaving the primitive unit type set to the default CSS_UNKNOWN.

[Gabriel Nava Marino]

Created attachment 439936 [details]
Patch

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #4)
> Comment on attachment 439616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439616&action=review
> 
> >>> Source/WebCore/css/CSSPrimitiveValue.cpp:436
> >>> +        m_value.calc = c.leakRef();
> >> 
> >> This specific change is either is a no-op or seems pretty dangerous. It will leave m_value.calc uninitialized if this is called with nullptr. That’s not *better* than setting it to nullptr.
> > 
> > 
> 
> Thinking further about this, even if we fix the true cause of the null
> CSSCalcValue, I suggest we also change this init function to either take
> Ref<CSSCalcValue> instead of RefPtr<CSSCalcValue>, like all the other init
> functions, or change it so that if it’s passed nullptr it returns early and
> does nothing, leaving the primitive unit type set to the default CSS_UNKNOWN.


Thank you for the suggestions! I will proceed with the second recommended approach to fix this particular bug, by returning early if we have a nullptr. However, I have filed https://bugs.webkit.org/show_bug.cgi?id=231111 to track the first approach.

I have also verified the updated patch resolves the crash referenced in this bug.

[Darin Adler]

Comment on attachment 439936 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=439936&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:435
> +    // TODO(webrtc:231111): this init should take Ref<CSSCalcValue> instead

This comment is not yet the right format for WebKit coding style:

1) We use FIXME, not TODO.
2) Not sure what "webrtc:" is about, OK to put a bug number in though.
3) We use sentence style, with a capital letter at the start, and a period at the end.

[Gabriel Nava Marino]

Created attachment 440100 [details]
Patch

[Gabriel Nava Marino]

(In reply to Darin Adler from comment #7)
> Comment on attachment 439936 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=439936&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:435
> > +    // TODO(webrtc:231111): this init should take Ref<CSSCalcValue> instead
> 
> This comment is not yet the right format for WebKit coding style:
> 
> 1) We use FIXME, not TODO.
> 2) Not sure what "webrtc:" is about, OK to put a bug number in though.
> 3) We use sentence style, with a capital letter at the start, and a period
> at the end.

Thank you, I have updated the format per the recommended style.

[EWS]

Committed r283562 (242527@main): <https://commits.webkit.org/242527@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440100 [details].

[Radar WebKit Bug Importer]

<rdar://problem/83895771>