From 8c5ca7c6e62c203b9642dca2a1d9118c36777ad5 Mon Sep 17 00:00:00 2001
From: Artem Dergachev <artem.dergachev@gmail.com>
Date: Sat, 21 Nov 2020 01:41:31 -0800
Subject: [PATCH] [analyzer] OSObjectCStyleCast: Improve warning message.

Suggest OSRequiredCast as a closer alternative to C-style cast.
Explain how to decide.
---
 .../StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp  | 21 ++++++++++++---------
 .../Analysis/osobjectcstylecastchecker_test.cpp     |  2 +-
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp b/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
index 53ed0e1..270b66d 100644
--- a/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
@@ -24,32 +24,36 @@ using namespace ento;
 using namespace ast_matchers;
 
 namespace {
-
-const char *WarnAtNode = "OSObjCast";
+static constexpr const char *const WarnAtNode = "WarnAtNode";
+static constexpr const char *const WarnRecordDecl = "WarnRecordDecl";
 
 class OSObjectCStyleCastChecker : public Checker<check::ASTCodeBody> {
 public:
-  void checkASTCodeBody(const Decl *D,
-                        AnalysisManager &AM,
+  void checkASTCodeBody(const Decl *D, AnalysisManager &AM,
                         BugReporter &BR) const;
 };
+}
 
 static void emitDiagnostics(const BoundNodes &Nodes,
                             BugReporter &BR,
                             AnalysisDeclContext *ADC,
                             const OSObjectCStyleCastChecker *Checker) {
   const auto *CE = Nodes.getNodeAs<CastExpr>(WarnAtNode);
-  assert(CE);
+  const CXXRecordDecl *RD = Nodes.getNodeAs<CXXRecordDecl>(WarnRecordDecl);
+  assert(CE && RD);
 
   std::string Diagnostics;
   llvm::raw_string_ostream OS(Diagnostics);
-  OS << "C-style cast of OSObject. Use OSDynamicCast instead.";
+  OS << "C-style cast of an OSObject is prone to type confusion attacks; "
+     << "use 'OSRequiredCast' if the object is definitely of type '"
+     << RD->getNameAsString() << "', or 'OSDynamicCast' followed by "
+     << "a null check if unsure",
 
   BR.EmitBasicReport(
     ADC->getDecl(),
     Checker,
     /*Name=*/"OSObject C-Style Cast",
-    /*BugCategory=*/"Security",
+    categories::SecurityError,
     OS.str(),
     PathDiagnosticLocation::createBegin(CE, BR.getSourceManager(), ADC),
     CE->getSourceRange());
@@ -68,7 +72,7 @@ void OSObjectCStyleCastChecker::checkASTCodeBody(const Decl *D, AnalysisManager
 
   auto OSObjTypeM = hasTypePointingTo(cxxRecordDecl(isDerivedFrom("OSMetaClassBase")));
   auto OSObjSubclassM = hasTypePointingTo(
-    cxxRecordDecl(isDerivedFrom("OSObject")));
+    cxxRecordDecl(isDerivedFrom("OSObject")).bind(WarnRecordDecl));
 
   auto CastM = cStyleCastExpr(
           allOf(hasSourceExpression(allOf(OSObjTypeM, unless(DynamicCastM))),
@@ -78,7 +82,6 @@ void OSObjectCStyleCastChecker::checkASTCodeBody(const Decl *D, AnalysisManager
   for (BoundNodes Match : Matches)
     emitDiagnostics(Match, BR, ADC, this);
 }
-}
 
 void ento::registerOSObjectCStyleCast(CheckerManager &Mgr) {
   Mgr.registerChecker<OSObjectCStyleCastChecker>();
diff --git a/clang/test/Analysis/osobjectcstylecastchecker_test.cpp b/clang/test/Analysis/osobjectcstylecastchecker_test.cpp
index fabed7e..a5ebb28 100644
--- a/clang/test/Analysis/osobjectcstylecastchecker_test.cpp
+++ b/clang/test/Analysis/osobjectcstylecastchecker_test.cpp
@@ -13,7 +13,7 @@ struct B : public A {
 };
 
 unsigned warn_on_explicit_downcast(OSObject * obj) {
-  OSArray *a = (OSArray *) obj; // expected-warning{{C-style cast of OSObject. Use OSDynamicCast instead}}
+  OSArray *a = (OSArray *) obj; // expected-warning{{C-style cast of an OSObject is prone to type confusion attacks; use 'OSRequiredCast' if the object is definitely of type 'OSArray', or 'OSDynamicCast' followed by a null check if unsure}}
   return a->getCount();
 }
 
-- 
2.7.4