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.

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

Fix for antlr#2164. #2398

Merged
merged 1 commit into from Nov 11, 2018
Merged

Fix for antlr#2164. #2398

merged 1 commit into from Nov 11, 2018

Conversation

@mike-lischke
Copy link
Member

mike-lischke commented Nov 11, 2018

A different approach to the problem, now really fixing the leak.

@parrt parrt added the target:cpp label Nov 11, 2018
@parrt parrt added this to the 4.7.2 milestone Nov 11, 2018
@parrt parrt merged commit 42275a6 into antlr:master Nov 11, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mike-lischke mike-lischke deleted the mike-lischke:issue-2164 branch Nov 11, 2018
@chund

This comment has been minimized.

Copy link
Contributor

chund commented on 95fecce Jan 10, 2019

Dear Mike,
since there are no proper copy constructors or assignment operators implemented, there could occure double frees here:

NoViableAltException::~NoViableAltException() {
if (_deleteConfigs)
delete _deadEndConfigs;

How to protect _deadEndConfigs against double frees?

  • Use shared_ptr?
  • Implement proper copy constructor and assignment operator and handle _deadEndConfigs accordingly? How would it need to be handled?

Regards,
Christian

This comment has been minimized.

Copy link
Member Author

mike-lischke replied Jan 10, 2019

Hi Christian, where do you see a double free? What happens is that under certain conditions a new config is created and passed to the NoViableAltException c-tor. This is then freed in the d-tor. No other code path can remove that.

In all other cases the config is already held in a shared_ptr and managed by that (which means NoViableAltException should not delete the reference.

@chrisaycock

This comment has been minimized.

Copy link
Contributor

chrisaycock commented May 29, 2019

I hate to reopen this debate, but @chund is right; that is definitely a double free. In C++, throw copies the exception object and then invokes the destructor. So the catch object (in the generated parse() function from ...Parser.cpp) now contains a copy of the pointer that has just been freed. So once catch ends, the destructor is run a second time on that pointer.

The solution is to simply switch to a shared_ptr. This PR almost has it; there shouldn't be a destructor at all for NoViableAltException if _deadEndConfigs is a shared_ptr.

@mike-lischke

This comment has been minimized.

Copy link
Member Author

mike-lischke commented May 29, 2019

@chund @chrisaycock since this PR is already closed let's discuss this issue in the reference bug report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.