[analyzer] CastValueChecker: Model casts

[Charusso]

Summary

It models the LLVM casts:

• cast<>
• dyn_cast<>
• cast_or_null<>
• dyn_cast_or_null<>

It has a very basic support without checking the classof() function.

[Charusso]

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

176

This CXXMemberCallExpr business is necessary? I am not sure as I have not seen it widely used.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

26–28

Kinda nice, but a simple pointer-to-member would have been sufficient and much more lightweight:

using CastCheck = void (CastValueChecker::*)(const CallExpr *,
                         DefinedOrUnknownSVal, CheckerContext &);

(not sure, i don't fully understand the performance implications of using std::function)

41–44

Please add an argument count check to your CallDescriptions.

Otherwise accessing argument 0 may result in a crash when we're analyzing code that isn't LLVM but defines a function with the same name which also has no arguments.

That's a fairly common source of crashes.

61–64

The assume operation is expensive. Let's avoid doing it twice:

State = State->assume(ParamDV, true);
if (!State)
  return;

68–69

Let's do ...getType()->getPointeeCXXRecordDecl()->getNameAsString(). It's shorter and more on point. (might need to check for null decl though).

76

This statement needs a predicate.

I suggest: Assuming dynamic cast from 'A' to 'B' succeeds.

79

true! We don't want a note about every random cast to bring in dozens of nested stack frames.

93

Just drop Invalidate. Situations where you actually notice the difference are extremely rare. In this case there's nothing to invalidate because there's no existing value for the call-expression.

101

Assuming dynamic cast from 'A' to 'B' fails.

Later when we add support for dynamic type tracking, we may as well add a "Knowing..." piece :)

176

It definitely needs different handling because this cannot ever be null.

If you haven't seen it widely used, it's probably not necessary; i too rarely see those.

178

Call.getArgSVal(0).

clang/test/Analysis/cast-value.cpp

35

debug.ExprInspection is your friend!

I'd also call for making the tests more isolated. In this test you're hardcoding a fairly arbitrary and random execution path that the analyzer chose through your function. It's generally interesting what paths do we explore first, but that's not what the patch is about.

Here's an example of a test that i would have written:

void foo(Shape *S) {
  Circle *C = dyn_cast_or_null<Circle>(S);
  clang_analyzer_numTimesReached(); // expected-warning{{3}}
  if (S && C)
    clang_analyzer_eval(C == S); // expected-warning{{TRUE}}
  if (!S)
    clang_analyzer_eval(!C); // expected-warning{{TRUE}}
  if (S && !C)
    clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
}

(you might want to test notes separately)

[NoQ]

The overall code looks great! It's well-structured and easy to understand, thanks!

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

125

Assuming null pointer is passed into cast o.o

[Charusso]

• Fix
• New getNoteTag() which accepts a plain note.

[Charusso]

Thanks! My mind was really set to actually model these with classof(), whoops.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

26–28

It took me an hour to realize I have to push the class as the signature of the function to create a member-callback. I think it is not that lightweight, but it is modern. I believe it is made for that purpose when you could write Foo[13] or *(13 + P), where the latter creeps me out.

Thanks for the copy-pasteable code, I really enjoy to see what do you think exactly, but this is the first time when I would pick my idea. (I would had recommended that modern way in your patch just I thought it is not working with methods.)

clang/test/Analysis/cast-value.cpp

35

Great idea, thanks!

[Charusso]

• Fix a typo.

[NoQ]

We should add at least some tests for the other three functions.

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

278

"Model implementation of custom RTTIs."

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

250

Great! I recognize the need for such function now.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

26–28

Ok then!

59–60

I suspect that this may crash if CD is an anonymous structure. So you should use getNameAsString() instead.

62

Use after free! QualType::getAsString() returns a temporary std::string. You're returning a StringRef that outlives the string it refers to. The solution is usually to return the std::string by value.

*summons @rnkovacs*

