[PATCH] D45532: [StaticAnalyzer] Checker to find uninitialized fields after a constructor call

[Umann Kristóf via Phabricator via cfe-commits]

Szelethus marked 28 inline comments as done.
Szelethus added inline comments.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:181-182
+/// Returns whether FD can be (transitively) dereferenced to a void pointer type
+/// (void*, void**, ...). The type of the region behind a void pointer isn't
+/// known, and thus FD can not be analyzed.
+bool isVoidPointer(const FieldDecl *FD);
----------------
NoQ wrote:
> Type of an object pointed to by a void pointer is something that our path-sensitive engine is sometimes able to provide. It is not uncommon that a void pointer points to a concrete variable on the current path, and we may know it in the analyzer. I'm not sure this sort of check is necessary (i.e. require more information).
> 
> You may also want to have a look at `getDynamicTypeInfo()` which is sometimes capable of retrieving the dynamic type of the object pointed to by a pointer or a reference - i.e. even if it's different from the pointee type of the pointer.
I took a look at it, but I found that it'd change the implementation of `isPointerOrReferenceUninit` so much, I'd be a lot more comfortable if I could implement it in a smaller followup patch. I placed some `TODO`s in the code about this.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:223-225
+  ExplodedNode *Node = Context.generateNonFatalErrorNode(Context.getState());
+  if (!Node)
+    return;
----------------
NoQ wrote:
> I suspect that a fatal error is better here. We don't want the user to receive duplicate report from other checkers that catch uninitialized values; just one report is enough.
I think that would be a bad idea. For example, this checker shouts so loudly while checking the LLVM project, it would practically halt the analysis of the code, reducing the coverage, which means that checkers other then uninit value checkers would "suffer" from it.

However, I also think that having multiple uninit reports for the same object might be good, especially with this checker, as it would be very easy to see where the problem originated from.

What do you think?


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:405-406
+  SVal DerefdV = StoreMgr.getBinding(S, V.castAs<Loc>());
+  while (Optional<Loc> L = DerefdV.getAs<Loc>())
+    DerefdV = StoreMgr.getBinding(S, *L);
+
----------------
NoQ wrote:
> Is this loop guaranteed to terminate? Is something like `void *v; v = &v;` possible?
> 
> I think this loop should unwrap the AST type on every iteration and dereference the pointer accordingly.
> 
> In general, i believe that every symbolic pointer dereference should be made with awareness of the type of the object we're trying to retrieve. Which is an optional argument for `State->getSVal(R, ...)`, but it seems that it should be made mandatory because in a lot of cases omitting it causes problems.
> Is this loop guaranteed to terminate? Is something like void *v; v = &v; possible?
I looked this up, and I am confident that it is not possible for a pointer to point to itself. Only a `void**` object may point to a `void*`. The loop terminates even if you do something evil like `v = reinterpret_cast<void*>(&v);` (I have tried this with `int`s).

>I think this loop should unwrap the AST type on every iteration and dereference the pointer accordingly.
>
>In general, i believe that every symbolic pointer dereference should be made with awareness of the type of the object we're trying to retrieve. Which is an optional argument for State->getSVal(R, ...), but it seems that it should be made mandatory because in a lot of cases omitting it causes problems.

I invested some serious effort into `getDynamicType`, but I think it'll require more time and some serious refactoring of this (`isPointerOrReferenceUninit`) function, so I'd post it separately.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:432
+      if (isUnionUninit(R)) {
+        LocalChain.dereference();
+        return addFieldToUninits(std::move(LocalChain));
----------------
NoQ wrote:
> Needs a comment on why the `IsDereferenced` flag on this path.
Added a comment at the line where I dereference the field that after that point we'll check the pointee.


================
Comment at: lib/StaticAnalyzer/Checkers/UninitializedObjectChecker.cpp:559-560
+  while (LC) {
+    if (isa<CXXConstructorDecl>(LC->getDecl()))
+      return true;
+
----------------
NoQ wrote:
> This doesn't check that the constructor on the parent stack frame is anyhow related to the current constructor, so it may introduce false negatives. For instance, a class may lazy-initialize a singleton but never store a reference to it within itself (because, well, it's a singleton, you can always obtain the reference to it). In this case we'll never find the bug in the constructor of the singleton.
Added a TODO and a test case to highlight this issue, and I'm planning to fix it in a followup patch.


https://reviews.llvm.org/D45532