[analyzer] PR41269: SmartPtrModeling.

[NoQ]

Summary

A tiny placeholder for a checker to assist modeling of smart pointers. Currently it doesn't emit any bug reports and it does only one thing: returns null for .get() and .release() methods and false for conversion to bool when the pointer is freshly moved from. It interacts with the MoveChecker in order to figure this out, which addresses https://bugs.llvm.org/show_bug.cgi?id=41269

This still doesn't quite follow my global plan of "make it possible to deal with C++ objects as if they're symbols", but i still keep my faith in this plan. Also it might be a good idea to untangle smart-pointer-related parts of MoveChecker and promote it to a more generic "dereference of a null smart pointer" check in the new checker.

[NoQ]

Why alpha? Because it needs a bug visitor to explain what's going on when it suddenly returns a note. Because it's going to be a bugtype-inspecific note that is emitted by a checker, i want to implement it via note tags (D58367), otherwise it's quite a layering nightmare to set up communication between core and checker visitors (especially given that we'd want to attach a new move visitor immediately after emitting the note).

[a_sidorin]

Hi Artem,
There are some comments inline, but looks promising!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

46

const?

54

I find the std::bind syntax a bit overcomplicated. Maybe a lambda would be better?
[Call](const auto &CD) { return Call->isCalled(CD); }

60

1. & operator doesn't return nullptr (if not overloaded)
2. If CallRef can be nullptr, this can be kinda dangerous.

Should we use get() instead?

[Szelethus]

The patch looks OK in general! Left some comments inline, my main concern is whether this checker actually depends on MoveChecker or not.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

496

Hmmm, shouldn't this depend from MoveChecker?

Dependencies<[MoveChecker]>,

clang/lib/StaticAnalyzer/Checkers/Move.h

31

#endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_TAINT_H

:^)

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

236

Can't we just return just State->contains<TrackedRegionMap>(Region)? Unless you plan to add more values to RegionState::Kind, I think it would be fine.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

54

+1 :)

[NoQ]

Fxd.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

496

For now it probably should, but i think eventually it will be useful on its own, independently.

On the other hand, it might make sense to have MoveChecker depend on this checker, because this checker eliminates false positives in MoveChecker, so we shouldn't allow enabling MoveChecker without this checker. But for now it would mean depending on an alpha checker.

WDYT?

clang/lib/StaticAnalyzer/Checkers/Move.h

31

Whoops, and it shouldn't say anything about TAINT, right?

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

236

Unless you plan to add more values to RegionState::Kind, I think it would be fine.

I suspect that we may have to add an "Escaped" state eventually. I.e.,

T a;
escape(&a);
T b = std::move(a);
reset_escaped(); // resets 'a'
a.foo(); // no-warning

For now i don't know how to easily address this problem fully without storing every single escaping object in the program state (regardless of whether it is going to ever be moved), and this problem is pretty rare: i've only seen one move-checker false positive of this kind, it was on LLVM, and it was fixed on LLVM side independently in D57618. In this code the error object was escaping into an iterator and getting reset on every loop iteration, but now they made the idiom more sane.

Also if we ever end up tracking escaped objects, we might as well track *all* escaped objects for *all* checkers in *one* set-trait. Simply because escaped locals behave more like arbitrary heap objects than like locals. But for now i suspect that this is an overkill.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

54

Oh well. I tried so hard to learn how to use std::bind but nobody likes it :)

60

Yuck. Indeed, i'm forming a null reference in the middle while returning from operator*(). At least, i would be forming a null reference if we ever invoke evalCall() on a call that makes getCall() fail, 'cause for now we seem to never do that. Also we should simply pass a CallEvent to evalCall, as we trivially have it available on ExprEngine side :/

[Szelethus]

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

496

Ugh, yea, good points. I think we should express, even if temporarily, that this checker depends on MoveChecker. Just leave a couple TODO's around that this has to revised!

On the long term, it makes sense for MoveChecker to depend on this one, agreed.

clang/lib/StaticAnalyzer/Checkers/Move.h

31

Riiight. :)

[NoQ]

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

60

Whoops, i actually forgot to fix this one. On the other hand, i found the intended way of writing this code, and it's neither &* nor .get(). Behold!

[NoQ]

The actual "Behold!".

[NoQ]

On second thought, maybe let's restrict the scope to just bool conversions (don't model .get() and .release() yet) but enable the check by default immediately?

