Bug 230929 - Nullptr deref when accessing m_value.calc in CSSPrimitiveValue::primitiveType()
Summary: Nullptr deref when accessing m_value.calc in CSSPrimitiveValue::primitiveType()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-09-28 18:35 PDT by Gabriel Nava Marino
Modified: 2021-10-05 11:28 PDT (History)
11 users (show)

See Also:


Attachments
Patch (3.70 KB, patch)
2021-09-29 09:43 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2021-10-01 17:04 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2021-10-04 13:37 PDT, Gabriel Nava Marino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gabriel Nava Marino 2021-09-28 18:35:22 PDT
We should check for m_value.calc existence before dereferencing, as is currently done in other places in CSSPrimitiveValue class.
Comment 1 Gabriel Nava Marino 2021-09-29 09:43:49 PDT
Created attachment 439616 [details]
Patch
Comment 2 Darin Adler 2021-09-29 15:16:56 PDT
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.
Comment 3 Gabriel Nava Marino 2021-09-29 17:16:54 PDT
(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.
Comment 4 Darin Adler 2021-09-29 18:26:46 PDT
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.
Comment 5 Gabriel Nava Marino 2021-10-01 17:04:45 PDT
Created attachment 439936 [details]
Patch
Comment 6 Gabriel Nava Marino 2021-10-01 17:05:07 PDT
(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.
Comment 7 Darin Adler 2021-10-01 17:17:09 PDT
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.
Comment 8 Gabriel Nava Marino 2021-10-04 13:37:29 PDT
Created attachment 440100 [details]
Patch
Comment 9 Gabriel Nava Marino 2021-10-04 13:39:53 PDT
(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.
Comment 10 EWS 2021-10-05 11:27:00 PDT
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].
Comment 11 Radar WebKit Bug Importer 2021-10-05 11:28:17 PDT
<rdar://problem/83895771>