Skip to content

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

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

Closed
kaby76 wants to merge 2 commits into from
Closed

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

Changes from all commits
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
4 changes: 4 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,10 @@ namespace {
}

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

@parrt parrt Nov 7, 2022

Choose a reason for hiding this comment

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

I wonder how badly this will hit performance

Copy link
Contributor Author

@kaby76 kaby76 Nov 8, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ericvergnaud ericvergnaud Nov 8, 2022

Choose a reason for hiding this comment

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

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

Copy link
Member

@parrt parrt Nov 8, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@kaby76 kaby76 Nov 9, 2022

Choose a reason for hiding this comment

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

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.

return rhs == nullptr;
else if (rhs == nullptr)
return false;
return *lhs == *rhs;
}

Expand Down