PR23057: fix use-after-free due to local token buffer in ParseCXXAmbiguousParenExpression

[DmitryPolukhin]

Summary

To completely eliminate use-after-free in this place I had to copy tokens into new array and pass ownership. As far as I understand the code it is not possible to guarantee that all inserted tokens are consumed before exiting from this function. Depending on how many tokens were consumed before SkipUntil is called, it may not happen due to unbalanced brackets, etc. Other places where EnterTokenStream is called usually pass vector that some class owns but here it is not possible because this place can be called recursively.

[dblaikie]

I don't fully understand your explanation. Is your change introducing a
memory leak? (or was the old code causing a double free (if
EnterTokenStream does take ownership of this argument)in addition to
use-after-free?)

[DmitryPolukhin]

I didn't introduce a leak because I pass ownership of the Buffer to EnterTokenStream (i.e. pass true as the last argument OwnsTokens). ASan also doesn't report a leak with my patch. Original code didn't have double free because it used call EnterTokenStream with OwnsTokens == false.

[majnemer]

I believe this change is correct but I believe we can handle this a little more efficiently.

I wonder if it would be better if we instead inserted a special EOF token at the end of Toks. If any errors occur, consume tokens until we hit our special EOF token. ParseLexedAttribute and ParseLexedMethodDeclaration follow this pattern. To make it a little more obvious that the lifetime of Toks is somehow tied to EnterTokenStream and the parsing code that follows, it might make sense to factor the code after EnterTokenStream into another function.

[DmitryPolukhin]

Use EOF token instead of copy buffer. This approach looks a bit more fragile but definitely more efficient, PTAL.

[majnemer]

LGTM

[ABataev]

Committed in http://reviews.llvm.org/rL259750

lib/Parse/ParseExprCXX.cpp

if (Tok.is(tok::l_paren) && NextToken().is(tok::r_paren)) {
                                    IsTypeCast);
     }
 
     // If we parsed a cast-expression, it's really a type-id, otherwise it's
     // an expression.
     ParseAs = NotCastExpr ? SimpleExpr : CastExpr;
   }
 
+  // Create a fake EOF to mark end of Toks buffer.
+  Token AttrEnd;
+  AttrEnd.startToken();
+  AttrEnd.setKind(tok::eof);
+  AttrEnd.setLocation(Tok.getLocation());
+  AttrEnd.setEofData(Toks.data());
+  Toks.push_back(AttrEnd);
+
   // The current token should go after the cached tokens.
   Toks.push_back(Tok);
   // Re-enter the stored parenthesized tokens into the token stream, so we may
   // parse them now.
   PP.EnterTokenStream(Toks.data(), Toks.size(),
                       true/*DisableMacroExpansion*/, false/*OwnsTokens*/);
   // Drop the current token and bring the first cached one. It's the same token
   // as when we entered this function.
   ConsumeAnyToken();
 
   if (ParseAs >= CompoundLiteral) {
     // Parse the type declarator.
     DeclSpec DS(AttrFactory);
     Declarator DeclaratorInfo(DS, Declarator::TypeNameContext);
     {
       ColonProtectionRAIIObject InnerColonProtection(*this);
       ParseSpecifierQualifierList(DS);
       ParseDeclarator(DeclaratorInfo);
     }
 
     // Match the ')'.
     Tracker.consumeClose();
     ColonProt.restore();
 
+    // Consume EOF marker for Toks buffer.
+    assert(Tok.is(tok::eof) && Tok.getEofData() == AttrEnd.getEofData());
+    ConsumeAnyToken();
+
     if (ParseAs == CompoundLiteral) {
       ExprType = CompoundLiteral;
       if (DeclaratorInfo.isInvalidType())
         return ExprError();
 
       TypeResult Ty = Actions.ActOnTypeName(getCurScope(), DeclaratorInfo);
       return ParseCompoundLiteralExpression(Ty.get(),
                                             Tracker.getOpenLocation(),
Parser::ParseCXXAmbiguousParenExpression(ParenParseOption &ExprType,
   ExprType = SimpleExpr;
   Result = ParseExpression();
   if (!Result.isInvalid() && Tok.is(tok::r_paren))
     Result = Actions.ActOnParenExpr(Tracker.getOpenLocation(), 
                                     Tok.getLocation(), Result.get());
 
   // Match the ')'.
   if (Result.isInvalid()) {
-    SkipUntil(tok::r_paren, StopAtSemi);
+    while (Tok.isNot(tok::eof))
+      ConsumeAnyToken();
+    assert(Tok.getEofData() == AttrEnd.getEofData());
+    ConsumeAnyToken();
     return ExprError();
   }
 
   Tracker.consumeClose();
+  // Consume EOF marker for Toks buffer.
+  assert(Tok.is(tok::eof) && Tok.getEofData() == AttrEnd.getEofData());
+  ConsumeAnyToken();
   return Result;
 }

test/Parser/cxx-ambig-paren-expr-asan.cpp

+// RUN: %clang_cc1 -fsyntax-only -pedantic -verify %s
+
+// This syntax error used to cause use-after free due to token local buffer
+// in ParseCXXAmbiguousParenExpression.
+int H((int()[)]);
+// expected-error@-1 {{expected expression}}
+// expected-error@-2 {{expected ']'}}
+// expected-note@-3 {{to match this '['}}
+// expected-error@-4 {{expected ';' after top level declarator}}