static Matcher generate Use After Free

Hello,

I need help. I see, that using static Matcher generate error on deallocated itself. For example, if we use:
static StatementMatcher MatcherA = callExpr();
MatcherA on dealloc tyrying to release reference counter of itself, but reference counter was deleted by method llvm_shutdown, so it use free memory.
Is it ok? We shouldn't use static matchers, or we have bug in implementation in reference counter. What is it case?

I think i've also noticed that static matcher objects don't work, but didn't pay enough attention to figure out why.

In clang-tidy, as far as i understand, they don't use static matchers, but instead they have long-lived MatchFinder objects filled with all the matchers they need, so they don't need to construct the same matchers again and again. Maybe that'd be a viable approach in your case?

06/06/2017 12:15 PM, Aleksandr wrote:

I think i've also noticed that static matcher objects don't work, but didn't pay enough attention to figure out why.

In clang-tidy, as far as i understand, they don't use static matchers, but instead they have long-lived MatchFinder objects filled with all the matchers they need, so they don't need to construct the same matchers again and again. Maybe that'd be a viable approach in your case?

Hm, but tools/extra/clang-tidy/modernize/ReplaceAutoPtrCheck.cpp actually has a static matcher:

/// \brief Matcher that finds expressions that are candidates to be wrapped with
/// 'std::move'.
///
/// Binds the id \c AutoPtrOwnershipTransferId to the expression.
static StatementMatcher MovableArgumentMatcher =
     expr(allOf(isLValue(), hasType(AutoPtrType)))
         .bind(AutoPtrOwnershipTransferId);

Or perhaps I'm missing something?

-Maxim

Dunno. Crashes i've seen were random, so i'm not sure if being static is the only requirement.

Also it seems to have been refactored out a few weeks ago, so there's no static matcher here anymore.

06/06/2017 12:45 PM, Maxim Ostapenko wrote:

Yes, crashes happen by assert in "llvm/include/llvm/ADT/IntrusiveRefCntPtr.h":

assert(NewRefCount >= 0 && "Reference count was already zero.");

So, if in freed memory was insert negative value by someone we get crash, in another case we can miss this problem.

CC'ing Samuel, Alexey and Ilya.

Dunno. Crashes i've seen were random, so i'm not sure if being static is the only requirement.

Yeah, you are right, this is not enough. E.g. when I tried to make several checkers static in NumberObjectConversionChecker.cpp nothing changed (everything works fine).
However, when I've added following checker into NumberObjectConversionChecker.cpp:

   static auto varD =
       expr(
         hasDescendant(
           declRefExpr(
             hasDeclaration(
               varDecl().bind("var")))));

I've actually got a use after free bug (ASan log is below). I'm not sure how to fix this, but this is non-obvious even for experienced developers that in some (non-obvious) cases static matchers don't work. IMHO this is a bug that needs to be fixed upstream.

-Maxim

$ /home/maxim/src/build/upstream/build_asan/./bin/clang -cc1 -internal-isystem /home/maxim/src/build/upstream/build_asan/lib/clang/5.0.0/include -nostdsysteminc -analyze -analyzer-constraints=range -analyzer-checker=osx,unix,core,alpha.security.taint -w -verify /home/maxim/src/llvm-upstream/tools/clang/test/Analysis/redefined_system.c