Manage preprocessor Macro objects with shared pointers

src/compiler/preprocessor/DirectiveParser.cpp

@@ -131,7 +131,7 @@ bool hasDoubleUnderscores(const std::string &name)
 bool isMacroPredefined(const std::string &name, const pp::MacroSet &macroSet)
 {
     pp::MacroSet::const_iterator iter = macroSet.find(name);
-    return iter != macroSet.end() ? iter->second.predefined : false;
+    return iter != macroSet.end() ? iter->second->predefined : false;
 }
 
 }  // namespace anonymous
@@ -358,30 +358,30 @@ void DirectiveParser::parseDefine(Token *token)
                              token->text);
     }
 
-    Macro macro;
-    macro.type = Macro::kTypeObj;
-    macro.name = token->text;
+    std::shared_ptr<Macro> macro = std::make_shared<Macro>();
+    macro->type                  = Macro::kTypeObj;
+    macro->name                  = token->text;
 
     mTokenizer->lex(token);
     if (token->type == '(' && !token->hasLeadingSpace())
     {
         // Function-like macro. Collect arguments.
-        macro.type = Macro::kTypeFunc;
+        macro->type = Macro::kTypeFunc;
         do
         {
             mTokenizer->lex(token);
             if (token->type != Token::IDENTIFIER)
                 break;
 
-            if (std::find(macro.parameters.begin(), macro.parameters.end(), token->text) !=
-                macro.parameters.end())
+            if (std::find(macro->parameters.begin(), macro->parameters.end(), token->text) !=
+                macro->parameters.end())
             {
                 mDiagnostics->report(Diagnostics::PP_MACRO_DUPLICATE_PARAMETER_NAMES,
                                      token->location, token->text);
                 return;
             }
 
-            macro.parameters.push_back(token->text);
+            macro->parameters.push_back(token->text);
 
             mTokenizer->lex(token);  // Get ','.
         } while (token->type == ',');
@@ -400,24 +400,24 @@ void DirectiveParser::parseDefine(Token *token)
         // list. Resetting it also allows us to reuse Token::equals() to
         // compare macros.
         token->location = SourceLocation();
-        macro.replacements.push_back(*token);
+        macro->replacements.push_back(*token);
         mTokenizer->lex(token);
     }
-    if (!macro.replacements.empty())
+    if (!macro->replacements.empty())
     {
         // Whitespace preceding the replacement list is not considered part of
         // the replacement list for either form of macro.
-        macro.replacements.front().setHasLeadingSpace(false);
+        macro->replacements.front().setHasLeadingSpace(false);
     }
 
     // Check for macro redefinition.
-    MacroSet::const_iterator iter = mMacroSet->find(macro.name);
-    if (iter != mMacroSet->end() && !macro.equals(iter->second))
+    MacroSet::const_iterator iter = mMacroSet->find(macro->name);
+    if (iter != mMacroSet->end() && !macro->equals(*iter->second))
     {
-        mDiagnostics->report(Diagnostics::PP_MACRO_REDEFINED, token->location, macro.name);
+        mDiagnostics->report(Diagnostics::PP_MACRO_REDEFINED, token->location, macro->name);
         return;
     }
-    mMacroSet->insert(std::make_pair(macro.name, macro));
+    mMacroSet->insert(std::make_pair(macro->name, macro));
 }
 
 void DirectiveParser::parseUndef(Token *token)
@@ -434,13 +434,13 @@ void DirectiveParser::parseUndef(Token *token)
     MacroSet::iterator iter = mMacroSet->find(token->text);
     if (iter != mMacroSet->end())
     {
-        if (iter->second.predefined)
+        if (iter->second->predefined)
         {
             mDiagnostics->report(Diagnostics::PP_MACRO_PREDEFINED_UNDEFINED, token->location,
                                  token->text);
             return;
         }
-        else if (iter->second.expansionCount > 0)
+        else if (iter->second->expansionCount > 0)
         {
             mDiagnostics->report(Diagnostics::PP_MACRO_UNDEFINED_WHILE_INVOKED, token->location,
                                  token->text);

src/compiler/preprocessor/Macro.cpp

@@ -24,11 +24,11 @@ void PredefineMacro(MacroSet *macroSet, const char *name, int value)
     token.type = Token::CONST_INT;
     token.text = ToString(value);
 
-    Macro macro;
-    macro.predefined = true;
-    macro.type       = Macro::kTypeObj;
-    macro.name       = name;
-    macro.replacements.push_back(token);
+    std::shared_ptr<Macro> macro = std::make_shared<Macro>();
+    macro->predefined            = true;
+    macro->type                  = Macro::kTypeObj;
+    macro->name                  = name;
+    macro->replacements.push_back(token);
 
     (*macroSet)[name] = macro;
 }

src/compiler/preprocessor/Macro.h

@@ -8,6 +8,7 @@
 #define COMPILER_PREPROCESSOR_MACRO_H_
 
 #include <map>
+#include <memory>
 #include <string>
 #include <vector>
 
@@ -39,7 +40,7 @@ struct Macro
     Replacements replacements;
 };
 
-typedef std::map<std::string, Macro> MacroSet;
+typedef std::map<std::string, std::shared_ptr<Macro>> MacroSet;
 
 void PredefineMacro(MacroSet *macroSet, const char *name, int value);
 

