Fix for issue 3845 -- null pointer deref in Cpp target

[kaby76]

Very simply, the code that compares two PredictionContext must test before dereferencing.

antlr4/runtime/Cpp/runtime/src/atn/ArrayPredictionContext.cpp

Line 25 in aa1f1f1

return *lhs == *rhs;

. If both pointers are null then the compare is "true".

[KvanTTT]

Please simplify:

Suggested change

	if (lhs == nullptr && rhs == nullptr)
	return true;
	if (lhs == nullptr && !(rhs == nullptr))
	return false;
	if (!(lhs == nullptr) && rhs == nullptr)
	return false;
	if (lhs == nullptr)
	return rhs == nullptr;
	else if (rhs == nullptr)
	return false;

[kaby76]

Sure.

But, I bet there are other places like this. For example, these two statements are very similar to this bug, with parameters of type Ref<> &.

• antlr4/runtime/Cpp/runtime/src/atn/ATNConfig.h

  Line 39 in aa1f1f1

  return (lhs == rhs) || (*lhs == *rhs);

• antlr4/runtime/Cpp/runtime/src/atn/PredictionContextCache.cpp

  Line 55 in aa1f1f1

  return *lhs == *rhs;

People make changes that "fix" a tiny little problem, but don't read the damn code. @parrt what about these occurrences?

[parrt]

Yep, that's why I'm opposed to refactoring for refactoring sake. It can introduce all sorts of subtle bugs.

We have to be careful here for example because the real solution is that context objects should never be null... There is a sentinel that indicates empty stack. That is why I didn't simply add the null checks... There's a larger bug and we shouldn't paint over it.

[kaby76]

Yep, that's why I'm opposed to refactoring for refactoring sake. It can introduce all sorts of subtle bugs.

We have to be careful here for example because the real solution is that context objects should never be null... There is a sentinel that indicates empty stack. That is why I didn't simply add the null checks... There's a larger bug and we shouldn't paint over it.

Then, these should be at least assert()'s so that it crashes with a message.

Is there a method that anyone wrote that checks the integrity of the data structure so I can percolate the check around and snuff this bug out? Someone is creating an array with a null pointer at the end of the array.

[parrt]

That's what I was hoping to do with the Data dump from the ATN simulator... We could use it to compare how the different targets work.

BTW, here is the start of me trying to fix C++ by refracturing the complex code: #3863

I gave up trying to debug when a print statement exploded all over my screen with compile errors.

Here is my attempt to start a tracer for ATN: #3817

[kaby76]

Actually, I think your assumption that there cannot be null parents in an array of PredicateContext may be wrong. Many ports violate that assumption.

I wrote a Check() function that checks the integrity of the ArrayPredictionContext and it led to this code in the Cpp target. The code creates a "parents" array containing a null pointer.

• Cpp:
  

  antlr4/runtime/Cpp/runtime/src/atn/PredictionContext.cpp

  Line 319 in 16cfc6b

  std::vector<Ref<const PredictionContext>> parents = { a->parent, nullptr };

Compare this with CSharp, Java, PHP, and Python3.

• CSharp:
  

  antlr4/runtime/CSharp/src/Atn/PredictionContext.cs

  Line 391 in 16cfc6b

  PredictionContext[] parents = { a.parent, null };

• Java:
  

  antlr4/runtime/Java/src/org/antlr/v4/runtime/atn/PredictionContext.java

  Line 324 in 16cfc6b

  PredictionContext[] parents = {a.parent, null};

• PHP:
  https://github.com/antlr/antlr-php-runtime/blob/64fd1e2e8ca6fd80b57f53a0729e74b984bd767f/src/PredictionContexts/PredictionContext.php#L371

• Python3:
  

  antlr4/runtime/Python3/src/antlr4/PredictionContext.py

  Line 420 in 16cfc6b

  parents = [ a.parentCtx, None ]

For Dart, null pointers are disallowed (I think??). In any case, the code looks like it never creates a "parents" array with a null pointer.

• Dart:
  

  antlr4/runtime/Dart/lib/src/prediction_context.dart

  Line 247 in 16cfc6b

  parents = [b.parent, a.parent];

For Go, I think it too was written without using null pointers:

• Go:
  

  antlr4/runtime/Go/antlr/prediction_context.go

  Line 534 in 16cfc6b

  parents = []PredictionContext{b.parentCtx, a.parentCtx}

[parrt]

Well, dang. You're right. Hmm...yeah, touching the ATN simulation is really risky as I don't remember the details well. :(

[kaby76]

Yes, I can relate to forgetting details. It definitely is concerning that a couple of the targets don't have nulls in the parents array. I think to be "safe", I should test this against the regression tests of grammars-v4 for Cpp.

[parrt]

I would love to do a comparison of the java target against other targets. I have good trust in java behavior.

[kaby76]

Changing back from "draft" to "review" because ArrayPredictionContext can contain null pointers.

[parrt]

I wonder how badly this will hit performance

[kaby76]

I wonder how badly this will hit performance

I think this question is difficult to answer without changing the Cpp runtime to not create "parent" arrays with null pointers. For tests that call the comparison function, the parse crashes; for tests that don't call the function, the parse works. Presumably, in Java when it calls equals()] on comparing two "parents" arrays, it will test whether the element in the array is null (see Arrays.equals() => Object.equals()). This change makes Cpp operate the same way as the Java code. The choice is either to get the Cpp working, or rewrite all the runtimes to not use null pointers in "parents".

[ericvergnaud]

I suspect it doesn't make any sense to have nulls in parents arrays, so I would opt for x-target cleanup, which as a side effect is likely to slightly improve performance

[parrt]

I could swear that we had a sentinel node that represented top of the stack rather than null but I have to look at the code more carefully because @kaby76 found something that makes it look like it really is using null. I am terrified of changing anything in the ATN simulation given just how much code in the world relies on it.

[kaby76]

The old code compare called Arrays::equals(). That in turn would test null pointers. The nullptr in the parents array has existed for 6 years at least.

[parrt]

Fixed by #3958