[llvm-branch-commits] [clang] 8c5ca7c - [analyzer] OSObjectCStyleCast: Improve warning message.

[Artem Dergachev via llvm-branch-commits]

Author: Artem Dergachev
Date: 2020-12-10T19:46:33-08:00
New Revision: 8c5ca7c6e62c203b9642dca2a1d9118c36777ad5

URL: https://github.com/llvm/llvm-project/commit/8c5ca7c6e62c203b9642dca2a1d9118c36777ad5
DIFF: https://github.com/llvm/llvm-project/commit/8c5ca7c6e62c203b9642dca2a1d9118c36777ad5.diff

LOG: [analyzer] OSObjectCStyleCast: Improve warning message.

Suggest OSRequiredCast as a closer alternative to C-style cast.
Explain how to decide.

Added: 
    

Modified: 
    clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
    clang/test/Analysis/osobjectcstylecastchecker_test.cpp

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp b/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
index 53ed0e187a4c..270b66dab020 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 fabed7ee34b1..a5ebb2823a92 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();
 }