Bug 160870 - Member call on NULL pointer in JavaScriptCore/dfg/DFGAbstractInterpretterInlines.h
Summary: Member call on NULL pointer in JavaScriptCore/dfg/DFGAbstractInterpretterInli...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-15 15:28 PDT by Jonathan Bedard
Modified: 2016-09-06 16:53 PDT (History)
15 users (show)

See Also:


Attachments
Patch (1.73 KB, patch)
2016-08-15 15:45 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (1.75 KB, patch)
2016-08-17 11:53 PDT, Jonathan Bedard
sbarati: review-
Details | Formatted Diff | Diff
patch (2.05 KB, patch)
2016-09-02 15:37 PDT, Saam Barati
no flags Details | Formatted Diff | Diff
patch (2.04 KB, patch)
2016-09-02 15:43 PDT, Saam Barati
darin: review+
Details | Formatted Diff | Diff
patch for landing (2.04 KB, patch)
2016-09-06 16:10 PDT, Saam Barati
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2016-08-15 15:28:14 PDT
In a few cases (js/regress/simple-regexp-exec-folding.html, for example) forAllTransitiveIncomingValues in PhiChildren (JavaScriptCore/dfg/DFGPhiChildren.h) is called from a NULL pointer.  Because of the current implementation of forAllTransitiveIncomingValues, this does not currently cause a crash.  It is, however, an obvious bug, especially because in another case, the caller checks the PhiChildren pointer before calling this function.

NULL pointer: JavaScriptCore/dfg/DFGAbstractInterpretterInlines.h line 1997
Analogous call: JavaScriptCore/dfg/DFGAbstractInterpretterInlines.h line 2273
Comment 1 Jonathan Bedard 2016-08-15 15:45:13 PDT
Created attachment 286102 [details]
Patch
Comment 2 Jonathan Bedard 2016-08-15 15:46:05 PDT
This patch will fail style check, since it contains an edit to a Lambda function.
Comment 3 WebKit Commit Bot 2016-08-15 15:48:07 PDT
Attachment 286102 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2000:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Darin Adler 2016-08-15 23:31:26 PDT
Comment on attachment 286102 [details]
Patch

Needs a test case.
Comment 5 Saam Barati 2016-08-16 11:55:30 PDT
Comment on attachment 286102 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): NULL check on m_phiChildren before function call.

Can you elaborate on why this happens?
Comment 6 Jonathan Bedard 2016-08-16 15:18:18 PDT
I will revisit this bug tomorrow.  Changes in the last 24 hours have resulted in my inability to duplicate it.  I will update my undefined behavior sanitizer and see if I can replicate this bug (unfortunately, the undefined behavior sanitizer takes hours to build if top-level headers have been changed)
Comment 7 Jonathan Bedard 2016-08-17 11:53:55 PDT
Created attachment 286310 [details]
Patch
Comment 8 WebKit Commit Bot 2016-08-17 11:55:18 PDT
Attachment 286310 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2000:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 Jonathan Bedard 2016-08-17 12:02:09 PDT
A quick update on Daren's request: This is going to be a very difficult change to test.  The most obvious way would be to integrate undefined behavior sanitizer into our testing infrastructure, although this is many months off if it will happen at all.

The other method of testing would be construct a test which would crash without this change.  While this is likely possible, it's unclear to me what such a test would like like.  While attempting to construct a test which would crash without this change, I discovered that it really only seems to be js/regress/simple-regexp-exec-folding.html which exhibits the bug, but even this test will not always exhibit this error (most notably, is the number of iterations through the loop is decreased, the error will no longer occur).

If uncovering the precise code path which triggers this bug is important, I can continue to investigate.  However, I don't think continued investigation is worthwhile, as an analogous case in forAllTransitiveIncomingValues preforms this check.
Comment 10 Darin Adler 2016-08-20 19:16:16 PDT
Comment on attachment 286310 [details]
Patch

