[PATCH] D20863: [analyzer] Fix for the strdup family in unix.malloc checker

[Artem Dergachev via cfe-commits]

NoQ added a comment.

This patch is doing a very good thing - modeling contents of the copied string as a lazy compound value of the original string. For that, i guess it's worth adding some tests (ExprInspection-based(?)) that show that, say, first few characters of the copied string are same as the original. It might also be a good idea to move this modeling to CStringChecker completely, because right now it's easier to assume a range on SymbolExtent of the strdupped region, than transfer correct string length into MallocChecker. But then you'd need to enable CStringChecker in your builds so that they could work together to model strdup().

The effect you observe, however - finding leaks on overwriting variables that hold leaked symbols - is in fact not because you modeled the copy better, but because you managed to accidentally work around the "zombie symbols" bug, described in http://lists.llvm.org/pipermail/cfe-dev/2016-March/047922.html and http://reviews.llvm.org/D18860 (there's still no certainty as of how exactly to fix it). Because you model contents of the copied string by binding a value to it, the Store is eager to reap the symbol as soon as possible, and hence detect a leak. Before, only the checker knew about the symbol of the copied string, and it wasn't eager to reap that symbol after it disappeared from the rest of the program state through overwriting. The problem could as well be fixed with code as simple and unobvious as:

  void MallocChecker::checkLiveSymbols(ProgramStateRef State,
                                       SymbolReaper &SR) const {
    for (const auto &Item: State->get<RegionState>())
      SR.maybeDead(Item.first);
  }


================
Comment at: lib/StaticAnalyzer/Checkers/MallocChecker.cpp:2134
@@ +2133,3 @@
+  auto sizeTy = svalBuilder.getContext().getSizeType();
+  auto strLength = svalBuilder.getMetadataSymbolVal(MallocChecker::getTag(), MR,
+                                                    Ex, sizeTy, C.blockCount());
----------------
I'm not sure if this is truly worth it. In CStringChecker, a lot of work is done in order to model the length of the string; however, currently MallocChecker does not have any access to that info, and a lot more code copy-paste'ing would be necessary to re-model all the stuff. And even if you do so, the symbol you create in that statement wouldn't be the same as the symbol used in CStringChecker, so you may never compare them to each other and see that they're equal - say, strlen() of a duplicated string, strlen() for the original string, and the value you produce on that statement, would still be three different symbols.

Ideally, we can leave this out for later, and wait until we allow checkers interact with each other and share such information. So that we could ask CStringChecker directly here, and obtain string length as CStringChecker sees it.


http://reviews.llvm.org/D20863