[analyzer] Invalidate the object in opaque ctor calls regardless if an arg refers to it

[steakhal]

The conservative call invalidation logic is a bit complicated, and would deserve some refactoring.

When a call has some arguments, we escape them. Except, if its a pointer to constant storage - because we assume that the program honors const-correctness.

In that case, it puts it in the "Preserved" list to keep its contents. However, if we had a constructor call that's job is to initialize an object had a const pointer/reference parameter then the invalidation didn't take place.

This meant that if the object was on the stack, that we start warning about uninitialized fields when accessed. (See the example) Similar could be achieved on the heap of course.

We should have honored the fact that the constructor should initialize the pointee of "this", thus escape that region regardless (in other words, don't put it on the "preserved" list).

rdar://156942972

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-static-analyzer-1

Author: Balázs Benics (steakhal)

Changes

The conservative call invalidation logic is a bit complicated, and would deserve some refactoring.

When a call has some arguments, we escape them. Except, if its a pointer to constant storage - because we assume that the program honors const-correctness.

In that case, it puts it in the "Preserved" list to keep its contents. However, if we had a constructor call that's job is to initialize an object had a const pointer/reference parameter then the invalidation didn't take place.

This meant that if the object was on the stack, that we start warning about uninitialized fields when accessed. (See the example) Similar could be achieved on the heap of course.

We should have honored the fact that the constructor should initialize the pointee of "this", thus escape that region regardless (in other words, don't put it on the "preserved" list).

rdar://156942972

---

Full diff: https://github.com/llvm/llvm-project/pull/170887.diff

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Core/CallEvent.cpp (+25-5)
• (modified) clang/test/Analysis/call-invalidation.cpp (+18)

diff --git a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
index d04c827ce1391..04b0db0d1ffef 100644
--- a/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/clang/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -229,6 +229,14 @@ static void findPtrToConstParams(llvm::SmallSet<unsigned, 4> &PreserveArgs,
   }
 }
 
+static const MemRegion *getThisRegionBaseOrNull(const CallEvent &Call) {
+  if (const auto *CtorCall = dyn_cast<CXXConstructorCall>(&Call)) {
+    if (const MemRegion *R = CtorCall->getCXXThisVal().getAsRegion())
+      return R->getBaseRegion();
+  }
+  return nullptr;
+}
+
 ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
                                              ProgramStateRef State) const {
   // Don't invalidate anything if the callee is marked pure/const.
@@ -246,14 +254,26 @@ ProgramStateRef CallEvent::invalidateRegions(unsigned BlockCount,
   if (!argumentsMayEscape())
     findPtrToConstParams(PreserveArgs, *this);
 
+  // We should not preserve the contents of the region pointed by "this" when
+  // constructing the object, even if an argument refers to it.
+  const auto *ThisRegionBaseOrNull = getThisRegionBaseOrNull(*this);
+
   for (unsigned Idx = 0, Count = getNumArgs(); Idx != Count; ++Idx) {
     // Mark this region for invalidation.  We batch invalidate regions
     // below for efficiency.
-    if (PreserveArgs.count(Idx))
-      if (const MemRegion *MR = getArgSVal(Idx).getAsRegion())
-        ETraits.setTrait(MR->getBaseRegion(),
-                        RegionAndSymbolInvalidationTraits::TK_PreserveContents);
-        // TODO: Factor this out + handle the lower level const pointers.
+    if (PreserveArgs.count(Idx)) {
+      if (const MemRegion *ArgBaseR = getArgSVal(Idx).getAsRegion()) {
+        ArgBaseR = ArgBaseR->getBaseRegion();
+
+        // Preserve the contents of the pointee of the argument - except if it
+        // refers to the object under construction (ctor call).
+        if (ArgBaseR != ThisRegionBaseOrNull) {
+          ETraits.setTrait(
+              ArgBaseR, RegionAndSymbolInvalidationTraits::TK_PreserveContents);
+          // TODO: Factor this out + handle the lower level const pointers.
+        }
+      }
+    }
 
     ValuesToInvalidate.push_back(getArgSVal(Idx));
 
diff --git a/clang/test/Analysis/call-invalidation.cpp b/clang/test/Analysis/call-invalidation.cpp
index cb503023a1071..84970dcae18af 100644
--- a/clang/test/Analysis/call-invalidation.cpp
+++ b/clang/test/Analysis/call-invalidation.cpp
@@ -280,3 +280,21 @@ int testNestedStdNamespacesAndRecords() {
   int y = obj.uninit; // expected-warning {{Assigned value is uninitialized}}
   return x + y;
 }
+
+struct SpecialVector {
+  SpecialVector(const void *); // Takes a const pointer!
+  int size() const {
+    return Size; // no-warning: We should not warn "uninitialized Size" because the ctor might have initialized it.
+  }
+  int Size;
+};
+
+void selfPtrPassedAsConstPointerToOpaqueCtorCall() {
+  // We construct a "SpecialVector" that takes the address of itself
+  // (or to a subobject somewhere itself) by a const-pointer.
+  // Despite the var region "buf" is mentioned via a const argument, the opaque
+  // ctor cal should still take presecedent and invalidate the underlying object.
+  SpecialVector buf(&buf);
+  buf.size();
+}
+

[necto]

Looks good. I seems to me a subclass of a more generic issue with passing const+mutable pointers to the same object, but I could not reproduce it, so perhaps I don't understand it well enough:

void opaque(const void* a, void* b);

int top() {
    int *x;
    opaque(&x, &x);
    return *x; // I expected an issue here
}

[necto]

Suggested change

	// ctor cal should still take presecedent and invalidate the underlying object.
	// ctor call should still take precedent and invalidate the underlying object.

[steakhal]

void opaque(const void* a, void* b);

int top() {
int *x;
opaque(&x, &x);
return *x; // I expected an issue here
}

Great observation. I could craft an example breaking this.
https://godbolt.org/z/E5avb1xdn

void opaque(const int* a, int* b);
void clang_analyzer_value(int);

int top() {
    int x = 1;
    opaque(&x, &x);
    clang_analyzer_value(x); // we should not be sure it's 1.
    return 100 / (x - 1);
}