Bug 160362 – NULL Reference Error in ElementRuleCollector

• Component: CSS
• Status: RESOLVED FIXED

[Jonathan Bedard]

The ElementRuleCollector class defines m_authorStyle as a reference.  However, in some cases, this reference is instantiated by a pointer from DocumentRuleSets, and this pointer is occasionally NULL.  This is clearly undefined behavior, although surprisingly, many of the tests which exhibit this (canvas/philip/tests/2d.strokeRect.path.html, for example) still pass.

[Jonathan Bedard]

Created attachment 284910 [details]
Patch

[Jonathan Bedard]

Note that it is possible the issue is much deeper than just this fix.

This fix only applies if it is legal for m_authorStyle to be undefined.  It seems like this should be the case, particularly since DocumentRuleSets seems to think such a case is legal, and all but one use of m_authorStyle was using the address of the reference anyways.

[Darin Adler]

Comment on attachment 284910 [details]
Patch

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

I’m not sure this change is the best fix.

Before landing this we need to understand why authorStyle is null. It doesn’t make sense because StyleResolver is the only place I see us creating a DocumentRuleSets object, and when creating it we call resetAuthorStyle. If authorStyle is null I suppose that means this is happening inside the StyleResolver constructor before resetAuthorStyle is called. If so, this can be fixed by reordering the code so it is called earlier.

In fact, I think DocumentRuleSets could probably call resetAuthorStyle inside its own constructor; there is no reason it has to wait until later. Then we could remove the explicit call to it inside the StyleResolver. And we could even change the authorStyle() function to return a reference rather than a pointer.

> Source/WebCore/css/ElementRuleCollector.cpp:299
> +        collectMatchingRulesForList(&(m_authorStyle->slottedPseudoElementRules()), matchRequest, ruleRange);

No need to add the parentheses to this expression. Please don’t add them.

[Antti Koivisto]

Another possibility is that DocumentRuleSets is dead (==StyleResolver is dead). A trivially crashing logic error here would be surprising.

[Antti Koivisto]

> undefined behavior, although surprisingly, many of the tests which exhibit
> this (canvas/philip/tests/2d.strokeRect.path.html, for example) still pass.

Null pointer dereference always crashes. How do the tests "exhibit" this?

[Jonathan Bedard]

(In reply to comment #5)
> > undefined behavior, although surprisingly, many of the tests which exhibit
> > this (canvas/philip/tests/2d.strokeRect.path.html, for example) still pass.
> 
> Null pointer dereference always crashes. How do the tests "exhibit" this?

This bug was found through open source clang's undefined behavior sanitizer.  As far as I can tell, a reference is being bound to a dereferenced NULL pointer but then this reference is never used.  If the reference were used, we would see a crash.

Tests "exhibit" this (not even sure if that's the right way to describe this) when the undefined behavior sanitizer flags the reference being bound to a dereferenced NULL pointer during runtime.

[Jonathan Bedard]

(In reply to comment #3)
> Comment on attachment 284910 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284910&action=review
> 
> I’m not sure this change is the best fix.
> 
> Before landing this we need to understand why authorStyle is null. It
> doesn’t make sense because StyleResolver is the only place I see us creating
> a DocumentRuleSets object, and when creating it we call resetAuthorStyle. If
> authorStyle is null I suppose that means this is happening inside the
> StyleResolver constructor before resetAuthorStyle is called. If so, this can
> be fixed by reordering the code so it is called earlier.
> 
> In fact, I think DocumentRuleSets could probably call resetAuthorStyle
> inside its own constructor; there is no reason it has to wait until later.
> Then we could remove the explicit call to it inside the StyleResolver. And
> we could even change the authorStyle() function to return a reference rather
> than a pointer.
> 
> > Source/WebCore/css/ElementRuleCollector.cpp:299
> > +        collectMatchingRulesForList(&(m_authorStyle->slottedPseudoElementRules()), matchRequest, ruleRange);
> 
> No need to add the parentheses to this expression. Please don’t add them.

I think your points here center around the question I had when I first encountered this bug: should the authorStyle ever be undefined?  I'm not familiar enough with the desired behavior here to answer this question.

If the author style should always be defined, then should it not be an object in the DocumentRulesSets instead of a pointer?  And, if the author style should always be defined, what about the user style, which has a very similar pattern to the author style in the DocumentRulesSets (i.e.: both are pointers, instantiated through member functions but left undefined after the constructor).  I modeled the handling of the author style in ElementRuleCollector after the way the class handles the user style.

[Jonathan Bedard]

Some more details about the circumstances when the author style is NULL:

I have been debugging this with canvas/philip.2d.strokeRect.path.html.  It is not the only test where this happens, but it does seems to be fairly representative of the very long list of tests I have seen this in.

The author style only appears to be NULL when it is the StyleResolver which calls StyleResolver::styleForElement in it's constructor.  (As Darin pointed out)

In the canvas/philip.2d.strokeRect.path.html test, the author style is defined, but not until the TreeResolver calls StyeResolver::styleForElement.

Ensuring that the author style is instantiated seems like a good approach, especially since it appears that there is a well-defined default.

[Jonathan Bedard]

In attempting to change the author style from a pointer to a reference to an object, some of the logic in ElementRuleCollector::matchSlottedPseudoElementRules(), in ElementRuleCollector.cpp line 256, seems to indicate that the author style can be NULL, and that this is a meaningful state.

[Jonathan Bedard]

Created attachment 285128 [details]
Patch

[Jonathan Bedard]

This latest patch assumes the opposite of the first, that there should always be a defined author style.

As noted in the change log, however, there is a bit of a catch with this approach: some tests need the author style to be undefined to trigger a redefinition, such as css3/masking/mask-repeat-space-border.html.  So, I added a flag so that functions can access if the current author style was created in the constructor.  This approach results in the tests which need the author style to be undefined working.

Additionally, these changes cause css3/masking/mask-repeat-space-border.html and css3/flexbox/multiline-reverse-wrap-baseline.html to pass, which they did not before.

[Darin Adler]

Comment on attachment 285128 [details]
Patch

I know you are just following my direction here, but it seems strange now to allocate an authorStyle in this peculiar case where there is "no author style defined". I think this change is OK as-is, but someone should refine this to get rid of this peculiar state. Maybe Antti has an idea how to clean this up?

[Jonathan Bedard]

I think what is happening here (although I am by no means an expert in CSS) is that if the author style is undefined for a particular element, it adopts the author style of the entire page.  However, if it tries to access the author style before adapting the author style of the page (which really shouldn't happen) then it will be accessing a NULL pointer.  As far as I can tell, what is happening in this case is the ElementRuleCollector is being instantiated before this adoption happens and then either being re-instantiated after or never using the bound NULL reference (although the behavior of the the two newly passing tests may indicate that in some circumstances ElementRuleCollector was using the NULL reference)

What this comes down to is that the old code made two competing assumptions: that the author style would never be NULL when accessed and that the author style could be NULL and would need to instantiated with the author style of the page when it was.  I'm not sure which assumption is correct.

[Jonathan Bedard]

Actually, taking a closer look at our build bots, those two tests seem to have been passing for some time, and are unrelated to this bug.

[Darin Adler]

Comment on attachment 285128 [details]
Patch

Where is the code path where we

[WebKit Commit Bot]

Comment on attachment 285128 [details]
Patch

Clearing flags on attachment: 285128

Committed r204220: <http://trac.webkit.org/changeset/204220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.