Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ea7f4a7
Fix for https://github.com/antlr/grammars-v4/issues/2909 https://gith…
kaby76 Nov 5, 2022
f736631
Simplify.
kaby76 Nov 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions runtime/Cpp/runtime/src/atn/ArrayPredictionContext.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,12 @@ namespace {
}

bool predictionContextEqual(const Ref<const PredictionContext> &lhs, const Ref<const PredictionContext> &rhs) {
if (lhs == nullptr && rhs == nullptr)
return true;
if (lhs == nullptr && !(rhs == nullptr))
return false;
if (!(lhs == nullptr) && rhs == nullptr)
return false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@kaby76 kaby76 Nov 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return *lhs == *rhs;
}

Expand Down