[PATCH] D49057: [analyzer] Track multiple raw pointer symbols in DanglingInternalBufferChecker

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Much symbols!



================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:126
+      NewSet = F.add(NewSet, RawPtr.getAsSymbol());
+      if (!NewSet.isEmpty()) {
+        State = State->set<RawPtrMap>(ObjRegion, NewSet);
----------------
rnkovacs wrote:
> xazax.hun wrote:
> > Is it possible here the set to be empty? We just added a new element to it above. Maybe turn this into an assert or just omit this if it is impossible?
> I'm not sure whether `add()` can fail. I turned it into an assert now and will see if it ever fails.
It totally can't. We're just adding an object to a set. What could possibly go wrong?


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:32-40
+namespace clang {
+namespace ento {
+template<> struct ProgramStateTrait<PtrSet>
+  : public ProgramStatePartialTrait<PtrSet> {
+  static void *GDMIndex() {
+    static int Index = 0;
+    return &Index;
----------------
Please add a comment on how this template is useful. This trick is used by some checkers, but it's a veeeery unobvious trick. We should probably add a macro for that, i.e. `REGISTER_FACTORY_WITH_PROGRAMSTATE` or something like that.


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:113
-      const SymbolRef *StrBufferPtr = State->get<RawPtrMap>(TypedR);
-      // FIXME: What if Origin is null?
       const Expr *Origin = Call.getOriginExpr();
----------------
Maybe turn the FIXME into a NOTE or something like that. It indeed seems fine, but let's still make sure the reader realizes that there's a subtle potential problem here.


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:115
     SVal RawPtr = Call.getReturnValue();
     if (!RawPtr.isUnknown()) {
+      // Start tracking this raw pointer by adding it to the set of symbols
----------------
I'd much rather unwrap the symbol here, i.e. `if (SymbolRef Sym = RawPtr.getAsSymbol())`. A lot of other cornercases may occur if the implementation is accidentally inlined (`Undefined`, concrete regions). Also, speculatively, `.getAsSymbol(/* search for symbolic base = */ true)` for the same reason//*//.

If we didn't care about inlined implementations, the `Unknown` check would have been redundant. So it should also be safe to straightforwardly ignore inlined implementations by consulting `C.wasInlined`, then the presence of the symbol can be asserted. But i'd rather speculatively care about inlined implementations as long as it seems easy.

__
//*// In fact your code relies on a very wonky implementation detail of our `SVal` hierarchy: namely, pointer-type return values of conservatively evaluated functions are always expressed as `&SymRegion{conj_$N<pointer type>}` and never as `&element{SymRegion{conj_$N<pointer type>}, 0 S32b, pointee type}`. Currently nobody knows the rules under which zero element regions are added in different parts of the analyzer, i.e. what is the "canonical" representation of the symbolic pointer, though i made a few attempts to speculate.


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:119
+      PtrSet::Factory &F = State->getStateManager().get_context<PtrSet>();
+      PtrSet NewSet = F.getEmptySet();
+      if (const PtrSet *OldSet = State->get<RawPtrMap>(ObjRegion)) {
----------------
My favorite idiom for such cases, looks a bit cleaner:
```
const PtrSet *SetPtr = State->get<RawPtrMap>(ObjRegion)
PtrSet Set = SetPtr ? *SetPtr : F.getEmptySet()
```


================
Comment at: lib/StaticAnalyzer/Checkers/DanglingInternalBufferChecker.cpp:122
+        NewSet = *OldSet;
+        if (NewSet.contains(RawPtr.getAsSymbol()))
+          return;
----------------
This `contains` check is very unlikely to yield true because if the implementation is not inlined, the symbol is newly born. I'd much rather skip the check; it doesn't sound like it'd make things any faster.

Even better, let's assert that: `assert(C.wasInlined || !Set.contains(Sym))`. It'll probably help us catch some "reincarnated symbol" bugs in the core.


https://reviews.llvm.org/D49057