[clang] [analyzer] Trust base to derived casts for dynamic types (PR #69057)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang

Author: Balazs Benics (steakhal)

<details>
<summary>Changes</summary>

When doing a base to derived cast, and we should add a cast info recording that fact.
This information will be then used for example inside `CXXInstanceCall::getRuntimeDefinition` (CallEvent.cpp), where for virtual calls, it will query the associated dynamic type for the `*this` region.

Note that inside `ExprEngine::defaultEvalCall`, if the runtime definition is found but it might have other definitions, then we will split the path and have one where we inline the candidate and one other doing conservative call evaluation. This ensures that if we inlined the wrong definition, we still have a path where conservative evaluation happened. In addition to this, remember that once a function was conservatively evaluated on a path, then further calls to the same function will always result in conservative evaluation, see `ExprEngine::BifurcateCall` and the `DynamicDispatchBifurcationMap`.

As a consequence of these rules, we can have execution paths where a function call might be resolved to different implemetations, depending on the sequence of events, like here:

```C++
void top(Base *p) {
  p->fun(); // Will split into 2 paths:
  // 1) Inlines the virtual `Base::fun()`.
  // 2) Conservative eval calls, and remembers to always conservatively eval this function.

  // Perform a base to derived cast, and just discard the result.
  (void)static_cast<Derived *>(p);

  p->fun(); // Will split path (1) into 2:
  // 2.1) Inlines the virtual `Derived::fun()`.
  // 2.2) Conservative eval calls, and remembers to always conservatively eval this function.

  // Now, we have exactly 3 paths at this point.
}
```

Fixes #<!-- -->62663

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


3 Files Affected:

- (modified) clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp (+17-10) 
- (added) clang/test/Analysis/cast-trust-base-to-derived.cpp (+105) 
- (modified) clang/test/Analysis/osobject-retain-release.cpp (+7-5) 


``````````diff
diff --git a/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp b/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
index eda8302595ba4fb..6539f3c1e09e321 100644
--- a/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/DynamicTypePropagation.cpp
@@ -20,6 +20,7 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "clang/AST/OperationKinds.h"
 #include "clang/AST/ParentMap.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/Basic/Builtins.h"
@@ -392,10 +393,6 @@ void DynamicTypePropagation::checkPostCall(const CallEvent &Call,
   }
 }
 
