Bug 194530 – Add some null checks in JSNodeCustom.h's root() and generated isReachableFromOpaqueRoots() functions.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mark Lam]

This is needed to fix a null pointer dereference that arises from the following scenario:
1. a Document detaches from its StyleSheetList.
2. the JSStyleSheetList that is associate with the detached StyleSheetList has yet to be scanned and collected by the GC.
3. the GC eventually looks for the opaque root of the StyleSheetList's owner, and discovers a null owner pointer.

This patch fixes this issue by applying the needed null checks.

<rdar://problem/47973274>

[Mark Lam]

Created attachment 361781 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 361781 [details]
proposed patch.

Attachment 361781 [details] did not pass bindings-ews (mac):
Output: https://webkit-queues.webkit.org/results/11119707

New failing tests:
(JS) JSTestGenerateIsReachable.cpp

[Mark Lam]

Created attachment 361782 [details]
proposed patch.

[EWS Watchlist]

Comment on attachment 361782 [details]
proposed patch.

Attachment 361782 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11120135

New failing tests:
http/tests/inspector/network/resource-initiatorNode.html

[EWS Watchlist]

Created attachment 361788 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Mark Lam]

(In reply to Build Bot from comment #4)
> Comment on attachment 361782 [details]
> proposed patch.
> 
> Attachment 361782 [details] did not pass mac-debug-ews (mac):
> Output: https://webkit-queues.webkit.org/results/11120135
> 
> New failing tests:
> http/tests/inspector/network/resource-initiatorNode.html

This cannot be due to this patch.  All I added are null checks.  If the test is not working with nullptrs in the affected areas, the code's behavior will be unchanged.  If the test is working with nullptrs in the affected areas, it would;d have crashed.

Also, I ran the test locally and it passed without any issues.

[Chris Dumez]

Comment on attachment 361782 [details]
proposed patch.

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

> Source/WebCore/ChangeLog:11
> +        2. the JSStyleSheetList that is associate with the detached StyleSheetList has yet

associated

[Mark Lam]

Thanks for the review.  I've fixed the ChangeLog typo.

Landed in r241300: <http://trac.webkit.org/r241300>.

[Darin Adler]

Comment on attachment 361782 [details]
proposed patch.

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4682
> +            push(@implContent, "    return root && visitor.containsOpaqueRoot(root);\n");

This change doesn’t seem right to me. The cases above that can be null already have null checks, don’t they? We could remove those null checks if we want to keep this one, I suppose.

[Mark Lam]

Comment on attachment 361782 [details]
proposed patch.

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

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4682
>> +            push(@implContent, "    return root && visitor.containsOpaqueRoot(root);\n");
> 
> This change doesn’t seem right to me. The cases above that can be null already have null checks, don’t they? We could remove those null checks if we want to keep this one, I suppose.

Here's the crashing function before this fix was applied:

    bool JSStyleSheetListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor, const char** reason)
    {
        auto* jsStyleSheetList = jsCast<JSStyleSheetList*>(handle.slot()->asCell());
        void* root = WebCore::root(jsStyleSheetList->wrapped().ownerNode());  // <===== crashed here !!!
        if (UNLIKELY(reason))
            *reason = "Reachable from StyleSheetList ownerNode";
        return visitor.containsOpaqueRoot(root);
    }

The crash happened because jsStyleSheetList->wrapped().ownerNode() is null.  As a result, WebCore::root() executes node->opaqueRoot() with a null node.  Hence, the fix in root() in JSNodeCustom.h (above) is necessary.

With the fix in root(), the returned root pointer can be null.  While calling visitor.containsOpaqueRoot(root) with a null root pointer should not crash, it will always return false because we should never have null as an opaque root.  The addition of "root && " here saves us from having to do a has look up that is guaranteed to fail.  Hence, this is part of the change is not strictly needed for correctness, but is simply an optimization.

On the other hand, we would normally expect the root pointer to not be null.  So, I'm optimizing for the unlikely case here.  I'll land a follow up to remove this second  null check.

[Mark Lam]

(In reply to Mark Lam from comment #10)
> I'll land a follow up to remove this second  null check.

Addressing this in https://bugs.webkit.org/show_bug.cgi?id=194581.

[Darin Adler]

Comment on attachment 361782 [details]
proposed patch.

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

>>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4682
>>> +            push(@implContent, "    return root && visitor.containsOpaqueRoot(root);\n");
>> 
>> This change doesn’t seem right to me. The cases above that can be null already have null checks, don’t they? We could remove those null checks if we want to keep this one, I suppose.
> 
> Here's the crashing function before this fix was applied:
> 
>     bool JSStyleSheetListOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor, const char** reason)
>     {
>         auto* jsStyleSheetList = jsCast<JSStyleSheetList*>(handle.slot()->asCell());
>         void* root = WebCore::root(jsStyleSheetList->wrapped().ownerNode());  // <===== crashed here !!!
>         if (UNLIKELY(reason))
>             *reason = "Reachable from StyleSheetList ownerNode";
>         return visitor.containsOpaqueRoot(root);
>     }
> 
> The crash happened because jsStyleSheetList->wrapped().ownerNode() is null.  As a result, WebCore::root() executes node->opaqueRoot() with a null node.  Hence, the fix in root() in JSNodeCustom.h (above) is necessary.
> 
> With the fix in root(), the returned root pointer can be null.  While calling visitor.containsOpaqueRoot(root) with a null root pointer should not crash, it will always return false because we should never have null as an opaque root.  The addition of "root && " here saves us from having to do a has look up that is guaranteed to fail.  Hence, this is part of the change is not strictly needed for correctness, but is simply an optimization.
> 
> On the other hand, we would normally expect the root pointer to not be null.  So, I'm optimizing for the unlikely case here.  I'll land a follow up to remove this second  null check.

Yes, because the null check is missing from the ImplOwnerNodeRoot case above. The other cases above have null checks as needed, for example, the one in ImplDocument. Adding a null check here in the common code is not what I would have done; I would have either followed the pattern or moved the null check here and removed the now-redundant ones above like the one in ImplDocument.

[Darin Adler]

(In reply to Mark Lam from comment #11)
> (In reply to Mark Lam from comment #10)
> > I'll land a follow up to remove this second  null check.
> 
> Addressing this in https://bugs.webkit.org/show_bug.cgi?id=194581.

Great!