[analyzer] MoveChecker: correct invalidation of this-regions (llvm#169626)

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.

Author: guillem-bartrina-sonarsource

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=*/isa<CXXInstanceCall>(Call));
     }
   } else {
     // For invalidations that aren't caused by calls, assume nothing. In

clang/test/Analysis/use-after-move-invalidation.cpp

@@ -0,0 +1,51 @@
+// RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s -std=c++11\
+// RUN:  -analyzer-output=text
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+class C {
+  int n;
+public:
+  void memberFun();
+};
+
+template <class... Ts> void opaqueFreeFun(Ts...);
+
+class Foo {
+  std::unique_ptr<C> up;
+  void opaqueMemberFun();
+
+  void testOpaqueMemberFun() {
+    auto tmp = std::move(up);
+    opaqueMemberFun(); // clears region state
+    (void)up->memberFun(); // 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->memberFun(); // 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 testInstanceMemberFun(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.memberFun(); // does not clear region state
+  auto tmp4 = std::move(c);
+  auto tmp5 = std::move(c); // no-warning
+}
+
+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);
+  opaqueFreeFun<C&>(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}}
+}