-/// TODO: Handle explicit casts.
-///       Handle C++ casts.
-///
-/// Precondition: the cast is between ObjCObjectPointers.
 ExplodedNode *DynamicTypePropagation::dynamicTypePropagationOnCasts(
     const CastExpr *CE, ProgramStateRef &State, CheckerContext &C) const {
   // We only track type info for regions.
@@ -403,8 +400,19 @@ ExplodedNode *DynamicTypePropagation::dynamicTypePropagationOnCasts(
   if (!ToR)
     return C.getPredecessor();
 
-  if (isa<ExplicitCastExpr>(CE))
+  if (CE->getCastKind() == CK_BaseToDerived) {
+    bool CastSucceeds = true;
+    QualType FromTy = CE->getSubExpr()->getType();
+    QualType ToTy = CE->getType();
+    ToR = ToR->StripCasts(/*StripBaseAndDerivedCasts=*/true);
+    State = setDynamicTypeAndCastInfo(State, ToR, FromTy, ToTy, CastSucceeds);
+    return C.addTransition(State);
+  }
+
+  // TODO: Handle the rest of explicit casts, inluding the regular C++ casts.
+  if (isa<ExplicitCastExpr>(CE)) {
     return C.getPredecessor();
+  }
 
   if (const Type *NewTy = getBetterObjCType(CE, C)) {
     State = setDynamicTypeInfo(State, ToR, QualType(NewTy, 0));
@@ -609,9 +617,13 @@ storeWhenMoreInformative(ProgramStateRef &State, SymbolRef Sym,
 /// symbol and the destination type of the cast are unrelated, report an error.
 void DynamicTypePropagation::checkPostStmt(const CastExpr *CE,
                                            CheckerContext &C) const {
+  ProgramStateRef State = C.getState();
+  ExplodedNode *AfterTypeProp = dynamicTypePropagationOnCasts(CE, State, C);
+
   if (CE->getCastKind() != CK_BitCast)
     return;
 
+  ASTContext &ASTCtxt = C.getASTContext();
   QualType OriginType = CE->getSubExpr()->getType();
   QualType DestType = CE->getType();
 
@@ -621,11 +633,6 @@ void DynamicTypePropagation::checkPostStmt(const CastExpr *CE,
   if (!OrigObjectPtrType || !DestObjectPtrType)
     return;
 
-  ProgramStateRef State = C.getState();
-  ExplodedNode *AfterTypeProp = dynamicTypePropagationOnCasts(CE, State, C);
-
-  ASTContext &ASTCtxt = C.getASTContext();
-
   // This checker detects the subtyping relationships using the assignment
   // rules. In order to be able to do this the kindofness must be stripped
   // first. The checker treats every type as kindof type anyways: when the
diff --git a/clang/test/Analysis/cast-trust-base-to-derived.cpp b/clang/test/Analysis/cast-trust-base-to-derived.cpp
new file mode 100644
index 000000000000000..e717e5617867ca9
--- /dev/null
+++ b/clang/test/Analysis/cast-trust-base-to-derived.cpp
@@ -0,0 +1,105 @@
+// RUN: %clang_analyze_cc1 %s -verify \
+// RUN:   -analyzer-checker=core \
+// RUN:   -analyzer-checker=alpha.security.taint.TaintPropagation \
+// RUN:   -analyzer-checker=debug.ExprInspection
+
+// See issue https://github.com/llvm/llvm-project/issues/62663
+
+template <typename T> void clang_analyzer_dump(T);
+void clang_analyzer_warnIfReached();
+void clang_analyzer_numTimesReached();
+void clang_analyzer_isTainted(int);
+
+extern int scanf(const char *format, ...);
+
+class ActionHandler {
+public:
+  virtual ~ActionHandler() = default;
+  virtual void onAction(int x, int &) {
+    clang_analyzer_dump(x + 1); // expected-warning {{101}}
+  }
+};
+
+class MyHandler final : public ActionHandler {
+public:
+  void onAction(int x, int &) override {
+    clang_analyzer_dump(x + 2); // expected-warning {{202}}
+  }
+};
+
+void trust_static_types(ActionHandler *p) {
+  // This variable will help to see if conservative call evaluation happened or not.
+  int invalidation_detector;
+
+  // At this point we don't know anything about the dynamic type of `*p`, thus the `onAction` call might be resolved to the default implementation, matching the static type.
+  invalidation_detector = 1000;
+  p->onAction(100, invalidation_detector);
+  clang_analyzer_dump(invalidation_detector);
+  // expected-warning at -1 {{1000}} on this path we trusted the type, and resolved the call to `ActionHandler::onAction(int, int&)`
+  // expected-warning at -2 {{conj}} on this path we conservatively evaluated the previous call
+
+  clang_analyzer_numTimesReached(); // expected-warning {{2}} we only have that 2 paths here
+  // 1) inlined `ActionHandler::onAction(int, int&)`
+  // 2) conservatively eval called
+
+  // Trust that the `*p` refers to an object with `MyHandler` static type (or to some other sub-class).
+  auto *q = static_cast<MyHandler *>(p);
+  (void)q; // Discard the result of the cast. We already learned the type `p` might refer to.
+
+  invalidation_detector = 2000;
+  p->onAction(200, invalidation_detector);
+  clang_analyzer_dump(invalidation_detector);
+  // expected-warning at -1 {{2000}} on this path we trusted the type, and resolved the call to `MyHandler::onAction(int, int&)`
+  // expected-warning at -2 {{conj}} on this path we conservatively evaluated the previous call
+
+  clang_analyzer_numTimesReached(); // expected-warning {{3}} we only have 3 paths here, not 4
+  // 1) inlined 2 different callees
+  // 2) inlined only 1st
+  // 3) non were inlined
+  // 4) inlined only the second: This can't happen because if we conservative called a specific function on a path, we will always evaluate it like that.
+  //    See ExprEngine::BifurcateCall and DynamicDispatchBifurcationMap.
+}
+
+// -------
+
+class Base {
+public:
+  virtual void OnRecvCancel(int port) = 0;
+};
+class Handler final : public Base {
+public:
+  void OnRecvCancel(int port) override {
+    clang_analyzer_dump(100 + port); // expected-warning {{+ 150}}
+    clang_analyzer_isTainted(port);  // expected-warning {{YES}}
+  }
+};
+class PParent  {
+public:
+  bool OnMessageReceived();
+};
+class Actor {
+public:
+  explicit Actor(Base* aRequest) : m(aRequest) {}
+protected:
+  Base* m;
+};
+
+class Parent : public Actor, public PParent {
+public:
+  explicit Parent(Base* aRequest) : Actor(aRequest) {}
+  bool RecvCancel(int port) {
+    clang_analyzer_dump(200 + port); // expected-warning {{+ 200}}
+    clang_analyzer_isTainted(port);  // expected-warning {{YES}}
+    Handler* foo = (Handler*)m;
+    foo->OnRecvCancel(port + 50);
+    return true;
+  }
+};
+
+bool PParent::OnMessageReceived() {
+  int port;
+  scanf("%i", &port);
+  clang_analyzer_isTainted(port); // expected-warning {{YES}}
+  Parent* foo = static_cast<Parent*>(this);
+  return foo->RecvCancel(port);
+}
diff --git a/clang/test/Analysis/osobject-retain-release.cpp b/clang/test/Analysis/osobject-retain-release.cpp
index 2ae5752f4402374..b4085813a72a1aa 100644
--- a/clang/test/Analysis/osobject-retain-release.cpp
+++ b/clang/test/Analysis/osobject-retain-release.cpp
@@ -492,11 +492,13 @@ void check_required_cast() {
 
 void check_cast_behavior(OSObject *obj) {
   OSArray *arr1 = OSDynamicCast(OSArray, obj);
-  clang_analyzer_eval(arr1 == obj); // expected-warning{{TRUE}}
-                                    // expected-note at -1{{TRUE}}
-                                    // expected-note at -2{{Assuming 'arr1' is not equal to 'obj'}}
-                                    // expected-warning at -3{{FALSE}}
-                                    // expected-note at -4   {{FALSE}}
+  clang_analyzer_eval(arr1 == obj); // #check_cast_behavior_1
+  // expected-warning@#check_cast_behavior_1 {{TRUE}}
+  // expected-note@#check_cast_behavior_1    {{TRUE}}
+  // expected-note@#check_cast_behavior_1{{Assuming 'arr1' is equal to 'obj'}}
+  // expected-warning@#check_cast_behavior_1 {{FALSE}}
+  // expected-note@#check_cast_behavior_1    {{FALSE}}
+  // expected-note@#check_cast_behavior_1{{Assuming 'arr1' is not equal to 'obj'}}
   OSArray *arr2 = OSRequiredCast(OSArray, obj);
   clang_analyzer_eval(arr2 == obj); // expected-warning{{TRUE}}
                                     // expected-note at -1{{TRUE}}

``````````

</details>


https://github.com/llvm/llvm-project/pull/69057