[PATCH] D18860: [analyzer] Fix the "Zombie symbols" issue.

[Artem Dergachev via Phabricator via cfe-commits]

NoQ updated this revision to Diff 172263.
NoQ added a comment.

Ha! Apart from Zombie symbols that are neither dead nor alive and therefore not buried properly, there are also Schrödinger symbols that are dead and alive at the same time. Moreover, asking whether a Schrödinger symbol is `isLive()` or `maybeDead()` would immediately collapse it into an alive-only state. Moreover, if a checker iterates through the dead set and asks whether a Schrödinger symbol is alive, undefined behavior occurs. Ah, thin ice.

As far as i understand, this only happens to derived symbols under fairly specific circumstances. Namely, if the derived symbol is marked as dead and then its parent symbol is marked as live, the derived symbol is not automatically removed from the "dead set". It is impossible to automatically add derived symbols to the dead set because there may be infinitely many symbols derived from the same parent symbol. Therefore there exists moment of time when the derived symbol is still in the dead set, but asking whether it is `isLive()` would yield true (because it'll check whether the parent symbol is alive). Additionally, when `isLive()` notices that it's alive, it automatically removes the symbol from the dead set in order to "memoize" the answer, so it becomes purely alive. In particular, it invalidates dead set iterators, which results in undefined behavior (which in practice boils down to past-end iterator access, ultimately crashing Clang).

Schrödinger symbols cause false positive leaks, unlike zombies who only cause false negative leaks. But those false positives are relatively rare because most checkers don't track derived symbols; eg., MallocChecker only tracks pure conjured symbols. RetainCountChecker is affected, as demonstrated by the newly added test. Not sure if it's possible to make CStringChecker track a derived symbol and forget a string length due to that effect. MacOSKeychainAPI checker should also be affected, i think, but i didn't try. NullabilityChecker is probably unaffected; its behavior on dead symbols is pretty weird, but it seems safe; it also has a separate bug that was exposed but i'll address in a separate patch.


https://reviews.llvm.org/D18860

Files:
  include/clang/StaticAnalyzer/Core/PathSensitive/SMTConstraintManager.h
  include/clang/StaticAnalyzer/Core/PathSensitive/SymbolManager.h
  lib/StaticAnalyzer/Checkers/CStringChecker.cpp
  lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
  lib/StaticAnalyzer/Checkers/MPI-Checker/MPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MacOSKeychainAPIChecker.cpp
  lib/StaticAnalyzer/Checkers/MallocChecker.cpp
  lib/StaticAnalyzer/Checkers/NullabilityChecker.cpp
  lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
  lib/StaticAnalyzer/Checkers/StreamChecker.cpp
  lib/StaticAnalyzer/Core/Environment.cpp
  lib/StaticAnalyzer/Core/ExprEngine.cpp
  lib/StaticAnalyzer/Core/RangeConstraintManager.cpp
  lib/StaticAnalyzer/Core/RegionStore.cpp
  lib/StaticAnalyzer/Core/SymbolManager.cpp
  test/Analysis/keychainAPI.m
  test/Analysis/pr22954.c
  test/Analysis/retain-release-cpp-classes.cpp
  test/Analysis/self-assign.cpp
  test/Analysis/simple-stream-checks.c
  test/Analysis/unions.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D18860.172263.patch
Type: text/x-patch
Size: 23931 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20181101/8e088a55/attachment-0001.bin>