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_brace)) {
 
     // If we parsed a cast-expression, it's really a type-id, otherwise it's
     // an expression.
     ParseAs = NotCastExpr ? SimpleExpr : CastExpr;
   }
 
   // The current token should go after the cached tokens.
   Toks.push_back(Tok);
+
+  // Make a copy of stored token to pass ownership to EnterTokenStream function.
+  // This copy is required because we may exit from this function when some
+  // tokens are not consumed yet.
+  Token *Buffer = new Token[Toks.size()];
+  std::copy(Toks.begin(), Toks.end(), Buffer);
+
   // 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*/);
+  PP.EnterTokenStream(Buffer, Toks.size(),
+                      true/*DisableMacroExpansion*/, true/*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);

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 '['}}