It's still bad that we don't explain why if (P) takes a false branch for a moved-from pointer P, but it's not that bad, because it's "just" a control-flow dependency of the bug; there won't be any actual bug reports about some bool being false (hopefully nobody would ever try to divide by such bool), but there may be actual bug reports about a pointer being null (eg., when it's dereferenced).

This way we will address the false positive quickly and hopefully don't cause too much trouble.

[Szelethus]

In D60796#1472587, @NoQ wrote:

On second thought, maybe let's restrict the scope to just bool conversions (don't model .get() and .release() yet) but enable the check by default immediately?

It's still bad that we don't explain why if (P) takes a false branch for a moved-from pointer P, but it's not that bad, because it's "just" a control-flow dependency of the bug; there won't be any actual bug reports about some bool being false (hopefully nobody would ever try to divide by such bool), but there may be actual bug reports about a pointer being null (eg., when it's dereferenced).

This way we will address the false positive quickly and hopefully don't cause too much trouble.

Yup, let's just roll with it. LGTM!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

24

Do you actually need this include?

62

How about now, after D58367 landed? :)

[NoQ]

Fix unused include.

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

62

That's still not immediately possible because i believe that this note should be prunable.

[Szelethus]

This might be an ideal candidate for being hidden by default though (D60925).

[NoQ]

In D60796#1473342, @Szelethus wrote:

This might be an ideal candidate for being hidden by default though (D60925).

Hmm, should i rename it into some sort of "SmartPtrModel" and later make a separate checker to emit actual warnings?

[Szelethus]

Yea, something along the lines of SmartPtrModeling would be neat -- logically, if you plan to emit errors down the line, subcheckers should be used for that, in order to enable checkers to depend purely on the modeling part.

[NoQ]

In D60796#1474826, @Szelethus wrote:

Yea, something along the lines of SmartPtrModeling would be neat -- logically, if you plan to emit errors down the line, subcheckers should be used for that, in order to enable checkers to depend purely on the modeling part.

Will do!

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

2

Sigh.

[NoQ]

Rename the checker.

[Szelethus]

I think you can land this one if you're comfortable with it! +1 on adding core in the test RUN: lines.

clang/include/clang/StaticAnalyzer/Checkers/Checkers.td

def IteratorRangeChecker : Checker<"IteratorRange">,
   Documentation<HasAlphaDocumentation>;
 
 def MismatchedIteratorChecker : Checker<"MismatchedIterator">,
   HelpText<"Check for use of iterators of different containers where iterators "
            "of the same container are expected">,
   Dependencies<[IteratorModeling]>,
   Documentation<HasAlphaDocumentation>;
 
+def SmartPtrChecker: Checker<"SmartPtr">,
+  HelpText<"Checks use of C++ smart pointers">,
+  Documentation<NotDocumented>;
+
 def UninitializedObjectChecker: Checker<"UninitializedObject">,
      HelpText<"Reports uninitialized fields after object construction">,
   Documentation<HasAlphaDocumentation>;
 
 } // end: "alpha.cplusplus"
 
 
 //===----------------------------------------------------------------------===//

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   PointerSubChecker.cpp
   PthreadLockChecker.cpp
   RetainCountChecker/RetainCountChecker.cpp
   RetainCountChecker/RetainCountDiagnostics.cpp
   ReturnPointerRangeChecker.cpp
   ReturnUndefChecker.cpp
   RunLoopAutoreleaseLeakChecker.cpp
   SimpleStreamChecker.cpp
+  SmartPtrChecker.cpp
   StackAddrEscapeChecker.cpp
   StdLibraryFunctionsChecker.cpp
   StreamChecker.cpp
   Taint.cpp
   TaintTesterChecker.cpp
   TestAfterDivZeroChecker.cpp
   TraversalChecker.cpp
   TrustNonnullChecker.cpp

clang/lib/StaticAnalyzer/Checkers/Move.h

+//=== Move.h - Tracking moved-from objects. ------------------------*- C++ -*-//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Defines inter-checker API for the use-after-move checker. It allows
+// dependent checkers to figure out if an object is in a moved-from state.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MOVE_H
+#define LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MOVE_H
+
+#include "clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h"
+
+namespace clang {
+namespace ento {
+namespace move {
+
+/// Returns true if the object is known to have been recently std::moved.
+bool isMovedFrom(ProgramStateRef State, const MemRegion *Region);
+
+} // namespace move
+} // namespace ento
+} // namespace clang
+
+#endif // LLVM_CLANG_LIB_STATICANALYZER_CHECKERS_MOVE_H

