REGRESSION (r224592): oss-fuzz: jsc: Null-dereference READ in JSC::JSCell::isObject

[Robin Morisset]

<rdar://problem/35550513>, this problem has been found by David Kilzer through fuzzing.
The bug was exposed by a change in r224592 (the addition of phantomLocalDirect(virtualRegisterForArgument(0)) in flush) but is not directly related otherwise.
The root cause of the bug was found by Saam Barati: when doing an OSR enter, |this| would be assumed to be a valid, non-null cell. This would then lead to the removal of tdz_check, making the next operation (pushByVal in this case) dereference the null value.

[Robin Morisset]

Created attachment 327057 [details]
Patch

[Robin Morisset]

Created attachment 327058 [details]
Patch

[Build Bot]

Attachment 327058 [details] did not pass style-queue:


ERROR: JSTests/ChangeLog:9:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: fuzzer  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Radar WebKit Bug Importer]

<rdar://problem/35587643>

[Keith Miller]

Comment on attachment 327058 [details]
Patch

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

r=me.

> Source/JavaScriptCore/ChangeLog:3
> +        REGRESSION (r224592): oss-fuzz: jsc: Null-dereference READ in JSC::JSCell::isObject (4216)

We can probably drop the line number.

[Robin Morisset]

Comment on attachment 327058 [details]
Patch

>Subversion Revision: 224677
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index acff454706f86bf6a95ba92f35d1fa8972a0680b..613051cc3feaf51da6d4940d7083102ff3160d7f 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,20 @@
>[phone]  Robin Morisset  <[e-mail]>
>+
>+        REGRESSION (r224592): oss-fuzz: jsc: Null-dereference READ in JSC::JSCell::isObject
>+        https://bugs.webkit.org/show_bug.cgi?id=179763
>+        <rdar://problem/35550513>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix null pointer dereference caused by an eliminated tdz_check
>+
>+        The problem was when doing an OSR entry in DFG while |this| was null
>+        (because super() had not yet been called in the constructor of this
>+        subclass), it would be marked as non-null, and the tdz_check eliminated.
>+
>+        * dfg/DFGInPlaceAbstractState.cpp:
>+        (JSC::DFG::InPlaceAbstractState::initialize):
>+
> 2017-11-09  Yusuke Suzuki  <[e-mail]>
> 
>         [JSC] Retry module fetching if previous request fails
>diff --git a/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp b/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>index b49cb4cd96234a9271ccab82c4e678ba93d78243..8d84990bdb56a83de86916d91373725c6b7c66dc 100644
>--- a/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGInPlaceAbstractState.cpp
>@@ -129,7 +129,7 @@ void InPlaceAbstractState::initialize()
>                     entrypoint->valuesAtHead.argument(i).setType(SpecBoolean);
>                     break;
>                 case FlushedCell:
>-                    entrypoint->valuesAtHead.argument(i).setType(m_graph, SpecCell);
>+                    entrypoint->valuesAtHead.argument(i).setType(m_graph, SpecCellCheck);
>                     break;
>                 case FlushedJSValue:
>                     entrypoint->valuesAtHead.argument(i).makeBytecodeTop();
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 584a9a701226e1fab5e870400615cf4f31097d32..c9a4a34ee4e6b378ccc943a53fdeea24aee497b1 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,17 @@
>[phone]  Robin Morisset  <[e-mail]>
>+
>+        REGRESSION (r224592): oss-fuzz: jsc: Null-dereference READ in JSC::JSCell::isObject
>+        https://bugs.webkit.org/show_bug.cgi?id=179763
>+        <rdar://problem/35550513>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Just adding a slightly cleaned-up version of the original fuzzer-found test.
>+
>+        * stress/tdz-this-in-try-catch.js: Added.
>+        (__v_6388):
>+        (__v_6392):
>+
> 2017-11-08  Saam Barati  <[e-mail]>
> 
>         A JSFunction's ObjectAllocationProfile should watch the poly prototype watchpoint so it can clear its object allocation profile
>diff --git a/JSTests/stress/tdz-this-in-try-catch.js b/JSTests/stress/tdz-this-in-try-catch.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..89f22baf572df31e68cfd463d1491567df03edef
>--- /dev/null
>+++ b/JSTests/stress/tdz-this-in-try-catch.js
>@@ -0,0 +1,22 @@
>+var __v_6388 = class __c_95 {
>+};
>+var __v_6392 = class __c_97 extends __v_6388 {
>+  constructor() {
>+    var __v_6407 = () => {
>+        try {
>+          __v_6386();
>+        } catch (e) {}
>+        try {
>+          super.foo = 'q';
>+        } catch (e) {}
>+        super()
>+        try {
>+          this.idValue
>+        } catch (e) {}
>+    };
>+    __v_6407();
>+  }
>+};
>+for (var i = 0; i < 1000; ++i) {
>+    new __v_6392()
>+}

[Robin Morisset]

I just removed the line number from the Changelog, as suggested by Keith.

[WebKit Commit Bot]

Comment on attachment 327058 [details]
Patch

Clearing flags on attachment: 327058

Committed r224915: <https://trac.webkit.org/changeset/224915>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

<rdar://problem/35550513>