[clang] 9037730 - [analyzer] Support allocClassWithName in OSObjectCStyleCast checker

[Valeriy Savchenko via cfe-commits]

Author: Valeriy Savchenko
Date: 2021-03-30T15:58:06+03:00
New Revision: 90377308de6cac8239bc1a1dcd32b57b9ec91444

URL: https://github.com/llvm/llvm-project/commit/90377308de6cac8239bc1a1dcd32b57b9ec91444
DIFF: https://github.com/llvm/llvm-project/commit/90377308de6cac8239bc1a1dcd32b57b9ec91444.diff

LOG: [analyzer] Support allocClassWithName in OSObjectCStyleCast checker

`allocClassWithName` allocates an object with the given type.
The type is actually provided as a string argument (type's name).
This creates a possibility for not particularly useful warnings
from the analyzer.

In order to combat with those, this patch checks for casts of the
`allocClassWithName` results to types mentioned directly as its
argument.  All other uses of this method should be reasoned about
as before.

rdar://72165694

Differential Revision: https://reviews.llvm.org/D99500

Added: 
    

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

Removed: 
    


################################################################################
diff  --git a/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp b/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
index 270b66dab020..0a8379d9ab99 100644
--- a/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/OSObjectCStyleCast.cpp
@@ -32,7 +32,21 @@ class OSObjectCStyleCastChecker : public Checker<check::ASTCodeBody> {
   void checkASTCodeBody(const Decl *D, AnalysisManager &AM,
                         BugReporter &BR) const;
 };
+} // namespace
+
+namespace clang {
+namespace ast_matchers {
+AST_MATCHER_P(StringLiteral, mentionsBoundType, std::string, BindingID) {
+  return Builder->removeBindings([this, &Node](const BoundNodesMap &Nodes) {
+    const auto &BN = Nodes.getNode(this->BindingID);
+    if (const auto *ND = BN.get<NamedDecl>()) {
+      return ND->getName() != Node.getString();
+    }
+    return true;
+  });
 }
+} // end namespace ast_matchers
+} // end namespace clang
 
 static void emitDiagnostics(const BoundNodes &Nodes,
                             BugReporter &BR,
@@ -63,22 +77,41 @@ static decltype(auto) hasTypePointingTo(DeclarationMatcher DeclM) {
   return hasType(pointerType(pointee(hasDeclaration(DeclM))));
 }
 
-void OSObjectCStyleCastChecker::checkASTCodeBody(const Decl *D, AnalysisManager &AM,
+void OSObjectCStyleCastChecker::checkASTCodeBody(const Decl *D,
+                                                 AnalysisManager &AM,
                                                  BugReporter &BR) const {
 
   AnalysisDeclContext *ADC = AM.getAnalysisDeclContext(D);
 
   auto DynamicCastM = callExpr(callee(functionDecl(hasName("safeMetaCast"))));
-
-  auto OSObjTypeM = hasTypePointingTo(cxxRecordDecl(isDerivedFrom("OSMetaClassBase")));
+  // 'allocClassWithName' allocates an object with the given type.
+  // The type is actually provided as a string argument (type's name).
+  // This makes the following pattern possible:
+  //
+  // Foo *object = (Foo *)allocClassWithName("Foo");
+  //
+  // While OSRequiredCast can be used here, it is still not a useful warning.
+  auto AllocClassWithNameM = callExpr(
+      callee(functionDecl(hasName("allocClassWithName"))),
+      // Here we want to make sure that the string argument matches the
+      // type in the cast expression.
+      hasArgument(0, stringLiteral(mentionsBoundType(WarnRecordDecl))));
+
+  auto OSObjTypeM =
+      hasTypePointingTo(cxxRecordDecl(isDerivedFrom("OSMetaClassBase")));
   auto OSObjSubclassM = hasTypePointingTo(
-    cxxRecordDecl(isDerivedFrom("OSObject")).bind(WarnRecordDecl));
-
-  auto CastM = cStyleCastExpr(
-          allOf(hasSourceExpression(allOf(OSObjTypeM, unless(DynamicCastM))),
-          OSObjSubclassM)).bind(WarnAtNode);
-
-  auto Matches = match(stmt(forEachDescendant(CastM)), *D->getBody(), AM.getASTContext());
+      cxxRecordDecl(isDerivedFrom("OSObject")).bind(WarnRecordDecl));
+
+  auto CastM =
+      cStyleCastExpr(
+          allOf(OSObjSubclassM,
+                hasSourceExpression(
+                    allOf(OSObjTypeM,
+                          unless(anyOf(DynamicCastM, AllocClassWithNameM))))))
+          .bind(WarnAtNode);
+
+  auto Matches =
+      match(stmt(forEachDescendant(CastM)), *D->getBody(), AM.getASTContext());
   for (BoundNodes Match : Matches)
     emitDiagnostics(Match, BR, ADC, this);
 }

diff  --git a/clang/test/Analysis/os_object_base.h b/clang/test/Analysis/os_object_base.h
index 4698185f2b3c..c3d5d6271d48 100644
--- a/clang/test/Analysis/os_object_base.h
+++ b/clang/test/Analysis/os_object_base.h
@@ -66,6 +66,7 @@ struct OSObject : public OSMetaClassBase {
 
 struct OSMetaClass : public OSMetaClassBase {
   virtual OSObject * alloc() const;
+  static OSObject * allocClassWithName(const char * name);
   virtual ~OSMetaClass(){}
 };
 

diff  --git a/clang/test/Analysis/osobjectcstylecastchecker_test.cpp b/clang/test/Analysis/osobjectcstylecastchecker_test.cpp
index a5ebb2823a92..aa43a18e6346 100644
--- a/clang/test/Analysis/osobjectcstylecastchecker_test.cpp
+++ b/clang/test/Analysis/osobjectcstylecastchecker_test.cpp
@@ -37,3 +37,12 @@ unsigned no_warn_on_other_type_cast(A *a) {
   return b->getCount();
 }
 
+unsigned no_warn_alloc_class_with_name() {
+  OSArray *a = (OSArray *)OSMetaClass::allocClassWithName("OSArray"); // no warning
+  return a->getCount();
+}
+
+unsigned warn_alloc_class_with_name() {
+  OSArray *a = (OSArray *)OSMetaClass::allocClassWithName("OSObject"); // 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();
+}