clang/lib/StaticAnalyzer/Checkers/MoveChecker.cpp

private:
   const ExplodedNode *getMoveLocation(const ExplodedNode *N,
                                       const MemRegion *Region,
                                       CheckerContext &C) const;
 };
 } // end anonymous namespace
 
 REGISTER_MAP_WITH_PROGRAMSTATE(TrackedRegionMap, const MemRegion *, RegionState)
 
+// Define the inter-checker API.
+namespace clang {
+namespace ento {
+namespace move {
+bool isMovedFrom(ProgramStateRef State, const MemRegion *Region) {
+  const RegionState *RS = State->get<TrackedRegionMap>(Region);
+  return RS && (RS->isMoved() || RS->isReported());
+}
+} // namespace move
+} // namespace ento
+} // namespace clang
+
 // If a region is removed all of the subregions needs to be removed too.
 static ProgramStateRef removeFromState(ProgramStateRef State,
                                        const MemRegion *Region) {
   if (!Region)
     return State;
   for (auto &E : State->get<TrackedRegionMap>()) {
     if (E.first->isSubRegionOf(Region))
       State = State->remove<TrackedRegionMap>(E.first);

clang/lib/StaticAnalyzer/Checkers/SmartPtrChecker.cpp

+// SmartPtrChecker.cpp - Check use of moved-from objects. - C++ -----------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file defines a checker that models various aspects of
+// C++ smart pointer behavior.
+//
+//===----------------------------------------------------------------------===//
+
+#include "Move.h"
+
+#include "clang/AST/ExprCXX.h"
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h"
+#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h"
+#include "clang/StaticAnalyzer/Core/Checker.h"
+#include "clang/StaticAnalyzer/Core/CheckerManager.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h"
+#include "llvm/ADT/StringSet.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class SmartPtrChecker : public Checker<eval::Call> {
+  std::vector<CallDescription> NullAfterMoveMethods = {
+    {{ "unique_ptr", "get" }},
+    {{ "unique_ptr", "release" }},
+    {{ "shared_ptr", "get" }},
+    {{ "shared_ptr", "release" }},
+    {{ "weak_ptr",   "get" }},
+    {{ "weak_ptr",   "release" }},
+  };
+  bool isNullAfterMoveMethod(const CXXInstanceCall *Call);
+
+public:
+  bool evalCall(const CallExpr *CE, CheckerContext &C) const;
+};
+} // end of anonymous namespace
+
+bool SmartPtrChecker::isNullAfterMoveMethod(const CXXInstanceCall *Call) {
+  // TODO: Update CallDescription to support anonymous calls?
+  const auto *CD = dyn_cast<CXXConversionDecl>(Call->getDecl());
+  if (CD && CD->getConversionType()->isBooleanType())
+    return true;
+
+  return llvm::any_of(
+      NullAfterMoveMethods,
+      std::bind(&CallEvent::isCalled, Call, std::placeholders::_1));
+}
+
+bool SmartPtrChecker::evalCall(const CallExpr *CE, CheckerContext &C) const {
+  CallEventRef<> CallRef = C.getStateManager().getCallEventManager().getCall(
+      CE, C.getState(), C.getLocationContext());
+  const auto *Call = dyn_cast_or_null<CXXInstanceCall>(&*CallRef);
+  if (!Call)
+    return false;
+
+  ProgramStateRef State = C.getState();
+  const MemRegion *ThisR = Call->getCXXThisVal().getAsRegion();
+
+  if (!move::isMovedFrom(State, ThisR)) {
+    // TODO: Model this case as well. At least, avoid invalidation of globals.
+    return false;
+  }
+
+  // TODO: Add a note to bug reports describing this decision.
+  C.addTransition(
+      State->BindExpr(CE, C.getLocationContext(),
+                      C.getSValBuilder().makeZeroVal(CE->getType())));
+  return true;
+}
+
+void ento::registerSmartPtrChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<SmartPtrChecker>();
+}
+
+bool ento::shouldRegisterSmartPtrChecker(const LangOptions &LO) {
+  return LO.CPlusPlus;
+}

clang/test/Analysis/Inputs/system-header-simulator-cxx.h

namespace std {
   public:
     unique_ptr(const unique_ptr &) = delete;
     unique_ptr(unique_ptr &&);
 
     T *get() const;
 