Generally, i'd rather bail out on this branch. If we're seeing a dyn_cast of something that isn't a class, we're already doing something wrong.

clang/test/Analysis/cast-value.cpp

4

Nice!

I guess putting them into separate files would have been easier this time.

[Charusso]

• Fix.
• More tests.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

62

Well, at least it does not crash. Thanks! I like that general return which we could evolve further. It is the same story like Assuming....

[Charusso]

• Simplify the new getNoteTag().

[Szelethus]

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

279

This checker only does modeling, but isn't hidden. Should we hide it?

[NoQ]

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

101–102

I believe we want to put this checker as well as the return value checker into apiModeling.llvm.

This doesn't prevent the checkers from being generally useful for other packages: if we want to add support for other APIs, we'll simply reserve a different checker name for the same checker object (with internal flags to enable/disable particular API groups).

279

Dunno :)

I think we should hide the checkers that model core language functionality (including standard library functionality, as defining your own stuff in namespace std has undefined behavior in most cases, at least in c++) but we shouldn't hide checkers for specific user-made APIs because people may want to disable them simply because they have their own conflicting API.

Also, btw, the whole apiModeling package is currently hidden.

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

2

Let's mention RTTI here as well. Otherwise it sounds weird, as all actual casts are applied by ExprEngine.

10

And here.

62

No, it's not the same story. It's not "some valid situation that we don't support yet". It's "a normally impossible situation that indicates that we should not try to model this function because there's no reason for us to believe we know what it does".

(yay, i understood what you mean without a reference; that was hard^^)

[Charusso]

• Move to apiModeling.llvm.
• Prevent unknown casts.
• Refactor.

[NoQ]

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

59

You should still check if Cast->getType()->getPointeeCXXRecordDecl() returns null, otherwise you can crash.

But when it's null, you should immediately return false from your evalCall.

[Charusso]

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

173

If we cannot obtain any of the CXXRecordDecls then we bail out. You are right about that feature, and I like it.

[NoQ]

