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

[Reka Kovacs via Phabricator via cfe-commits]

rnkovacs added inline comments.


================
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;
----------------
NoQ wrote:
> 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.
Should I do that then? Maybe in `CheckerContext.h`?


================
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
----------------
NoQ wrote:
> 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.
I wasn't aware of much of this. Thanks for the detailed explanation :)


https://reviews.llvm.org/D49057