Author: Ziqing Luo Date: 2026-03-25T17:54:47-07:00 New Revision: daf7fe214778fe36e5659c9adaca7ec43c31ba42
URL: https://github.com/llvm/llvm-project/commit/daf7fe214778fe36e5659c9adaca7ec43c31ba42 DIFF: https://github.com/llvm/llvm-project/commit/daf7fe214778fe36e5659c9adaca7ec43c31ba42.diff LOG: [ssaf][UnsafeBufferUsage] Add support for extracting unsafe pointers from all kinds of contributors (#184899) - Generalize the `-Wunsafe-buffer-usage` API for finding unsafe pointers in all kinds of `Decl`s - Add support in SSAF-based UnsafeBufferUsage analysis for extracting from various contributors - Mock implementation of HandleTranslationUnit rdar://171735836 This is a follow-up PR to https://github.com/llvm/llvm-project/pull/182941 Added: Modified: clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h clang/include/clang/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsage.h clang/lib/Analysis/UnsafeBufferUsage.cpp clang/lib/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageExtractor.cpp clang/unittests/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageTest.cpp Removed: ################################################################################ diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 876682ad779d4..e0d583c735e61 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -201,7 +201,14 @@ bool anyConflict(const llvm::SmallVectorImpl<FixItHint> &FixIts, const SourceManager &SM); } // namespace internal -std::set<const Expr *> findUnsafePointers(const FunctionDecl *FD); +/// Find unsafe pointers in body/initializer of `D`, if `D` is one of the +/// followings: +/// VarDecl +/// FieldDecl +/// FunctionDecl +/// BlockDecl +/// ObjCMethodDecl +std::set<const Expr *> findUnsafePointers(const Decl *D); } // end namespace clang #endif /* LLVM_CLANG_ANALYSIS_ANALYSES_UNSAFEBUFFERUSAGE_H */ diff --git a/clang/include/clang/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsage.h b/clang/include/clang/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsage.h index 5f418000723b4..c0a4c2f76ab48 100644 --- a/clang/include/clang/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsage.h +++ b/clang/include/clang/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsage.h @@ -98,6 +98,8 @@ class UnsafeBufferUsageEntitySummary final : public EntitySummary { bool operator==(const EntityPointerLevelSet &Other) const { return UnsafeBuffers == Other; } + + bool empty() const { return UnsafeBuffers.empty(); } }; } // namespace clang::ssaf diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 133e39b8fac2b..5a9241acbee36 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -28,6 +28,7 @@ #include "clang/Lex/Preprocessor.h" #include "llvm/ADT/APInt.h" #include "llvm/ADT/APSInt.h" +#include "llvm/ADT/STLExtras.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/StringRef.h" @@ -36,6 +37,7 @@ #include <queue> #include <set> #include <sstream> +#include <vector> using namespace clang; @@ -2942,7 +2944,37 @@ template <typename NodeTy> struct CompareNode { } }; -std::set<const Expr *> clang::findUnsafePointers(const FunctionDecl *FD) { +// Populate `Stmts` with the body/initializer Stmt of `D`, if `D` is one of the +// followings: +// VarDecl +// FieldDecl +// FunctionDecl +// BlockDecl +// ObjCMethodDecl +static void populateStmtsForFindingGadgets(SmallVector<const Stmt *> &Stmts, + const Decl *D) { + auto AddStmt = [&Stmts](const Stmt *S) { + if (S) + Stmts.push_back(S); + }; + if (const auto *FD = dyn_cast<FunctionDecl>(D)) { + AddStmt(FD->getBody()); + for (const auto *PD : FD->parameters()) + AddStmt(PD->getDefaultArg()); + if (const auto *CtorD = dyn_cast<CXXConstructorDecl>(FD)) + llvm::append_range( + Stmts, llvm::map_range(CtorD->inits(), + std::mem_fn(&CXXCtorInitializer::getInit))); + } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) { + AddStmt(D->getBody()); + } else if (const auto *VD = dyn_cast<VarDecl>(D)) { + AddStmt(VD->getInit()); // FIXME: default arg for ParmVarDecl? + } else if (const auto *FD = dyn_cast<FieldDecl>(D)) { + AddStmt(FD->getInClassInitializer()); + } +} + +std::set<const Expr *> clang::findUnsafePointers(const Decl *D) { class MockReporter : public UnsafeBufferUsageHandler { public: MockReporter() {} @@ -2981,9 +3013,13 @@ std::set<const Expr *> clang::findUnsafePointers(const FunctionDecl *FD) { WarningGadgetList WarningGadgets; DeclUseTracker Tracker; MockReporter IgnoreHandler; + ASTContext &Ctx = D->getASTContext(); + SmallVector<const Stmt *> Stmts; - findGadgets(FD->getBody(), FD->getASTContext(), IgnoreHandler, false, - FixableGadgets, WarningGadgets, Tracker); + populateStmtsForFindingGadgets(Stmts, D); + for (auto *Stmt : Stmts) + findGadgets(Stmt, Ctx, IgnoreHandler, false, FixableGadgets, WarningGadgets, + Tracker); std::set<const Expr *> Result; for (auto &G : WarningGadgets) { @@ -4673,9 +4709,6 @@ void clang::checkUnsafeBufferUsage(const Decl *D, #endif assert(D); - - SmallVector<Stmt *> Stmts; - if (const auto *FD = dyn_cast<FunctionDecl>(D)) { // Consteval functions are free of UB by the spec, so we don't need to // visit them or produce diagnostics. @@ -4697,24 +4730,18 @@ void clang::checkUnsafeBufferUsage(const Decl *D, break; } } + } - Stmts.push_back(FD->getBody()); + SmallVector<const Stmt *> Stmts; - if (const auto *ID = dyn_cast<CXXConstructorDecl>(D)) { - for (const CXXCtorInitializer *CI : ID->inits()) { - Stmts.push_back(CI->getInit()); - } - } - } else if (isa<BlockDecl>(D) || isa<ObjCMethodDecl>(D)) { - Stmts.push_back(D->getBody()); - } + populateStmtsForFindingGadgets(Stmts, D); assert(!Stmts.empty()); FixableGadgetList FixableGadgets; WarningGadgetList WarningGadgets; DeclUseTracker Tracker; - for (Stmt *S : Stmts) { + for (const Stmt *S : Stmts) { findGadgets(S, D->getASTContext(), Handler, EmitSuggestions, FixableGadgets, WarningGadgets, Tracker); } diff --git a/clang/lib/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageExtractor.cpp b/clang/lib/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageExtractor.cpp index c150b2918db59..c609168e4dc7d 100644 --- a/clang/lib/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageExtractor.cpp +++ b/clang/lib/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageExtractor.cpp @@ -33,6 +33,32 @@ static bool hasPointerType(const Expr *E) { constexpr inline auto buildEntityPointerLevel = UnsafeBufferUsageTUSummaryExtractor::buildEntityPointerLevel; +static llvm::Error makeUnsupportedStmtKindError(const Stmt *Unsupported) { + return llvm::createStringError( + "unsupported expression kind for translation to " + "EntityPointerLevel: %s", + Unsupported->getStmtClassName()); +} + +static llvm::Error makeCreateEntityNameError(const NamedDecl *FailedDecl, + ASTContext &Ctx) { + std::string LocStr = FailedDecl->getSourceRange().getBegin().printToString( + Ctx.getSourceManager()); + return llvm::createStringError( + "failed to create entity name for %s declared at %s", + FailedDecl->getNameAsString().c_str(), LocStr.c_str()); +} + +static llvm::Error makeAddEntitySummaryError(const NamedDecl *FailedContributor, + ASTContext &Ctx) { + std::string LocStr = + FailedContributor->getSourceRange().getBegin().printToString( + Ctx.getSourceManager()); + return llvm::createStringError( + "failed to add entity summary for contributor %s declared at %s", + FailedContributor->getNameAsString().c_str(), LocStr.c_str()); +} + // Translate a pointer type expression 'E' to a (set of) EntityPointerLevel(s) // associated with the declared type of the base address of `E`. If the base // address of `E` is not associated with an entity, the translation result is an @@ -59,10 +85,7 @@ class EntityPointerLevelTranslator // Fallback method for all unsupported expression kind: llvm::Error fallback(const Stmt *E) { - return llvm::createStringError( - "unsupported expression kind for translation to " - "EntityPointerLevel: %s", - E->getStmtClassName()); + return makeUnsupportedStmtKindError(E); } static EntityPointerLevel incrementPointerLevel(const EntityPointerLevel &E) { @@ -92,10 +115,12 @@ class EntityPointerLevelTranslator } UnsafeBufferUsageTUSummaryExtractor &Extractor; + ASTContext &Ctx; public: - EntityPointerLevelTranslator(UnsafeBufferUsageTUSummaryExtractor &Extractor) - : Extractor(Extractor) {} + EntityPointerLevelTranslator(UnsafeBufferUsageTUSummaryExtractor &Extractor, + ASTContext &Ctx) + : Extractor(Extractor), Ctx(Ctx) {} Expected<EntityPointerLevelSet> translate(const Expr *E) { return Visit(E); } @@ -210,18 +235,14 @@ class EntityPointerLevelTranslator Expected<EntityPointerLevelSet> VisitDeclRefExpr(const DeclRefExpr *E) { if (auto EntityName = getEntityName(E->getDecl())) return EntityPointerLevelSet{createEntityPointerLevelFor(*EntityName)}; - return llvm::createStringError( - "failed to create entity name from the Decl of " + - E->getNameInfo().getAsString()); + return makeCreateEntityNameError(E->getDecl(), Ctx); } // Translate({., ->}f) -> {(MemberDecl, 1)} Expected<EntityPointerLevelSet> VisitMemberExpr(const MemberExpr *E) { if (auto EntityName = getEntityName(E->getMemberDecl())) return EntityPointerLevelSet{createEntityPointerLevelFor(*EntityName)}; - return llvm::createStringError( - "failed to create entity name from the MemberDecl of " + - E->getMemberDecl()->getNameAsString()); + return makeCreateEntityNameError(E->getMemberDecl(), Ctx); } Expected<EntityPointerLevelSet> @@ -232,9 +253,10 @@ class EntityPointerLevelTranslator Expected<EntityPointerLevelSet> buildEntityPointerLevels(std::set<const Expr *> &&UnsafePointers, - UnsafeBufferUsageTUSummaryExtractor &Extractor) { + UnsafeBufferUsageTUSummaryExtractor &Extractor, + ASTContext &Ctx) { EntityPointerLevelSet Result{}; - EntityPointerLevelTranslator Translator{Extractor}; + EntityPointerLevelTranslator Translator{Extractor, Ctx}; llvm::Error AllErrors = llvm::ErrorSuccess(); for (const Expr *Ptr : UnsafePointers) { @@ -258,24 +280,90 @@ buildEntityPointerLevels(std::set<const Expr *> &&UnsafePointers, } } // namespace +static std::set<const Expr *> findUnsafePointersInContributor(const Decl *D) { + if (isa<FunctionDecl>(D) || isa<VarDecl>(D)) + return findUnsafePointers(D); + if (auto *RD = dyn_cast<RecordDecl>(D)) { + std::set<const Expr *> Result; + + for (const FieldDecl *FD : RD->fields()) { + Result.merge(findUnsafePointers(FD)); + } + return Result; + } + return {}; +} + std::unique_ptr<UnsafeBufferUsageEntitySummary> UnsafeBufferUsageTUSummaryExtractor::extractEntitySummary( const Decl *Contributor, ASTContext &Ctx, llvm::Error &Error) { - // FIXME: findUnsafePointers should accept more kinds of `Decl`s than just - // `FunctionDecl`: - if (const auto *FD = dyn_cast<FunctionDecl>(Contributor)) { - Expected<EntityPointerLevelSet> EPLs = - buildEntityPointerLevels(findUnsafePointers(FD), *this); - - if (EPLs) - return std::make_unique<UnsafeBufferUsageEntitySummary>( - UnsafeBufferUsageEntitySummary(std::move(*EPLs))); - Error = EPLs.takeError(); - } + Expected<EntityPointerLevelSet> EPLs = buildEntityPointerLevels( + findUnsafePointersInContributor(Contributor), *this, Ctx); + + if (EPLs) + return std::make_unique<UnsafeBufferUsageEntitySummary>( + UnsafeBufferUsageEntitySummary(std::move(*EPLs))); + Error = EPLs.takeError(); return nullptr; } void UnsafeBufferUsageTUSummaryExtractor::HandleTranslationUnit( ASTContext &Ctx) { - // FIXME: impl me! + + // FIXME: I suppose finding contributor Decls is commonly needed by all/many + // extractors + class ContributorFinder : public DynamicRecursiveASTVisitor { + public: + std::vector<const NamedDecl *> Contributors; + + bool VisitFunctionDecl(FunctionDecl *D) override { + Contributors.push_back(D); + return true; + } + + bool VisitRecordDecl(RecordDecl *D) override { + Contributors.push_back(D); + return true; + } + + bool VisitVarDecl(VarDecl *D) override { + DeclContext *DC = D->getDeclContext(); + + if (DC->isFileContext() || DC->isNamespace()) + Contributors.push_back(D); + return false; + } + } ContributorFinder; + + ContributorFinder.VisitTranslationUnitDecl(Ctx.getTranslationUnitDecl()); + + llvm::Error Errors = llvm::ErrorSuccess(); + auto addError = [&Errors](llvm::Error Err) { + Errors = llvm::joinErrors(std::move(Errors), std::move(Err)); + }; + + for (auto *CD : ContributorFinder.Contributors) { + llvm::Error Error = llvm::ErrorSuccess(); + auto EntitySummary = extractEntitySummary(CD, Ctx, Error); + + if (Error) + addError(std::move(Error)); + if (EntitySummary->empty()) + continue; + + auto ContributorName = getEntityName(CD); + + if (!ContributorName) { + addError(makeCreateEntityNameError(CD, Ctx)); + continue; + } + + auto [EntitySummaryPtr, Success] = SummaryBuilder.addSummary( + addEntity(*ContributorName), std::move(EntitySummary)); + + if (!Success) + addError(makeAddEntitySummaryError(CD, Ctx)); + } + // FIXME: handle errors! + llvm::consumeError(std::move(Errors)); } diff --git a/clang/unittests/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageTest.cpp b/clang/unittests/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageTest.cpp index d10dac2b7a9d3..3555f5196a7b9 100644 --- a/clang/unittests/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageTest.cpp +++ b/clang/unittests/ScalableStaticAnalysisFramework/Analyses/UnsafeBufferUsage/UnsafeBufferUsageTest.cpp @@ -35,7 +35,7 @@ const SomeDecl *findDeclByName(StringRef Name, ASTContext &Ctx) { NamedDeclFinder(StringRef SearchingName) : SearchingName(SearchingName) {} bool VisitDecl(Decl *D) override { - if (const auto *ND = dyn_cast<NamedDecl>(D)) { + if (const auto *ND = dyn_cast<SomeDecl>(D)) { if (ND->getNameAsString() == SearchingName) { FoundDecl = ND; return false; @@ -69,16 +69,21 @@ class UnsafeBufferUsageTest : public testing::Test { : TUSum(BuildNamespace(BuildNamespaceKind::CompilationUnit, "Mock.cpp")), Builder(TUSum), Extractor(Builder) {} + template <typename ContributorDecl = NamedDecl> std::unique_ptr<UnsafeBufferUsageEntitySummary> setUpTest(StringRef Code, StringRef ContributorName) { AST = tooling::buildASTFromCodeWithArgs( - Code, {"-Wno-unused-value -Wno-int-to-pointer-cast"}); + Code, {"-Wno-unused-value", "-Wno-int-to-pointer-cast"}); const auto *ContributorDefn = - findDeclByName(ContributorName, AST->getASTContext()); + findDeclByName<ContributorDecl>(ContributorName, AST->getASTContext()); + + if (!ContributorDefn) + return nullptr; + std::optional<EntityName> EN = getEntityName(ContributorDefn); - if (!ContributorDefn || !EN) + if (!EN) return nullptr; llvm::Error Error = llvm::ErrorSuccess(); @@ -442,4 +447,133 @@ TEST_F(UnsafeBufferUsageTest, PointerToArrayOfPointers) { EXPECT_NE(Sum, nullptr); EXPECT_EQ(*Sum, makeSet(__LINE__, {{"p", 3}})); } + +TEST_F(UnsafeBufferUsageTest, UnsafePointerInGlobalVariableInitializer) { + auto Sum = setUpTest(R"cpp( + int *gp; + int x = gp[5]; + )cpp", + {"x"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"gp", 1U}})); +} + +TEST_F(UnsafeBufferUsageTest, UnsafePointerInFieldInitializer) { + auto Sum = setUpTest(R"cpp( + int *gp; + struct Foo { + int field = gp[5]; + }; + )cpp", + {"Foo"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"gp", 1U}})); +} + +TEST_F(UnsafeBufferUsageTest, UnsafePointerInFieldInitializer2) { + auto Sum = setUpTest(R"cpp( + int *gp; + union Foo { + int field = gp[5]; + int x; + }; + )cpp", + {"Foo"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"gp", 1U}})); +} + +TEST_F(UnsafeBufferUsageTest, InitializerList) { + auto Sum = setUpTest(R"cpp( + int *gp; + struct Foo { + int field; + int x; + }; + Foo FooObj{gp[5], 0}; + )cpp", + {"FooObj"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"gp", 1U}})); +} + +TEST_F(UnsafeBufferUsageTest, UnsafePointerInCXXCtorInitializer) { + auto Sum = setUpTest<CXXConstructorDecl>(R"cpp( + struct Foo { + int member; + Foo(int *p) : member(p[5]) {} + }; + )cpp", + {"Foo"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"p", 1U}})); +} + +TEST_F(UnsafeBufferUsageTest, UnsafePointerInDefaultArg) { + auto Sum = setUpTest(R"cpp( + int * gp; + void foo(int x = gp[5]); + )cpp", + {"foo"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"gp", 1U}})); +} + +TEST_F(UnsafeBufferUsageTest, NestedDefinitions) { + StringRef Code = R"cpp( + int * a = [](){ + struct Foo { + void bar(int * ptr) { ptr[3] = 0; } + }; + return nullptr; + }(); + )cpp"; + auto Sum = setUpTest(Code, {"bar"}); + + EXPECT_NE(Sum, nullptr); + // The closest contributor owns the fact: + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"ptr", 1U}})); + + Sum = setUpTest(Code, {"Foo"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_TRUE(Sum->empty()); + + Sum = setUpTest(Code, {"a"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_TRUE(Sum->empty()); +} + +TEST_F(UnsafeBufferUsageTest, NestedDefinitions2) { + StringRef Code = R"cpp( + int main(void) { + struct Foo { + void bar(int * ptr) { ptr[3] = 0; } + }; + } + )cpp"; + auto Sum = setUpTest(Code, {"bar"}); + + EXPECT_NE(Sum, nullptr); + // The closest contributor owns the fact: + EXPECT_EQ(*Sum, makeSet(__LINE__, {{"ptr", 1U}})); + + Sum = setUpTest(Code, {"Foo"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_TRUE(Sum->empty()); + + Sum = setUpTest(Code, {"main"}); + + EXPECT_NE(Sum, nullptr); + EXPECT_TRUE(Sum->empty()); +} + } // namespace _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
