Skip to content

Commit

Permalink
Fix for memory leak in function `ParserATNSimulator::execATNWithFullC…
Browse files Browse the repository at this point in the history 
…ontext`
  • Loading branch information
@mike-lischke
mike-lischke committed Nov 11, 2018
1 parent a02c2fa commit 95fecce
Show file tree
Hide file tree
Showing 4 changed files with 24 additions and 14 deletions.
14 changes: 10 additions & 4 deletions runtime/Cpp/runtime/src/NoViableAltException.cpp
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
/* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
* Use of this file is governed by the BSD 3-clause license that
* can be found in the LICENSE.txt file in the project root.
*/
Expand All @@ -11,12 +11,18 @@ using namespace antlr4;

NoViableAltException::NoViableAltException(Parser *recognizer)
: NoViableAltException(recognizer, recognizer->getTokenStream(), recognizer->getCurrentToken(),
recognizer->getCurrentToken(), nullptr, recognizer->getContext()) {
recognizer->getCurrentToken(), nullptr, recognizer->getContext(), false) {
}

NoViableAltException::NoViableAltException(Parser *recognizer, TokenStream *input,Token *startToken,
Token *offendingToken, atn::ATNConfigSet *deadEndConfigs, ParserRuleContext *ctx)
: RecognitionException("No viable alternative", recognizer, input, ctx, offendingToken), _deadEndConfigs(deadEndConfigs), _startToken(startToken) {
Token *offendingToken, atn::ATNConfigSet *deadEndConfigs, ParserRuleContext *ctx, bool deleteConfigs)
: RecognitionException("No viable alternative", recognizer, input, ctx, offendingToken),
_deadEndConfigs(deadEndConfigs), _startToken(startToken), _deleteConfigs(deleteConfigs) {
}

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

Token* NoViableAltException::getStartToken() const {
Expand Down
10 changes: 7 additions & 3 deletions runtime/Cpp/runtime/src/NoViableAltException.h
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
/* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
* Use of this file is governed by the BSD 3-clause license that
* can be found in the LICENSE.txt file in the project root.
*/
Expand All @@ -19,15 +19,19 @@ namespace antlr4 {
public:
NoViableAltException(Parser *recognizer); // LL(1) error
NoViableAltException(Parser *recognizer, TokenStream *input,Token *startToken,
Token *offendingToken, atn::ATNConfigSet *deadEndConfigs, ParserRuleContext *ctx);

Token *offendingToken, atn::ATNConfigSet *deadEndConfigs, ParserRuleContext *ctx, bool deleteConfigs);
~NoViableAltException();

virtual Token* getStartToken() const;
virtual atn::ATNConfigSet* getDeadEndConfigs() const;

private:
/// Which configurations did we try at input.index() that couldn't match input.LT(1)?
atn::ATNConfigSet* _deadEndConfigs;

// Flag that indicates if we own the dead end config set and have to delete it on destruction.
bool _deleteConfigs;

/// The token object at the start index; the input stream might
/// not be buffering tokens so get a reference to it. (At the
/// time the error occurred, of course the stream needs to keep a
Expand Down
10 changes: 5 additions & 5 deletions runtime/Cpp/runtime/src/atn/ParserATNSimulator.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ size_t ParserATNSimulator::execATN(dfa::DFA &dfa, dfa::DFAState *s0, TokenStream
// ATN states in SLL implies LL will also get nowhere.
// If conflict in states that dip out, choose min since we
// will get error no matter what.
NoViableAltException e = noViableAlt(input, outerContext, previousD->configs.get(), startIndex);
NoViableAltException e = noViableAlt(input, outerContext, previousD->configs.get(), startIndex, false);
input->seek(startIndex);
size_t alt = getSynValidOrSemInvalidAltThatFinishedDecisionEntryRule(previousD->configs.get(), outerContext);
if (alt != ATN::INVALID_ALT_NUMBER) {
Expand Down Expand Up @@ -236,7 +236,7 @@ size_t ParserATNSimulator::execATN(dfa::DFA &dfa, dfa::DFAState *s0, TokenStream
BitSet alts = evalSemanticContext(D->predicates, outerContext, true);
switch (alts.count()) {
case 0:
throw noViableAlt(input, outerContext, D->configs.get(), startIndex);
throw noViableAlt(input, outerContext, D->configs.get(), startIndex, false);

case 1:
return alts.nextSetBit(0);
Expand Down Expand Up @@ -351,7 +351,7 @@ size_t ParserATNSimulator::execATNWithFullContext(dfa::DFA &dfa, dfa::DFAState *
// ATN states in SLL implies LL will also get nowhere.
// If conflict in states that dip out, choose min since we
// will get error no matter what.
NoViableAltException e = noViableAlt(input, outerContext, previous, startIndex);
NoViableAltException e = noViableAlt(input, outerContext, previous, startIndex, previous != s0);
input->seek(startIndex);
size_t alt = getSynValidOrSemInvalidAltThatFinishedDecisionEntryRule(previous, outerContext);
if (alt != ATN::INVALID_ALT_NUMBER) {
Expand Down Expand Up @@ -1229,8 +1229,8 @@ void ParserATNSimulator::dumpDeadEndConfigs(NoViableAltException &nvae) {
}

NoViableAltException ParserATNSimulator::noViableAlt(TokenStream *input, ParserRuleContext *outerContext,
ATNConfigSet *configs, size_t startIndex) {
return NoViableAltException(parser, input, input->get(startIndex), input->LT(1), configs, outerContext);
ATNConfigSet *configs, size_t startIndex, bool deleteConfigs) {
return NoViableAltException(parser, input, input->get(startIndex), input->LT(1), configs, outerContext, deleteConfigs);
}

size_t ParserATNSimulator::getUniqueAlt(ATNConfigSet *configs) {
Expand Down
4 changes: 2 additions & 2 deletions runtime/Cpp/runtime/src/atn/ParserATNSimulator.h
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
/* Copyright (c) 2012-2017 The ANTLR Project. All rights reserved.
* Use of this file is governed by the BSD 3-clause license that
* can be found in the LICENSE.txt file in the project root.
*/
Expand Down Expand Up @@ -836,7 +836,7 @@ namespace atn {
virtual antlrcpp::BitSet getConflictingAltsOrUniqueAlt(ATNConfigSet *configs);

virtual NoViableAltException noViableAlt(TokenStream *input, ParserRuleContext *outerContext,
ATNConfigSet *configs, size_t startIndex);
ATNConfigSet *configs, size_t startIndex, bool deleteConfigs);

static size_t getUniqueAlt(ATNConfigSet *configs);

Expand Down

2 comments on commit 95fecce

@chund
Copy link
Contributor

@chund chund commented on 95fecce Jan 10, 2019

Choose a reason for hiding this comment

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

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

@mike-lischke
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Please sign in to comment.