Author: Ryosuke Niwa Date: 2025-03-12T20:21:22-07:00 New Revision: 4dcba5e08dc9cf2e5ec3f0df682f8ecc88e815be
URL: https://github.com/llvm/llvm-project/commit/4dcba5e08dc9cf2e5ec3f0df682f8ecc88e815be DIFF: https://github.com/llvm/llvm-project/commit/4dcba5e08dc9cf2e5ec3f0df682f8ecc88e815be.diff LOG: [alpha.webkit.ForwardDeclChecker] Add a new WebKit checker for forward declarations (#130554) Add a new static analyzer which emits warnings for function call arguments, local variables, and member variables that are only forward declared. These forward declaration prevents other WebKit checkers from checking the safety of code. Added: clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm Modified: clang/docs/analyzer/checkers.rst clang/include/clang/StaticAnalyzer/Checkers/Checkers.td clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt clang/test/Analysis/Checkers/WebKit/mock-system-header.h clang/test/Analysis/Checkers/WebKit/objc-mock-types.h Removed: ################################################################################ diff --git a/clang/docs/analyzer/checkers.rst b/clang/docs/analyzer/checkers.rst index 905c93678ffe1..28286a8a5dba6 100644 --- a/clang/docs/analyzer/checkers.rst +++ b/clang/docs/analyzer/checkers.rst @@ -3476,6 +3476,24 @@ Limitations: alpha.WebKit ^^^^^^^^^^^^ +alpha.webkit.ForwardDeclChecker +""""""""""""""""""""""""""""""" +Check for local variables, member variables, and function arguments that are forward declared. + +.. code-block:: cpp + + struct Obj; + Obj* provide(); + + struct Foo { + Obj* ptr; // warn + }; + + void foo() { + Obj* obj = provide(); // warn + consume(obj); // warn + } + .. _alpha-webkit-NoUncheckedPtrMemberChecker: alpha.webkit.MemoryUnsafeCastChecker diff --git a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td index deb36561839cd..6632254955fe6 100644 --- a/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td +++ b/clang/include/clang/StaticAnalyzer/Checkers/Checkers.td @@ -1761,6 +1761,10 @@ def UncountedLambdaCapturesChecker : Checker<"UncountedLambdaCapturesChecker">, let ParentPackage = WebKitAlpha in { +def ForwardDeclChecker : Checker<"ForwardDeclChecker">, + HelpText<"Check for forward declared local or member variables and function arguments">, + Documentation<HasDocumentation>; + def MemoryUnsafeCastChecker : Checker<"MemoryUnsafeCastChecker">, HelpText<"Check for memory unsafe casts from base type to derived type.">, Documentation<HasDocumentation>; diff --git a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt index a0b8549ade917..22dd3f0374849 100644 --- a/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt +++ b/clang/lib/StaticAnalyzer/Checkers/CMakeLists.txt @@ -130,6 +130,7 @@ add_clang_library(clangStaticAnalyzerCheckers ValistChecker.cpp VirtualCallChecker.cpp WebKit/ASTUtils.cpp + WebKit/ForwardDeclChecker.cpp WebKit/MemoryUnsafeCastChecker.cpp WebKit/PtrTypesSemantics.cpp WebKit/RefCntblBaseVirtualDtorChecker.cpp diff --git a/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp new file mode 100644 index 0000000000000..291eb140d3202 --- /dev/null +++ b/clang/lib/StaticAnalyzer/Checkers/WebKit/ForwardDeclChecker.cpp @@ -0,0 +1,327 @@ +//=======- ForwardDeclChecker.cpp --------------------------------*- 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 +// +//===----------------------------------------------------------------------===// + +#include "ASTUtils.h" +#include "DiagOutputUtils.h" +#include "PtrTypesSemantics.h" +#include "clang/AST/CXXInheritance.h" +#include "clang/AST/Decl.h" +#include "clang/AST/DeclCXX.h" +#include "clang/AST/RecursiveASTVisitor.h" +#include "clang/Analysis/DomainSpecific/CocoaConventions.h" +#include "clang/Basic/SourceLocation.h" +#include "clang/StaticAnalyzer/Checkers/BuiltinCheckerRegistration.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugReporter.h" +#include "clang/StaticAnalyzer/Core/BugReporter/BugType.h" +#include "clang/StaticAnalyzer/Core/Checker.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/Support/SaveAndRestore.h" +#include <optional> + +using namespace clang; +using namespace ento; + +namespace { + +class ForwardDeclChecker : public Checker<check::ASTDecl<TranslationUnitDecl>> { + BugType Bug; + mutable BugReporter *BR; + mutable RetainTypeChecker RTC; + mutable llvm::DenseSet<const Type *> SystemTypes; + +public: + ForwardDeclChecker() + : Bug(this, "Forward declared member or local variable or parameter", + "WebKit coding guidelines") {} + + void checkASTDecl(const TranslationUnitDecl *TUD, AnalysisManager &MGR, + BugReporter &BRArg) const { + BR = &BRArg; + + // The calls to checkAST* from AnalysisConsumer don't + // visit template instantiations or lambda classes. We + // want to visit those, so we make our own RecursiveASTVisitor. + struct LocalVisitor : public RecursiveASTVisitor<LocalVisitor> { + using Base = RecursiveASTVisitor<LocalVisitor>; + + const ForwardDeclChecker *Checker; + Decl *DeclWithIssue{nullptr}; + + explicit LocalVisitor(const ForwardDeclChecker *Checker) + : Checker(Checker) { + assert(Checker); + } + + bool shouldVisitTemplateInstantiations() const { return true; } + bool shouldVisitImplicitCode() const { return false; } + + bool VisitTypedefDecl(TypedefDecl *TD) { + Checker->visitTypedef(TD); + return true; + } + + bool VisitRecordDecl(const RecordDecl *RD) { + Checker->visitRecordDecl(RD, DeclWithIssue); + return true; + } + + bool TraverseDecl(Decl *D) { + llvm::SaveAndRestore SavedDecl(DeclWithIssue); + if (D && (isa<FunctionDecl>(D) || isa<ObjCMethodDecl>(D))) + DeclWithIssue = D; + return Base::TraverseDecl(D); + } + + bool VisitVarDecl(VarDecl *V) { + if (V->isLocalVarDecl()) + Checker->visitVarDecl(V, DeclWithIssue); + return true; + } + + bool VisitCallExpr(const CallExpr *CE) { + Checker->visitCallExpr(CE, DeclWithIssue); + return true; + } + + bool VisitCXXConstructExpr(const CXXConstructExpr *CE) { + Checker->visitConstructExpr(CE, DeclWithIssue); + return true; + } + + bool VisitObjCMessageExpr(const ObjCMessageExpr *ObjCMsgExpr) { + Checker->visitObjCMessageExpr(ObjCMsgExpr, DeclWithIssue); + return true; + } + }; + + LocalVisitor visitor(this); + RTC.visitTranslationUnitDecl(TUD); + visitor.TraverseDecl(const_cast<TranslationUnitDecl *>(TUD)); + } + + void visitTypedef(const TypedefDecl *TD) const { + RTC.visitTypedef(TD); + auto QT = TD->getUnderlyingType().getCanonicalType(); + if (BR->getSourceManager().isInSystemHeader(TD->getBeginLoc())) { + if (auto *Type = QT.getTypePtrOrNull(); Type && QT->isPointerType()) + SystemTypes.insert(Type); + } + } + + bool isUnknownType(QualType QT) const { + auto *Type = QT.getTypePtrOrNull(); + if (!Type) + return false; + auto *CanonicalType = QT.getCanonicalType().getTypePtrOrNull(); + auto PointeeQT = Type->getPointeeType(); + auto *PointeeType = PointeeQT.getTypePtrOrNull(); + if (!PointeeType) + return false; + auto *R = PointeeType->getAsCXXRecordDecl(); + if (!R) // Forward declaration of a Objective-C interface is safe. + return false; + auto Name = R->getName(); + return !R->hasDefinition() && !RTC.isUnretained(QT) && + !SystemTypes.contains(CanonicalType) && + !Name.starts_with("Opaque") && Name != "_NSZone"; + } + + void visitRecordDecl(const RecordDecl *RD, const Decl *DeclWithIssue) const { + if (!RD->isThisDeclarationADefinition()) + return; + + if (RD->isImplicit() || RD->isLambda()) + return; + + const auto RDLocation = RD->getLocation(); + if (!RDLocation.isValid()) + return; + + const auto Kind = RD->getTagKind(); + if (Kind != TagTypeKind::Struct && Kind != TagTypeKind::Class) + return; + + if (BR->getSourceManager().isInSystemHeader(RDLocation)) + return; + + // Ref-counted smartpointers actually have raw-pointer to uncounted type as + // a member but we trust them to handle it correctly. + auto R = llvm::dyn_cast_or_null<CXXRecordDecl>(RD); + if (!R || isRefCounted(R) || isCheckedPtr(R) || isRetainPtr(R)) + return; + + for (auto *Member : RD->fields()) { + auto QT = Member->getType(); + if (isUnknownType(QT)) { + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + const std::string TypeName = QT.getAsString(); + Os << "Member variable "; + printQuotedName(Os, Member); + Os << " uses a forward declared type '" << TypeName << "'"; + + const SourceLocation SrcLocToReport = Member->getBeginLoc(); + PathDiagnosticLocation BSLoc(SrcLocToReport, BR->getSourceManager()); + auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); + Report->addRange(Member->getSourceRange()); + Report->setDeclWithIssue(DeclWithIssue); + BR->emitReport(std::move(Report)); + } + } + } + + void visitVarDecl(const VarDecl *V, const Decl *DeclWithIssue) const { + if (BR->getSourceManager().isInSystemHeader(V->getBeginLoc())) + return; + + auto QT = V->getType(); + if (!isUnknownType(QT)) + return; + + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + Os << "Local variable "; + printQuotedQualifiedName(Os, V); + + reportBug(V->getBeginLoc(), V->getSourceRange(), DeclWithIssue, Os.str(), + QT); + } + + void visitCallExpr(const CallExpr *CE, const Decl *DeclWithIssue) const { + if (BR->getSourceManager().isInSystemHeader(CE->getExprLoc())) + return; + + if (auto *F = CE->getDirectCallee()) { + // Skip the first argument for overloaded member operators (e. g. lambda + // or std::function call operator). + unsigned ArgIdx = + isa<CXXOperatorCallExpr>(CE) && isa_and_nonnull<CXXMethodDecl>(F); + + for (auto P = F->param_begin(); + P < F->param_end() && ArgIdx < CE->getNumArgs(); ++P, ++ArgIdx) + visitCallArg(CE->getArg(ArgIdx), *P, DeclWithIssue); + } + } + + void visitConstructExpr(const CXXConstructExpr *CE, + const Decl *DeclWithIssue) const { + if (BR->getSourceManager().isInSystemHeader(CE->getExprLoc())) + return; + + if (auto *F = CE->getConstructor()) { + // Skip the first argument for overloaded member operators (e. g. lambda + // or std::function call operator). + unsigned ArgIdx = + isa<CXXOperatorCallExpr>(CE) && isa_and_nonnull<CXXMethodDecl>(F); + + for (auto P = F->param_begin(); + P < F->param_end() && ArgIdx < CE->getNumArgs(); ++P, ++ArgIdx) + visitCallArg(CE->getArg(ArgIdx), *P, DeclWithIssue); + } + } + + void visitObjCMessageExpr(const ObjCMessageExpr *E, + const Decl *DeclWithIssue) const { + if (BR->getSourceManager().isInSystemHeader(E->getExprLoc())) + return; + + if (auto *Receiver = E->getInstanceReceiver()->IgnoreParenCasts()) { + if (isUnknownType(E->getReceiverType())) + reportUnknownRecieverType(Receiver, DeclWithIssue); + } + + auto *MethodDecl = E->getMethodDecl(); + if (!MethodDecl) + return; + + auto ArgCount = E->getNumArgs(); + for (unsigned i = 0; i < ArgCount && i < MethodDecl->param_size(); ++i) + visitCallArg(E->getArg(i), MethodDecl->getParamDecl(i), DeclWithIssue); + } + + void visitCallArg(const Expr *Arg, const ParmVarDecl *Param, + const Decl *DeclWithIssue) const { + auto *ArgExpr = Arg->IgnoreParenCasts(); + if (auto *InnerCE = dyn_cast<CallExpr>(Arg)) { + auto *InnerCallee = InnerCE->getDirectCallee(); + if (InnerCallee && InnerCallee->isInStdNamespace() && + safeGetName(InnerCallee) == "move" && InnerCE->getNumArgs() == 1) { + ArgExpr = InnerCE->getArg(0); + if (ArgExpr) + ArgExpr = ArgExpr->IgnoreParenCasts(); + } + } + if (isa<CXXNullPtrLiteralExpr>(ArgExpr) || isa<IntegerLiteral>(ArgExpr) || + isa<CXXDefaultArgExpr>(ArgExpr)) + return; + if (auto *DRE = dyn_cast<DeclRefExpr>(ArgExpr)) { + if (auto *ValDecl = DRE->getDecl()) { + if (isa<ParmVarDecl>(ValDecl)) + return; + } + } + + QualType ArgType = Param->getType(); + if (!isUnknownType(ArgType)) + return; + + reportUnknownArgType(Arg, Param, DeclWithIssue); + } + + void reportUnknownArgType(const Expr *CA, const ParmVarDecl *Param, + const Decl *DeclWithIssue) const { + assert(CA); + + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + const std::string paramName = safeGetName(Param); + Os << "Call argument"; + if (!paramName.empty()) { + Os << " for parameter "; + printQuotedQualifiedName(Os, Param); + } + + reportBug(CA->getExprLoc(), CA->getSourceRange(), DeclWithIssue, Os.str(), + Param->getType()); + } + + void reportUnknownRecieverType(const Expr *Receiver, + const Decl *DeclWithIssue) const { + assert(Receiver); + reportBug(Receiver->getExprLoc(), Receiver->getSourceRange(), DeclWithIssue, + "Receiver", Receiver->getType()); + } + + void reportBug(const SourceLocation &SrcLoc, const SourceRange &SrcRange, + const Decl *DeclWithIssue, const StringRef &Description, + QualType Type) const { + SmallString<100> Buf; + llvm::raw_svector_ostream Os(Buf); + + const std::string TypeName = Type.getAsString(); + Os << Description << " uses a forward declared type '" << TypeName << "'"; + + PathDiagnosticLocation BSLoc(SrcLoc, BR->getSourceManager()); + auto Report = std::make_unique<BasicBugReport>(Bug, Os.str(), BSLoc); + Report->addRange(SrcRange); + Report->setDeclWithIssue(DeclWithIssue); + BR->emitReport(std::move(Report)); + } +}; + +} // namespace + +void ento::registerForwardDeclChecker(CheckerManager &Mgr) { + Mgr.registerChecker<ForwardDeclChecker>(); +} + +bool ento::shouldRegisterForwardDeclChecker(const CheckerManager &) { + return true; +} diff --git a/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm b/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm new file mode 100644 index 0000000000000..151cbe2affa92 --- /dev/null +++ b/clang/test/Analysis/Checkers/WebKit/forward-decl-checker.mm @@ -0,0 +1,136 @@ +// RUN: %clang_analyze_cc1 -analyzer-checker=alpha.webkit.ForwardDeclChecker -verify %s + +#include "mock-types.h" +#include "objc-mock-types.h" +#include "mock-system-header.h" + +namespace std { + +template <typename T> struct remove_reference { + typedef T type; +}; + +template <typename T> struct remove_reference<T&> { + typedef T type; +}; + +template<typename T> typename remove_reference<T>::type&& move(T&& t); + +} // namespace std + +typedef struct OpaqueJSString * JSStringRef; + +class Obj; +@class ObjCObj; + +Obj* provide_obj_ptr(); +void receive_obj_ptr(Obj* p = nullptr); + +Obj* ptr(Obj* arg) { + receive_obj_ptr(provide_obj_ptr()); + // expected-warning@-1{{Call argument for parameter 'p' uses a forward declared type 'Obj *'}} + auto *obj = provide_obj_ptr(); + // expected-warning@-1{{Local variable 'obj' uses a forward declared type 'Obj *'}} + receive_obj_ptr(arg); + receive_obj_ptr(nullptr); + receive_obj_ptr(); + return obj; +} + +Obj& provide_obj_ref(); +void receive_obj_ref(Obj& p); + +Obj& ref() { + receive_obj_ref(provide_obj_ref()); + // expected-warning@-1{{Call argument for parameter 'p' uses a forward declared type 'Obj &'}} + auto &obj = provide_obj_ref(); + // expected-warning@-1{{Local variable 'obj' uses a forward declared type 'Obj &'}} + return obj; +} + +Obj&& provide_obj_rval(); +void receive_obj_rval(Obj&& p); + +void rval(Obj&& arg) { + receive_obj_rval(provide_obj_rval()); + // expected-warning@-1{{Call argument for parameter 'p' uses a forward declared type 'Obj &&'}} + auto &&obj = provide_obj_rval(); + // expected-warning@-1{{Local variable 'obj' uses a forward declared type 'Obj &&'}} + receive_obj_rval(std::move(arg)); +} + +ObjCObj *provide_objcobj(); +void receive_objcobj(ObjCObj *p); +ObjCObj *objc_ptr() { + receive_objcobj(provide_objcobj()); + auto *objcobj = provide_objcobj(); + return objcobj; +} + +struct WrapperObj { + Obj* ptr { nullptr }; + // expected-warning@-1{{Member variable 'ptr' uses a forward declared type 'Obj *'}} + + WrapperObj(Obj* obj); + WrapperObj(Obj& obj); + WrapperObj(Obj&& obj); +}; + +void construct_ptr(Obj&& arg) { + WrapperObj wrapper1(provide_obj_ptr()); + // expected-warning@-1{{Call argument for parameter 'obj' uses a forward declared type 'Obj *'}} + WrapperObj wrapper2(provide_obj_ref()); + // expected-warning@-1{{Call argument for parameter 'obj' uses a forward declared type 'Obj &'}} + WrapperObj wrapper3(std::move(arg)); +} + +JSStringRef provide_opaque_ptr(); +void receive_opaque_ptr(JSStringRef); +NSZone *provide_zone(); + +JSStringRef opaque_ptr() { + receive_opaque_ptr(provide_opaque_ptr()); + auto ref = provide_opaque_ptr(); + return ref; +} + +@interface AnotherObj : NSObject +- (Obj *)ptr; +- (Obj &)ref; +- (void)objc; +- (void)doMoreWork:(ObjCObj *)obj; +@end + +@implementation AnotherObj +- (Obj *)ptr { + receive_obj_ptr(provide_obj_ptr()); + // expected-warning@-1{{Call argument for parameter 'p' uses a forward declared type 'Obj *'}} + auto *obj = provide_obj_ptr(); + // expected-warning@-1{{Local variable 'obj' uses a forward declared type 'Obj *'}} + return obj; +} + +- (Obj &)ref { + receive_obj_ref(provide_obj_ref()); + // expected-warning@-1{{Call argument for parameter 'p' uses a forward declared type 'Obj &'}} + auto &obj = provide_obj_ref(); + // expected-warning@-1{{Local variable 'obj' uses a forward declared type 'Obj &'}} + return obj; +} + +- (void)objc { + auto *obj = provide_objcobj(); + [obj doWork]; + [self doMoreWork:provide_objcobj()]; + [self doMoreWork:nil]; +} + +- (void)doMoreWork:(ObjCObj *)obj { + auto array = CFArrayCreateMutable(kCFAllocatorDefault, 10); + CFArrayAppendValue(array, nullptr); + auto log = os_log_create("Foo", "Bar"); + os_log_msg(log, OS_LOG_TYPE_DEFAULT, "Some Log"); + auto *zone = provide_zone(); +} + +@end diff --git a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h index a1d30957b19cb..73d6e3dbf4643 100644 --- a/clang/test/Analysis/Checkers/WebKit/mock-system-header.h +++ b/clang/test/Analysis/Checkers/WebKit/mock-system-header.h @@ -15,3 +15,17 @@ template <typename T> struct MemberVariable { T* obj { nullptr }; }; + +typedef unsigned char uint8_t; + +enum os_log_type_t : uint8_t { + OS_LOG_TYPE_DEFAULT = 0x00, + OS_LOG_TYPE_INFO = 0x01, + OS_LOG_TYPE_DEBUG = 0x02, + OS_LOG_TYPE_ERROR = 0x10, + OS_LOG_TYPE_FAULT = 0x11, +}; + +typedef struct os_log_s *os_log_t; +os_log_t os_log_create(const char *subsystem, const char *category); +void os_log_msg(os_log_t oslog, os_log_type_t type, const char *msg); diff --git a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h index c3925218c6c99..5bd265596a0b4 100644 --- a/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h +++ b/clang/test/Analysis/Checkers/WebKit/objc-mock-types.h @@ -14,9 +14,11 @@ typedef const struct CF_BRIDGED_TYPE(NSString) __CFString * CFStringRef; typedef const struct CF_BRIDGED_TYPE(NSArray) __CFArray * CFArrayRef; typedef struct CF_BRIDGED_MUTABLE_TYPE(NSMutableArray) __CFArray * CFMutableArrayRef; typedef struct CF_BRIDGED_MUTABLE_TYPE(CFRunLoopRef) __CFRunLoop * CFRunLoopRef; + #define NS_RETURNS_RETAINED __attribute__((ns_returns_retained)) #define CF_CONSUMED __attribute__((cf_consumed)) #define CF_RETURNS_RETAINED __attribute__((cf_returns_retained)) + extern const CFAllocatorRef kCFAllocatorDefault; typedef struct _NSZone NSZone; CFMutableArrayRef CFArrayCreateMutable(CFAllocatorRef allocator, CFIndex capacity); @@ -278,4 +280,4 @@ using WTF::adoptNS; using WTF::adoptCF; using WTF::retainPtr; using WTF::downcast; -using WTF::bridge_cast; \ No newline at end of file +using WTF::bridge_cast; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
