[webkit-reviews] review denied: [Bug 230929] Nullptr deref when accessing m_value.calc in CSSPrimitiveValue::primitiveType() : [Attachment 439616] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Gabriel Nava Marino
<gnavamarino at apple.com>'s request for review:
Bug 230929: Nullptr deref when accessing m_value.calc in
CSSPrimitiveValue::primitiveType()
https://bugs.webkit.org/show_bug.cgi?id=230929

Attachment 439616: Patch

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




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 439616
  --> https://bugs.webkit.org/attachment.cgi?id=439616
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.