[PATCH] D16572: PR23057: fix use-after-free due to local token buffer in ParseCXXAmbiguousParenExpression

[David Blaikie via cfe-commits]

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?)

On Tue, Jan 26, 2016 at 2:45 AM, Dmitry Polukhin via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> DmitryPolukhin created this revision.
> DmitryPolukhin added a reviewer: rjmccall.
> DmitryPolukhin added a subscriber: cfe-commits.
>
> 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.
>
> http://reviews.llvm.org/D16572
>
> Files:
>   lib/Parse/ParseExprCXX.cpp
>   test/Parser/cxx-ambig-paren-expr-asan.cpp
>
> Index: test/Parser/cxx-ambig-paren-expr-asan.cpp
> ===================================================================
> --- /dev/null
> +++ test/Parser/cxx-ambig-paren-expr-asan.cpp
> @@ -0,0 +1,8 @@
> +// 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 at -1 {{expected expression}}
> +// expected-error at -2 {{expected ']'}}
> +// expected-note at -3 {{to match this '['}}
> Index: lib/Parse/ParseExprCXX.cpp
> ===================================================================
> --- lib/Parse/ParseExprCXX.cpp
> +++ lib/Parse/ParseExprCXX.cpp
> @@ -3083,10 +3083,17 @@
>
>    // 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();
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160126/d8ae113d/attachment.html>