No review yet. Who's going to review this?
Comment 11 Saam Barati 2016-08-20 21:51:38 PDT
(In reply to comment #9)
> A quick update on Daren's request: This is going to be a very difficult
> change to test.  The most obvious way would be to integrate undefined
> behavior sanitizer into our testing infrastructure, although this is many
> months off if it will happen at all.
> 
> The other method of testing would be construct a test which would crash
> without this change.  While this is likely possible, it's unclear to me what
> such a test would like like.  While attempting to construct a test which
> would crash without this change, I discovered that it really only seems to
> be js/regress/simple-regexp-exec-folding.html which exhibits the bug, but
> even this test will not always exhibit this error (most notably, is the
> number of iterations through the loop is decreased, the error will no longer
> occur).
So there is a test where we call this function on a nullptr?
If so, why don't we crash? Does that function not load any fields?
If not, I'm a bit confused as to what your explanation for needing this
check is. I'm not too familiar about which states this particular field can be null in,
but there are other places where we allow for null in a pointer field,
but later access it without a null check because other conditions
being true imply that the field is non-null. (This may or may not
be the case here). 

> 
> If uncovering the precise code path which triggers this bug is important, I
> can continue to investigate.  However, I don't think continued investigation
> is worthwhile, as an analogous case in forAllTransitiveIncomingValues
> preforms this check.
Comment 12 Saam Barati 2016-08-20 21:52:34 PDT
When you say that that particular test "exhibits the bug",
what exactly do you mean?
Comment 13 Darin Adler 2016-08-20 21:59:54 PDT
Jonathan has been compiling WebKit with clang's UndefinedBehaviorSanitizer <http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html>. When compiled like this, code will trap if it makes a non-static function member call with a nullptr for the this pointer.

I think of it as a way to add additional assertions.

When Jonathan is saying that a test "exhibits the bug", he means that when compiled that way, that test hit the trap.
Comment 14 Darin Adler 2016-08-20 22:01:09 PDT
Please note: It’s not legal C++ to call a non-static member function with a null pointer for this, even if the function is non-virtual, and even if the function doesn't access the this pointer directly or indirectly.
Comment 15 Saam Barati 2016-08-21 11:14:28 PDT
(In reply to comment #13)
> Jonathan has been compiling WebKit with clang's UndefinedBehaviorSanitizer
> <http://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html>. When compiled
> like this, code will trap if it makes a non-static function member call with
> a nullptr for the this pointer.
> 
> I think of it as a way to add additional assertions.
> 
> When Jonathan is saying that a test "exhibits the bug", he means that when
> compiled that way, that test hit the trap.

I see. Very cool, I didn't know this was a thing.
Comment 16 Saam Barati 2016-08-21 11:19:52 PDT
Comment on attachment 286310 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:2003
> +        if (m_phiChildren) {
> +            m_phiChildren->forAllTransitiveIncomingValues(
> +                m_graph.varArgChild(node, 0).node(),
> +                [&] (Node* incoming) {
> +                    set.add(incoming->castConstant<Structure*>());
> +                });
> +        }

Filip recently added code that will add MaterializeNewObject into the IR even when we're not in SSA.
This is when we do RegExp constant folding on various RegExp operations, like RegExp.prototype.exec.
That said, I don't think this code is correct when we're not in SSA form. For example, I think you're saying
that the result of this node is the emptySet of structures when we're not in SSA. That's not what we want.
If anything, if we can't give a specific result filled with structure sets, we probably want to widen our result
type to just arbitrary Object or something similar to that. However, maybe there are other methods in which
we can attain the Structure(s) that this produce in non-SSA form.

Filip, what do you think?
Comment 17 Saam Barati 2016-08-21 11:23:03 PDT
Actually, it looks like the StructureSet is in the first OpInfo of the node.
It seems like we should use that.
(I'm looking at the code in StrengthReductionPhase that generates this node).
Comment 18 Saam Barati 2016-08-21 11:25:26 PDT
(In reply to comment #17)
> Actually, it looks like the StructureSet is in the first OpInfo of the node.
> It seems like we should use that.
> (I'm looking at the code in StrengthReductionPhase that generates this node).

Actually, it looks like that's what happens when we add this node
to the IR for allocation sinking phase as well. The first OpInfo of
the node has the StructureSet of the resulting type.
I believe the correct patch is to use the StructureSet from the OpInfo
to mark as the result of this node.
Comment 19 Saam Barati 2016-09-02 15:37:17 PDT
Created attachment 287826 [details]
patch
Comment 20 Mark Lam 2016-09-02 15:39:03 PDT
Comment on attachment 287826 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        The rule for MaterlializeNewObject inside AI was assuming that the graph

/MaterlializeNewObject/MaterializeNewObject/
Comment 21 Saam Barati 2016-09-02 15:43:05 PDT
Created attachment 287828 [details]
patch
Comment 22 Saam Barati 2016-09-02 15:58:22 PDT
This looks like its breaking an AsmJS test, not sure why. I'll investigate.
Comment 23 Saam Barati 2016-09-02 16:06:40 PDT
(In reply to comment #22)
> This looks like its breaking an AsmJS test, not sure why. I'll investigate.

actually, this failure looks unrelated to my change. Investigating now.
Comment 24 Saam Barati 2016-09-02 16:16:19 PDT
(In reply to comment #23)
> (In reply to comment #22)
> > This looks like its breaking an AsmJS test, not sure why. I'll investigate.
> 
> actually, this failure looks unrelated to my change. Investigating now.
Never mind, It looks like I'm running an older version of asm js.
Comment 25 Darin Adler 2016-09-03 06:46:07 PDT
Comment on attachment 287828 [details]
patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1953
> +        const StructureSet& set = node->structureSet();
>          forNode(node).set(m_graph, set);

Why the local variable? Does not seem like it makes this code clearer.
Comment 26 Saam Barati 2016-09-06 16:06:52 PDT
(In reply to comment #25)
> Comment on attachment 287828 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287828&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1953
> > +        const StructureSet& set = node->structureSet();
> >          forNode(node).set(m_graph, set);
> 
> Why the local variable? Does not seem like it makes this code clearer.

Thanks for the review. No reason in particular for the local variable, I 
was just refactoring the old code. I agree that it's not needed, I'll remove it.
Comment 27 Saam Barati 2016-09-06 16:10:32 PDT
Created attachment 288060 [details]
patch for landing
Comment 28 WebKit Commit Bot 2016-09-06 16:53:22 PDT
Comment on attachment 288060 [details]
patch for landing

Clearing flags on attachment: 288060

Committed r205522: <http://trac.webkit.org/changeset/205522>
Comment 29 WebKit Commit Bot 2016-09-06 16:53:28 PDT
All reviewed patches have been landed.  Closing bug.