aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM aside from testing request.
================
Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:313
+ AST_POLYMORPHIC_SUPPORTED_TYPES(Decl, Stmt, TypeLoc),
+ std::string, MacroName) {
// Verifies that the statement' beginning and ending are both expanded from
----------------
njames93 wrote:
> aaron.ballman wrote:
> > You mentioned that the change from `StringRef` to `std::string` was to
> > avoid lifetime issues while matching, but I'm wondering if you can expound
> > on that situation a bit more. I would have assumed that any memoization
> > that involves `StringRef` would be responsible for the lifetime issues
> > rather than the matchers themselves, but maybe I'm thinking about a
> > different way you can hit lifetime issues than you are.
> Take this as contrived, however using ASAN reports a heap-use-after-free for
> this code when isExpandedFromMacro takes a `StringRef` but works fine when
> using `std::string`.
> ```lang=c++
> TEST(IsExpandedFromMacro, IsExpandedFromMacro_MatchesDecls) {
> auto Matcher =
> isExpandedFromMacro(std::string("MY_MACRO_OVERFLOW_SMALL_STRING_SIZE"));//
> <-Temporary string created and destroyed here.
> StringRef input = R"cc(
> #define MY_MACRO_OVERFLOW_SMALL_STRING_SIZE(a) int i = a;
> void Test() { MY_MACRO_OVERFLOW_SMALL_STRING_SIZE(4); }
> )cc";
> EXPECT_TRUE(matches(input, varDecl(Matcher))); // <- heap-use-after-free
> detected down the callstack from here.
> }
> ```
> Obviously this is very contrived to ensure asan detects the use-after-free
> and to ensure the temporary string is destroyed before the matcher. For these
> tests it doesn't look like a problem, but in other instances these matchers
> will outlive a string being passed to them
> ```lang=c++
> void addVarFromMacro(ASTMatchFinder* Finder, std::string MacroName) {
> Finder->addMatcher(varDecl(isExpandedFromMacro(MacroName)), this);
> }
> ```
Oh, how interesting! I can see now where the lifetime issues come in to play,
thanks!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D90303/new/
https://reviews.llvm.org/D90303
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
