Attachment 361782 Details for Bug 194530 – proposed patch.

[patch] proposed patch.

bug-194530.patch (text/plain), 5.01 KB, created by Mark Lam on 2019-02-12 01:15:45 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-12 01:15:45 PST

Size: 5.01 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 241288)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,45 @@
>+2019-02-12  Mark Lam  <mark.lam@apple.com>
>+
>+        Add some null checks in JSNodeCustom.h's root() and generated isReachableFromOpaqueRoots() functions.
>+        https://bugs.webkit.org/show_bug.cgi?id=194530
>+        <rdar://problem/47973274>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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 following null checks:
>+
>+        1. Add a null check in JSNodeCustom.h's root().
>+
>+           root() is called from a isReachableFromOpaqueRoots() generated by CodeGeneratorJS.pm.
>+           isReachableFromOpaqueRoots() calls a ownerNode() method and passes its result
>+           to root().  However, depending on which class the ownerNode() method belongs to,
>+           it can either return a pointer or a reference.  The null check only makes sense
>+           in the pointer case.
>+
>+           To accommodate the 2 forms, root() itself is has an overload that takes a
>+           reference instead of a pointer.
>+
>+           Since CodeGeneratorJS.pm can't tell what the generated class' ownerNode()
>+           returns, it can't discern when the result is a pointer and apply the null check.
>+           Instead, we just add the null check to the version of root() that takes a
>+           pointer.  If the node pointer is null, we'll return a null opaque root.
>+
>+        2. Fix CodeGeneratorJS.pm to null check the opaque root before using it.
>+
>+        * bindings/js/JSNodeCustom.h:
>+        (WebCore::root):
>+        * bindings/scripts/CodeGeneratorJS.pm:
>+        (GenerateImplementation):
>+        * bindings/scripts/test/JS/JSTestGenerateIsReachable.cpp:
>+        (WebCore::JSTestGenerateIsReachableOwner::isReachableFromOpaqueRoots):
>+
> 2019-02-11  Myles C. Maxfield  <mmaxfield@apple.com>
> 
>         [Cocoa] Ask platform for generic font family mappings
>Index: Source/WebCore/bindings/js/JSNodeCustom.h
>===================================================================
>--- Source/WebCore/bindings/js/JSNodeCustom.h	(revision 241288)
>+++ Source/WebCore/bindings/js/JSNodeCustom.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2007, 2009, 2010 Apple Inc. All rights reserved.
>+ * Copyright (C) 2007-2019 Apple Inc. All rights reserved.
>  * Copyright (C) 2018 Yusuke Suzuki <utatane.tea@gmail.com>.
>  *
>  * Redistribution and use in source and binary forms, with or without
>@@ -80,7 +80,7 @@ inline void willCreatePossiblyOrphanedTr
> 
> inline void* root(Node* node)
> {
>-    return node->opaqueRoot();
>+    return node ? node->opaqueRoot() : nullptr;
> }
> 
> inline void* root(Node& node)
>Index: Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
>===================================================================
>--- Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	(revision 241288)
>+++ Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	(working copy)
>@@ -3,7 +3,7 @@
> # Copyright (C) 2006 Anders Carlsson <andersca@mac.com>
> # Copyright (C) 2006, 2007 Samuel Weinig <sam@webkit.org>
> # Copyright (C) 2006 Alexey Proskuryakov <ap@webkit.org>
>-# Copyright (C) 2006-2018 Apple Inc. All rights reserved.
>+# Copyright (C) 2006-2019 Apple Inc. All rights reserved.
> # Copyright (C) 2009 Cameron McCormack <cam@mcc.id.au>
> # Copyright (C) Research In Motion Limited 2010. All rights reserved.
> # Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
>@@ -4679,7 +4679,7 @@ sub GenerateImplementation
>             }
> 
>             push(@implContent, $rootString);
>-            push(@implContent, "    return visitor.containsOpaqueRoot(root);\n");
>+            push(@implContent, "    return root && visitor.containsOpaqueRoot(root);\n");
>         } else {
>             if (!$emittedJSCast) {
>                 push(@implContent, "    UNUSED_PARAM(handle);\n");
>Index: Source/WebCore/bindings/scripts/test/JS/JSTestGenerateIsReachable.cpp
>===================================================================
>--- Source/WebCore/bindings/scripts/test/JS/JSTestGenerateIsReachable.cpp	(revision 241288)
>+++ Source/WebCore/bindings/scripts/test/JS/JSTestGenerateIsReachable.cpp	(working copy)
>@@ -204,7 +204,7 @@ bool JSTestGenerateIsReachableOwner::isR
>     TestGenerateIsReachable* root = &jsTestGenerateIsReachable->wrapped();
>     if (UNLIKELY(reason))
>         *reason = "Reachable from TestGenerateIsReachable";
>-    return visitor.containsOpaqueRoot(root);
>+    return root && visitor.containsOpaqueRoot(root);
> }
> 
> void JSTestGenerateIsReachableOwner::finalize(JSC::Handle<JSC::Unknown> handle, void* context)

Flags:

cdumez: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 194530: 361781 | 361782 | 361788