[analyzer] MoveChecker: correct invalidation of this-regions

[guillem-bartrina-sonarsource]

By completely omitting invalidation in the case of InstanceCall, we do not clear the moved state of the fields of the this object after an opaque call to a member function of the object itself.

[llvmbot]

@llvm/pr-subscribers-clang

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

Author: None (guillem-bartrina-sonarsource)

Changes

By completely omitting invalidation in the case of InstanceCall, we do not clear the moved state of the fields of the this object after an opaque call to a member function of the object itself.

---

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

2 Files Affected:

• (modified) clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp (+8-9)
• (added) clang/test/Analysis/use-after-move-invalidation.cpp (+53)

diff --git a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
index f5c34078df2ee..95ccb183d1e6e 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp
@@ -244,11 +244,12 @@ bool isMovedFrom(ProgramStateRef State, const MemRegion *Region) {
 
 // If a region is removed all of the subregions needs to be removed too.
 static ProgramStateRef removeFromState(ProgramStateRef State,
-                                       const MemRegion *Region) {
+                                       const MemRegion *Region,
+                                       bool strict = false) {
   if (!Region)
     return State;
   for (auto &E : State->get<TrackedRegionMap>()) {
-    if (E.first->isSubRegionOf(Region))
+    if ((!strict || E.first != Region) && E.first->isSubRegionOf(Region))
       State = State->remove<TrackedRegionMap>(E.first);
   }
   return State;
@@ -709,18 +710,16 @@ ProgramStateRef MoveChecker::checkRegionChanges(
     // that are passed directly via non-const pointers or non-const references
     // or rvalue references.
     // In case of an InstanceCall don't invalidate the this-region since
-    // it is fully handled in checkPreCall and checkPostCall.
-    const MemRegion *ThisRegion = nullptr;
-    if (const auto *IC = dyn_cast<CXXInstanceCall>(Call))
-      ThisRegion = IC->getCXXThisVal().getAsRegion();
+    // it is fully handled in checkPreCall and checkPostCall, but do invalidate
+    // its strict subregions, as they are not handled.
 
     // Requested ("explicit") regions are the regions passed into the call
     // directly, but not all of them end up being invalidated.
     // But when they do, they appear in the InvalidatedRegions array as well.
     for (const auto *Region : RequestedRegions) {
-      if (ThisRegion != Region &&
-          llvm::is_contained(InvalidatedRegions, Region))
-        State = removeFromState(State, Region);
+      if (llvm::is_contained(InvalidatedRegions, Region))
+        State = removeFromState(State, Region,
+                                /*strict=*/!!dyn_cast<CXXInstanceCall>(Call));
     }
   } else {
     // For invalidations that aren't caused by calls, assume nothing. In
diff --git a/clang/test/Analysis/use-after-move-invalidation.cpp b/clang/test/Analysis/use-after-move-invalidation.cpp
new file mode 100644
index 0000000000000..8201fed26f1e3
--- /dev/null
+++ b/clang/test/Analysis/use-after-move-invalidation.cpp
@@ -0,0 +1,53 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s -std=c++11\
+// RUN: -analyzer-output=text -verify=expected
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+class C {
+  int n;
+public:
+  void meth();
+};
+
+void opaqueFreeFun();
+
+class Foo {
+  std::unique_ptr<C> up;
+  void opaqueMeth();
+
+  void testOpaqueMeth() {
+    auto tmp = std::move(up);
+    opaqueMeth(); // clears region state
+    (void)up->meth(); // no-warning
+  }
+
+  void testOpaqueFreeFun() {
+    auto tmp = std::move(up); // expected-note{{Smart pointer 'up' of type 'std::unique_ptr' is reset to null when moved from}}
+    opaqueFreeFun(); // does not clear region state
+    (void)up->meth(); // expected-warning{{Dereference of null smart pointer 'up' of type 'std::unique_ptr'}}
+                      // expected-note@-1{{Dereference of null smart pointer 'up' of type 'std::unique_ptr'}}
+  }
+};
+
+void testInstanceMeth(C c) {
+  auto tmp1 = std::move(c); // expected-note{{Object 'c' is moved}}
+  auto tmp2 = std::move(c); // expected-warning{{Moved-from object 'c' is moved}}
+                            // expected-note@-1{{Moved-from object 'c' is moved}}
+  auto tmp3 = std::move(c);
+  c.meth(); // does not clear region state
+  auto tmp4 = std::move(c);
+  auto tmp5 = std::move(c); // no-warning
+}
+
+void modify(C&);
+
+void testPassByRef(C c) {
+  auto tmp1 = std::move(c); // expected-note{{Object 'c' is moved}}
+  auto tmp2 = std::move(c); // expected-warning{{Moved-from object 'c' is moved}}
+                            // expected-note@-1{{Moved-from object 'c' is moved}}
+  auto tmp3 = std::move(c);
+  modify(c); // clears region state
+  auto tmp4 = std::move(c); // expected-note{{Object 'c' is moved}}
+  auto tmp5 = std::move(c); // expected-warning{{Moved-from object 'c' is moved}}
+                            // expected-note@-1{{Moved-from object 'c' is moved}}
+}

[steakhal]

Looks great. Thanks!

[guillem-bartrina-sonarsource]

Thanks for the review! Feel free to merge it once the CI finishes.