     typename std::add_lvalue_reference<T>::type operator*() const;
     T *operator->() const;
+    operator bool() const;
   };
 }
 #endif
 
 #ifdef TEST_INLINABLE_ALLOCATORS
 namespace std {
   void *malloc(size_t);
   void free(void *);

clang/test/Analysis/smart-ptr.cpp

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection\
+// RUN:   -analyzer-checker cplusplus.Move,alpha.cplusplus.SmartPtr\
+// RUN:   -std=c++11 -verify %s
+
+#include "Inputs/system-header-simulator-cxx.h"
+
+void clang_analyzer_warnIfReached();
+
+void derefAfterMove(std::unique_ptr<int> P) {
+  std::unique_ptr<int> Q = std::move(P);
+  if (Q)
+    clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  *Q.get() = 1; // no-warning
+  if (P)
+    clang_analyzer_warnIfReached(); // no-warning
+  // FIXME: Implement suppress-on-sink so that use-after-move warning didn't
+  // show up here. It's redundant when there's already a null dereference.
+  *P.get() = 1; // expected-warning {{Method called on moved-from object 'P'}}
+                // expected-warning@-1 {{Dereference of null pointer}}
+}

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

 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
-// RUN:  -analyzer-checker debug.ExprInspection\
+// RUN:  -analyzer-checker core,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -verify=expected,peaceful,non-aggressive
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS\
-// RUN:  -analyzer-checker debug.ExprInspection\
+// RUN:  -analyzer-checker core,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -verify=expected,peaceful,non-aggressive
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
 // RUN:  -analyzer-config cplusplus.Move:WarnOn=KnownsOnly\
-// RUN:  -analyzer-checker debug.ExprInspection\
+// RUN:  -analyzer-checker core,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -verify=expected,non-aggressive
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move -verify %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS\
 // RUN:  -analyzer-config cplusplus.Move:WarnOn=KnownsOnly\
-// RUN:  -analyzer-checker debug.ExprInspection\
+// RUN:  -analyzer-checker core,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -verify=expected,non-aggressive
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=unexplored_first_queue\
 // RUN:  -analyzer-config cplusplus.Move:WarnOn=All\
-// RUN:  -analyzer-checker debug.ExprInspection\
+// RUN:  -analyzer-checker core,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -verify=expected,peaceful,aggressive
 // RUN: %clang_analyze_cc1 -analyzer-checker=cplusplus.Move %s\
 // RUN:  -std=c++11 -analyzer-output=text -analyzer-config eagerly-assume=false\
 // RUN:  -analyzer-config exploration_strategy=dfs -DDFS\
 // RUN:  -analyzer-config cplusplus.Move:WarnOn=All\
-// RUN:  -analyzer-checker debug.ExprInspection\
+// RUN:  -analyzer-checker core,alpha.cplusplus.SmartPtr,debug.ExprInspection\
 // RUN:  -verify=expected,peaceful,aggressive
 
 // RUN: not %clang_analyze_cc1 -verify %s \
 // RUN:   -analyzer-checker=core \
 // RUN:   -analyzer-checker=cplusplus.Move \
 // RUN:   -analyzer-config cplusplus.Move:WarnOn="a bunch of things" \
 // RUN:   2>&1 | FileCheck %s -check-prefix=CHECK-MOVE-INVALID-VALUE
 
P.get(); // peaceful-warning {{Method called on moved-from object 'P'}}
            // peaceful-note@-1 {{Method called on moved-from object 'P'}}
 }
 
 void localUniquePtrWithArrow(std::unique_ptr<A> P) {
   std::unique_ptr<A> Q = std::move(P); // expected-note{{Smart pointer 'P' of type 'std::unique_ptr' is reset to null when moved from}}
   P->foo(); // expected-warning{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
             // expected-note@-1{{Dereference of null smart pointer 'P' of type 'std::unique_ptr'}}
 }
+
+void getAfterMove(std::unique_ptr<A> P) {
+  std::unique_ptr<A> Q = std::move(P); // peaceful-note {{Object 'P' is moved}}
+
+  if (P) // peaceful-note    {{Taking false branch}}
+         // expected-note@-1 {{Taking false branch}}
+    clang_analyzer_warnIfReached(); // no-warning
+
+  A *a = P.get(); // peaceful-warning {{Method called on moved-from object 'P'}}
+                  // peaceful-note@-1 {{Method called on moved-from object 'P'}}
+                  // FIXME: Explain why is this a null value.
+                  // expected-note@-3 {{'a' initialized to a null pointer value}}
+  a->foo(); // expected-warning {{Called C++ object pointer is null}}
+            // expected-note@-1 {{Called C++ object pointer is null}}
+}