Whoops. I underestimated you (:

Ok, anyway, this was my last comment. Great job! Remember to make sure that it works on the actual LLVM, not only on tests; mocked-up test headers are very easy to get wrong.

[Charusso]

In D64374#1577235, @NoQ wrote:

Whoops. I underestimated you (:

Ok, anyway, this was my last comment. Great job! Remember to make sure that it works on the actual LLVM, not only on tests; mocked-up test headers are very easy to get wrong.

The only crash-able code was that:

#0 Calling llvm::unique_dyn_cast

where:

ProgramStateRef ExprEngine::createTemporaryRegionIfNeeded():
Assertion `!InitValWithAdjustments.getAs<Loc>() || Loc::isLocType(Result->getType()) || Result->getType()->isMemberPointerType()'

because of cast<>:

template <class X, class Y>
LLVM_NODISCARD inline auto unique_dyn_cast(std::unique_ptr<Y> &Val)
    -> decltype(cast<X>(Val)) {
  if (!isa<X>(Val))
    return nullptr;
  return cast<X>(std::move(Val));
}

(source: https://llvm.org/doxygen/Casting_8h_source.html)

So we bypass every error with that CXXRecordDecl checking from now. Would we like to model that?

[Charusso]

• Add the forgotten llvm namespace to the CallDescription.

[NoQ]

In D64374#1577262, @Charusso wrote:

• Add the forgotten llvm namespace to the CallDescription.

Nice!

In D64374#1577254, @Charusso wrote:

The only crash-able code was that:

#0 Calling llvm::unique_dyn_cast

Mmm. What? Can you provide more info, eg. the full backtrace?

[Charusso]

In D64374#1577266, @NoQ wrote:

Can you provide more info, eg. the full backtrace?

Well, unique_dyn_cast<> and unique_dyn_cast_or_null<> is used like 20 times in the LLVM codebase, whoops. We want to model it.

Full info: Edit: removed due it breaks the entire page.

[NoQ]

Mmm. Ok, am i understanding correctly that this was crashing because you bound a 0 (Loc) to a prvalue expression of type unique_ptr? Yeah, right, don't do that :)

used like 20 times in the LLVM codebase

That doesn't sound like it's high on our list. That's not too many times. Probably very few false positives come out of it. Also modeling smart pointers is much harder than modeling regular pointers. We could add some modeling into SmartPtrChecker, but that'd require actually developing that checker :)

[Charusso]

Thanks for the review!

In D64374#1577279, @NoQ wrote:

That doesn't sound like it's high on our list. That's not too many times. Probably very few false positives come out of it. Also modeling smart pointers is much harder than modeling regular pointers.

Hm, because it is smart, probably it is out of our scope. Thanks for pointing it out!

[Charusso]

• Add the llvm namespace to the test file.

[Szelethus]

I guess getAs and castAs methods could join the party too. I'm evaluating plenty of results on LLVM, and those seem to be in the same problem category.

[Szelethus]

Do you have plans to extend this checker with the modeling of isa<>? I might take that off your shoulder if not :)

[Charusso]

In D64374#1594617, @Szelethus wrote:

I guess getAs and castAs methods could join the party too. I'm evaluating plenty of results on LLVM, and those seem to be in the same problem category.

In D64374#1618694, @Szelethus wrote:

Do you have plans to extend this checker with the modeling of isa<>? I might take that off your shoulder if not :)

Hey! Sorry for the late response, just it is a heavy change. This patch creates about 200 TP, I have checked out every of the new reports.
D65889 creates about 400 TP as an estimation, then isa() is the next goal. Thanks you for the ideas!

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

 def MPI : Package<"mpi">, ParentPackage<OptIn>;
 
 def LLVM : Package<"llvm">;
 def LLVMAlpha : Package<"llvm">, ParentPackage<Alpha>;
 
 // The APIModeling package is for checkers that model APIs and don't perform
 // any diagnostics. These checkers are always turned on; this package is
 // intended for API modeling that is not controlled by the target triple.
 def APIModeling : Package<"apiModeling">, Hidden;
 def GoogleAPIModeling : Package<"google">, ParentPackage<APIModeling>, Hidden;
 
 def Debug : Package<"debug">, Hidden;
 
 def CloneDetectionAlpha : Package<"clone">, ParentPackage<Alpha>;
 
 def NonDeterminismAlpha : Package<"nondeterminism">, ParentPackage<Alpha>;
 
 //===----------------------------------------------------------------------===//

 } // end "nullability"
 
 //===----------------------------------------------------------------------===//
 // APIModeling.
 //===----------------------------------------------------------------------===//
 
 let ParentPackage = APIModeling in {
 
+def CastValueChecker : Checker<"CastValue">,
+  HelpText<"Model casts">,
+  Documentation<NotDocumented>;
+
 def ReturnValueChecker : Checker<"ReturnValue">,
   HelpText<"Model the guaranteed boolean return value of function calls">,
   Documentation<NotDocumented>;
 
 def StdCLibraryFunctionsChecker : Checker<"StdCLibraryFunctions">,
   HelpText<"Improve modeling of the C standard library functions">,
   Documentation<NotDocumented>;
 

clang/include/clang/StaticAnalyzer/Core/PathSensitive/CheckerContext.h

public:
   ///        bug path significantly shorter.
   const NoteTag *getNoteTag(std::function<std::string(BugReport &)> &&Cb,
                             bool IsPrunable = false) {
     return getNoteTag(
         [Cb](BugReporterContext &, BugReport &BR) { return Cb(BR); },
         IsPrunable);
   }
 
+  /// A shorthand version of getNoteTag that accepts a plain note.
+  ///
+  /// @param Note The note.
+  /// @param IsPrunable Whether the note is prunable. It allows BugReporter
+  ///        to omit the note from the report if it would make the displayed
+  ///        bug path significantly shorter.
+  const NoteTag *getNoteTag(StringRef Note, bool IsPrunable = false) {
+    return getNoteTag(
+        [Note](BugReporterContext &, BugReport &BR) { return Note; },
+        IsPrunable);
+  }
+
   /// Returns the word that should be used to refer to the declaration
   /// in the report.
   StringRef getDeclDescription(const Decl *D);
 
   /// Get the declaration of the called function (path-sensitive).
   const FunctionDecl *getCalleeDecl(const CallExpr *CE) const;
 
   /// Get the name of the called function (path-sensitive).

clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt

add_clang_library(clangStaticAnalyzerCheckers
   BlockInCriticalSectionChecker.cpp
   BoolAssignmentChecker.cpp
   BuiltinFunctionChecker.cpp
   CStringChecker.cpp
   CStringSyntaxChecker.cpp
   CallAndMessageChecker.cpp
   CastSizeChecker.cpp
   CastToStructChecker.cpp
+  CastValueChecker.cpp
   CheckObjCDealloc.cpp
   CheckObjCInstMethSignature.cpp
   CheckSecuritySyntaxOnly.cpp
   CheckSizeofPointer.cpp
   CheckerDocumentation.cpp
   ChrootChecker.cpp
   CloneChecker.cpp
   ConversionChecker.cpp

clang/lib/StaticAnalyzer/Checkers/CastValueChecker.cpp

+//===- CastValueChecker - Applies casts -------------------------*- 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 defines CastValueChecker which models casts.
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.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/Optional.h"
+
+using namespace clang;
+using namespace ento;
+
+namespace {
+class CastValueChecker : public Checker<eval::Call> {
+  using CastCheck =
+      std::function<void(const CastValueChecker *, const CallExpr *,
+                         DefinedOrUnknownSVal, CheckerContext &)>;
+
+public:
+  // We have three cases to evaluate a cast:
+  // 1) The parameter is non-null, the return value is non-null
+  // 2) The parameter is non-null, the return value is null
+  // 3) The parameter is null, the return value is null
+  //
+  // cast: 1;  dyn_cast: 1, 2;  cast_or_null: 1, 3;  dyn_cast_or_null: 1, 2, 3.
+  bool evalCall(const CallEvent &Call, CheckerContext &C) const;
+
+private:
+  const CallDescriptionMap<CastCheck> CDM = {
+      {{{"cast"}, 1}, &CastValueChecker::evalCast},
+      {{{"dyn_cast"}, 1}, &CastValueChecker::evalDynCast},
+      {{{"cast_or_null"}, 1}, &CastValueChecker::evalCastOrNull},
+      {{{"dyn_cast_or_null"}, 1}, &CastValueChecker::evalDynCastOrNull}};
+
+  void evalCast(const CallExpr *CE, DefinedOrUnknownSVal ParamDV,
+                CheckerContext &C) const;
+  void evalDynCast(const CallExpr *CE, DefinedOrUnknownSVal ParamDV,
+                   CheckerContext &C) const;
+  void evalCastOrNull(const CallExpr *CE, DefinedOrUnknownSVal ParamDV,
+                      CheckerContext &C) const;
+  void evalDynCastOrNull(const CallExpr *CE, DefinedOrUnknownSVal ParamDV,
+                         CheckerContext &C) const;
+};
+} // namespace
+
+static StringRef getCastName(const Expr *Cast) {
+  QualType Ty = Cast->getType();
+  if (const CXXRecordDecl *CD = Ty->getPointeeCXXRecordDecl())
+    return CD->getName();
+
+  return Ty.getAsString();
+}
+
+static void evalNonNullParamNonNullReturn(const CallExpr *CE,
+                                          DefinedOrUnknownSVal ParamDV,
+                                          CheckerContext &C) {
+  ProgramStateRef State = C.getState()->assume(ParamDV, true);
+  if (!State)
+    return;
+
+  State = State->BindExpr(CE, C.getLocationContext(), ParamDV, false);
+
+  std::string CastFromName = getCastName(CE->getArg(0));
+  std::string CastToName = getCastName(CE);
+
+  const NoteTag *CastTag = C.getNoteTag(
+      [CastFromName, CastToName](BugReport &) -> std::string {
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+
+        Out << "Assuming dynamic cast from '" << CastFromName << "' to '"
+            << CastToName << "' succedds";
+        return Out.str();
+      },
+      /*IsPrunable=*/true);
+
+  C.addTransition(State, CastTag);
+}
+
+static void evalNonNullParamNullReturn(const CallExpr *CE,
+                                       DefinedOrUnknownSVal ParamDV,
+                                       CheckerContext &C) {
+  ProgramStateRef State = C.getState()->assume(ParamDV, true);
+  if (!State)
+    return;
+
+  State = State->BindExpr(CE, C.getLocationContext(),
+                          C.getSValBuilder().makeNull(), false);
+
+  std::string CastFromName = getCastName(CE->getArg(0));
+  std::string CastToName = getCastName(CE);
+
+  const NoteTag *CastTag = C.getNoteTag(
+      [CastFromName, CastToName](BugReport &) -> std::string {
+        SmallString<128> Msg;
+        llvm::raw_svector_ostream Out(Msg);
+
+        Out << "Assuming dynamic cast from '" << CastFromName << "' to '"
+            << CastToName << "' fails";
+        return Out.str();
+      },
+      /*IsPrunable=*/true);
+
+  C.addTransition(State, CastTag);
+}
+
+static void evalNullParamNullReturn(const CallExpr *CE,
+                                    DefinedOrUnknownSVal ParamDV,
+                                    CheckerContext &C) {
+  ProgramStateRef State = C.getState()->assume(ParamDV, false);
+  if (!State)
+    return;
+
+  State = State->BindExpr(CE, C.getLocationContext(),
+                          C.getSValBuilder().makeNull(), false);
+
+  const NoteTag *CastTag =
+      C.getNoteTag("Assuming null pointer is passed into cast",
+                   /*IsPrunable=*/true);
+
+  C.addTransition(State, CastTag);
+}
+
+void CastValueChecker::evalCast(const CallExpr *CE,
+                                DefinedOrUnknownSVal ParamDV,
+                                CheckerContext &C) const {
+  evalNonNullParamNonNullReturn(CE, ParamDV, C);
+}
+
+void CastValueChecker::evalDynCast(const CallExpr *CE,
+                                   DefinedOrUnknownSVal ParamDV,
+                                   CheckerContext &C) const {
+  evalNonNullParamNonNullReturn(CE, ParamDV, C);
+  evalNonNullParamNullReturn(CE, ParamDV, C);
+}
+
+void CastValueChecker::evalCastOrNull(const CallExpr *CE,
+                                      DefinedOrUnknownSVal ParamDV,
+                                      CheckerContext &C) const {
+  evalNonNullParamNonNullReturn(CE, ParamDV, C);
+  evalNullParamNullReturn(CE, ParamDV, C);
+}
+
+void CastValueChecker::evalDynCastOrNull(const CallExpr *CE,
+                                         DefinedOrUnknownSVal ParamDV,
+                                         CheckerContext &C) const {
+  evalNonNullParamNonNullReturn(CE, ParamDV, C);
+  evalNonNullParamNullReturn(CE, ParamDV, C);
+  evalNullParamNullReturn(CE, ParamDV, C);
+}
+
+bool CastValueChecker::evalCall(const CallEvent &Call,
+                                CheckerContext &C) const {
+  const CastCheck *Check = CDM.lookup(Call);
+  if (!Check)
+    return false;
+
+  const auto *CE = cast<CallExpr>(Call.getOriginExpr());
+  if (!CE)
+    return false;
+
+  SVal ParamV = Call.getArgSVal(0);
+  auto ParamDV = ParamV.getAs<DefinedOrUnknownSVal>();
+  if (!ParamDV)
+    return false;
+
+  (*Check)(this, CE, *ParamDV, C);
+  return true;
+}
+
+void ento::registerCastValueChecker(CheckerManager &Mgr) {
+  Mgr.registerChecker<CastValueChecker>();
+}
+
+bool ento::shouldRegisterCastValueChecker(const LangOptions &LO) {
+  return true;
+}

clang/test/Analysis/cast-value.cpp

+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,apiModeling.CastValue,debug.ExprInspection \
+// RUN:  -verify=logic %s
+// RUN: %clang_analyze_cc1 \
+// RUN:  -analyzer-checker=core,apiModeling.CastValue \
+// RUN:  -analyzer-output=text -verify %s
+
+void clang_analyzer_numTimesReached();
+void clang_analyzer_warnIfReached();
+void clang_analyzer_eval(bool);
+
+template <class X, class Y>
+const X *dyn_cast_or_null(Y Value);
+
+class Shape {};
+class Triangle : public Shape {};
+class Circle : public Shape {};
+
+namespace test_dyn_cast_or_null {
+void evalLogic(const Shape *S) {
+  const Circle *C = dyn_cast_or_null<Circle>(S);
+  clang_analyzer_numTimesReached(); // logic-warning {{3}}
+
+  if (S && C)
+    clang_analyzer_eval(C == S); // logic-warning {{TRUE}}
+
+  if (!S)
+    clang_analyzer_eval(!C); // logic-warning {{TRUE}}
+
+  if (S && !C)
+    clang_analyzer_warnIfReached(); // logic-warning {{REACHABLE}}
+}
+
+void evalNonNullParamNonNullReturn(const Shape *S) {
+  const auto *C = dyn_cast_or_null<Circle>(S);
+  // expected-note@-1 {{Assuming dynamic cast from 'Shape' to 'Circle' succedds}}
+  // expected-note@-2 {{Assuming pointer value is null}}
+  // expected-note@-3 {{'C' initialized here}}
+
+  (void)(1 / !(bool)C);
+  // expected-note@-1 {{'C' is non-null}}
+  // expected-note@-2 {{Division by zero}}
+  // expected-warning@-3 {{Division by zero}}
+  // logic-warning@-4 {{Division by zero}}
+}
+
+void evalNonNullParamNullReturn(const Shape *S) {
+  const auto *C = dyn_cast_or_null<Circle>(S);
+  // expected-note@-1 {{Assuming dynamic cast from 'Shape' to 'Circle' fails}}
+  // expected-note@-2 {{Assuming pointer value is null}}
+
+  if (const auto *T = dyn_cast_or_null<Triangle>(S)) {
+    // expected-note@-1 {{Assuming dynamic cast from 'Shape' to 'Triangle' succedds}}
+    // expected-note@-2 {{'T' initialized here}}
+    // expected-note@-3 {{'T' is non-null}}
+    // expected-note@-4 {{Taking true branch}}
+
+    (void)(1 / !T);
+    // expected-note@-1 {{'T' is non-null}}
+    // expected-note@-2 {{Division by zero}}
+    // expected-warning@-3 {{Division by zero}}
+    // logic-warning@-4 {{Division by zero}}
+  }
+}
+
+void evalNullParamNullReturn(const Shape *S) {
+  const auto *C = dyn_cast_or_null<Circle>(S);
+  // expected-note@-1 {{Assuming null pointer is passed into cast}}
+  // expected-note@-2 {{'C' initialized to a null pointer value}}
+
+  (void)(1 / (bool)C);
+  // expected-note@-1 {{Division by zero}}
+  // expected-warning@-2 {{Division by zero}}
+  // logic-warning@-3 {{Division by zero}}
+}
+} // namespace test_dyn_cast_or_null