Bug 160870 – Member call on NULL pointer in JavaScriptCore/dfg/DFGAbstractInterpretterInlines.h

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jonathan Bedard]

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

[Jonathan Bedard]

Created attachment 286102 [details]
Patch

[Jonathan Bedard]

This patch will fail style check, since it contains an edit to a Lambda function.

[WebKit Commit Bot]

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.

[Darin Adler]

Comment on attachment 286102 [details]
Patch

Needs a test case.

[Saam Barati]

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?

[Jonathan Bedard]

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)

[Jonathan Bedard]

Created attachment 286310 [details]
Patch

[WebKit Commit Bot]

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.

[Jonathan Bedard]

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.

[Darin Adler]

Comment on attachment 286310 [details]
Patch

No review yet. Who's going to review this?

[Saam Barati]

(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.

[Saam Barati]

When you say that that particular test "exhibits the bug",
what exactly do you mean?

[Darin Adler]

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.

[Darin Adler]

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.

[Saam Barati]

(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.

[Saam Barati]

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?

[Saam Barati]

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).

[Saam Barati]

(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.

[Saam Barati]

Created attachment 287826 [details]
patch

[Mark Lam]

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/

[Saam Barati]

Created attachment 287828 [details]
patch

[Saam Barati]

This looks like its breaking an AsmJS test, not sure why. I'll investigate.

[Saam Barati]

(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.

[Saam Barati]

(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.

[Darin Adler]

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.

[Saam Barati]

(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.

[Saam Barati]

Created attachment 288060 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 288060 [details]
patch for landing

Clearing flags on attachment: 288060

Committed r205522: <http://trac.webkit.org/changeset/205522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.