src/compiler/preprocessor/MacroExpander.cpp

@@ -70,8 +70,11 @@ MacroExpander::ScopedMacroReenabler::ScopedMacroReenabler(MacroExpander *expande
 MacroExpander::ScopedMacroReenabler::~ScopedMacroReenabler()
 {
     mExpander->mDeferReenablingMacros = false;
-    for (auto *macro : mExpander->mMacrosToReenable)
+    for (auto macro : mExpander->mMacrosToReenable)
     {
+        // Copying the string here by using substr is a check for use-after-free. It detects
+        // use-after-free more reliably than just toggling the disabled flag.
+        ASSERT(macro->name.substr() != "");
         macro->disabled = false;
     }
     mExpander->mMacrosToReenable.clear();
@@ -115,8 +118,8 @@ void MacroExpander::lex(Token *token)
         if (iter == mMacroSet->end())
             break;
 
-        const Macro &macro = iter->second;
-        if (macro.disabled)
+        std::shared_ptr<Macro> macro = iter->second;
+        if (macro->disabled)
         {
             // If a particular token is not expanded, it is never expanded.
             token->setExpansionDisabled(true);
@@ -125,12 +128,12 @@ void MacroExpander::lex(Token *token)
 
         // Bump the expansion count before peeking if the next token is a '('
         // otherwise there could be a #undef of the macro before the next token.
-        macro.expansionCount++;
-        if ((macro.type == Macro::kTypeFunc) && !isNextTokenLeftParen())
+        macro->expansionCount++;
+        if ((macro->type == Macro::kTypeFunc) && !isNextTokenLeftParen())
         {
             // If the token immediately after the macro name is not a '(',
             // this macro should not be expanded.
-            macro.expansionCount--;
+            macro->expansionCount--;
             break;
         }
 
@@ -190,22 +193,22 @@ bool MacroExpander::isNextTokenLeftParen()
     return lparen;
 }
 
-bool MacroExpander::pushMacro(const Macro &macro, const Token &identifier)
+bool MacroExpander::pushMacro(std::shared_ptr<Macro> macro, const Token &identifier)
 {
-    ASSERT(!macro.disabled);
+    ASSERT(!macro->disabled);
     ASSERT(!identifier.expansionDisabled());
     ASSERT(identifier.type == Token::IDENTIFIER);
-    ASSERT(identifier.text == macro.name);
+    ASSERT(identifier.text == macro->name);
 
     std::vector<Token> replacements;
-    if (!expandMacro(macro, identifier, &replacements))
+    if (!expandMacro(*macro, identifier, &replacements))
         return false;
 
     // Macro is disabled for expansion until it is popped off the stack.
-    macro.disabled = true;
+    macro->disabled = true;
 
     MacroContext *context = new MacroContext;
-    context->macro        = &macro;
+    context->macro        = macro;
     context->replacements.swap(replacements);
     mContextStack.push_back(context);
     mTotalTokensInContexts += context->replacements.size();

src/compiler/preprocessor/MacroExpander.h

@@ -35,7 +35,7 @@ class MacroExpander : public Lexer
     void ungetToken(const Token &token);
     bool isNextTokenLeftParen();
 
-    bool pushMacro(const Macro &macro, const Token &identifier);
+    bool pushMacro(std::shared_ptr<Macro> macro, const Token &identifier);
     void popMacro();
 
     bool expandMacro(const Macro &macro, const Token &identifier, std::vector<Token> *replacements);
@@ -56,7 +56,7 @@ class MacroExpander : public Lexer
         const Token &get();
         void unget();
 
-        const Macro *macro;
+        std::shared_ptr<Macro> macro;
         std::size_t index;
         std::vector<Token> replacements;
     };
@@ -72,7 +72,7 @@ class MacroExpander : public Lexer
     int mAllowedMacroExpansionDepth;
 
     bool mDeferReenablingMacros;
-    std::vector<const Macro *> mMacrosToReenable;
+    std::vector<std::shared_ptr<Macro>> mMacrosToReenable;
 
     class ScopedMacroReenabler;
 };

src/tests/preprocessor_tests/define_test.cpp

@@ -985,6 +985,27 @@ TEST_F(DefineTest, RecursiveMacroNameInsideIncompleteMacroInvocationInMacroExpan
     preprocess(input, expected);
 }
 
+// The name of the macro "a" is inside an incomplete macro invocation of macro "m()" in its own
+// expansion. Then the macro "a" is undef'd. This is a regression test for a memory management bug
+// where macro "a" would be freed on undef even though cleaning up the recursive macro invocation
+// would still need to refer to macro "a".
+TEST_F(DefineTest, UndefInsideRecursiveMacroInvocation)
+{
+    const char *input =
+        "#define m(a)\n"
+        "#define a m((a)\n"
+        "a\n"
+        "#undef a\n"
+        ")\n";
+    const char *expected =
+        "\n"
+        "\n"
+        "\n"
+        "\n"
+        "\n";
+    preprocess(input, expected);
+}
+
 // The macro invocations form a long chain. The macro expander should protect against stack overflow
 // and generate an error in this case.
 TEST_F(DefineTest, LongMacroInvocationChain)