From 5c2cda79df910a66df0dbe4e86da1e1502e91fa9 Mon Sep 17 00:00:00 2001 From: Patryk Stefanski Date: Mon, 29 Sep 2025 17:06:33 -0700 Subject: [PATCH] [-Wunsafe-buffer-usage] Check standalone count-attributed assignments This is an initial part of an analysis of count-attributed assignment groups. This commit adds an AST visitor that is responsible for finding bounds-attributed assignment groups and standalone assignments to bounds-attributed objects (pointers and dependent counts). As a PoC, this commit adds checks for standalone assignments, which are assignments that are not directly inside of a compound statement (like other assignment groups) and modify the pointer or count in some way. Our model rejects those and requires the user to simplify their code. For example: ``` void foo(int *__counted_by(count) p, int count) { q = p = ...; ^ this is rejected n = count = ...; ^ this is rejected // the following is fine: p = ...; count = ...; } ``` rdar://161607826 --- .../Analysis/Analyses/UnsafeBufferUsage.h | 8 +- .../clang/Basic/DiagnosticSemaKinds.td | 4 + clang/lib/Analysis/UnsafeBufferUsage.cpp | 313 +++++++++++++++++- clang/lib/Sema/AnalysisBasedWarnings.cpp | 14 +- ...ge-count-attributed-pointer-assignment.cpp | 57 ++++ 5 files changed, 393 insertions(+), 3 deletions(-) create mode 100644 clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-assignment.cpp diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 47753a7efdbbd..dc0ae2c6f6838 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -142,6 +142,11 @@ class UnsafeBufferUsageHandler { ASTContext &Ctx) { handleUnsafeOperation(Arg, IsRelatedToDecl, Ctx); } + + virtual void handleStandaloneAssign(const Expr *E, const ValueDecl *VD, + bool IsRelatedToDecl, ASTContext &Ctx) { + handleUnsafeOperation(E, IsRelatedToDecl, Ctx); + } /* TO_UPSTREAM(BoundsSafety) OFF */ /// Invoked when a fix is suggested against a variable. This function groups @@ -196,7 +201,8 @@ class UnsafeBufferUsageHandler { // This function invokes the analysis and allows the caller to react to it // through the handler class. void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler, - bool EmitSuggestions); + bool EmitSuggestions, + bool BoundsSafetyAttributes = false); namespace internal { // Tests if any two `FixItHint`s in `FixIts` conflict. Two `FixItHint`s diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index fd33296ed2185..e70cc46630c6b 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -14250,6 +14250,10 @@ def note_unsafe_count_attributed_pointer_argument_null_to_nonnull : Note< def warn_unsafe_single_pointer_argument : Warning< "unsafe assignment to function parameter of __single pointer type">, InGroup, DefaultIgnore; +def warn_standalone_assign_to_bounds_attributed_object : Warning< + "assignment to %select{count-attributed pointer|dependent count}0 '%1' " + "must be a simple statement '%1 = ...'">, + InGroup, DefaultIgnore; #ifndef NDEBUG // Not a user-facing diagnostic. Useful for debugging false negatives in // -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits). diff --git a/clang/lib/Analysis/UnsafeBufferUsage.cpp b/clang/lib/Analysis/UnsafeBufferUsage.cpp index 5c38255d2b839..162cc7969179f 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -5433,9 +5433,315 @@ static void applyGadgets(const Decl *D, FixableGadgetList FixableGadgets, } } +// Checks if Self and Other are the same member bases. This supports only very +// simple forms of member bases. +static bool isSameMemberBase(const Expr *Self, const Expr *Other) { + for (;;) { + if (Self == Other) + return true; + + const auto *SelfICE = dyn_cast(Self); + const auto *OtherICE = dyn_cast(Other); + if (SelfICE && OtherICE && SelfICE->getCastKind() == CK_LValueToRValue && + OtherICE->getCastKind() == CK_LValueToRValue) { + Self = SelfICE->getSubExpr(); + Other = OtherICE->getSubExpr(); + } + + const auto *SelfDRE = dyn_cast(Self); + const auto *OtherDRE = dyn_cast(Other); + if (SelfDRE && OtherDRE) + return SelfDRE->getDecl() == OtherDRE->getDecl(); + + const auto *SelfME = dyn_cast(Self); + const auto *OtherME = dyn_cast(Other); + if (!SelfME || !OtherME || + SelfME->getMemberDecl() != OtherME->getMemberDecl()) { + return false; + } + + Self = SelfME->getBase(); + Other = OtherME->getBase(); + } +} + +using DependentDeclSetTy = llvm::SmallPtrSet; + +// Gets the set/closure of bounds-attributed pointers and counts that belong to +// the same group. Consider the following example: +// int a, b, c; +// int *__counted_by(a + b) p; +// int *__counted_by(b - c) q; +// Passing any of the variables above as `InitVD`, the function should return +// the closure `{a, b, c, p, q}`. +static DependentDeclSetTy GetBoundsAttributedClosure(const ValueDecl *InitVD) { + DependentDeclSetTy Set; + + llvm::SmallVector WorkList; + WorkList.push_back(InitVD); + + while (!WorkList.empty()) { + const ValueDecl *CurVD = WorkList.pop_back_val(); + bool Inserted = Set.insert(CurVD).second; + if (!Inserted) + continue; + + // If CurVD is a dependent decl, add the pointers that depend on CurVD. + for (const auto *Attr : CurVD->specific_attrs()) { + for (const Decl *D : Attr->dependerDecls()) { + if (const auto *VD = dyn_cast(D)) + WorkList.push_back(VD); + } + } + + // If CurVD is a bounds-attributed pointer (or pointer to it), add its + // dependent decls. + QualType Ty = CurVD->getType(); + const auto *BAT = Ty->getAs(); + if (!BAT && Ty->isPointerType()) + BAT = Ty->getPointeeType()->getAs(); + if (BAT) { + for (const auto &DI : BAT->dependent_decls()) + WorkList.push_back(DI.getDecl()); + } + } + + return Set; +} + +// Bounds-attributed pointer or dependent count. +struct BoundsAttributedObject { + const ValueDecl *Decl = nullptr; + const Expr *MemberBase = nullptr; + int DerefLevel = 0; +}; + +static std::optional +getBoundsAttributedObject(const Expr *E) { + E = E->IgnoreParenCasts(); + + int DerefLevel = 0; + while (const auto *UO = dyn_cast(E)) { + if (UO->getOpcode() == UO_Deref) + DerefLevel++; + else if (UO->getOpcode() == UO_AddrOf) + DerefLevel--; + else + break; + E = UO->getSubExpr()->IgnoreParenCasts(); + } + assert(DerefLevel >= 0); + + const ValueDecl *Decl; + const Expr *MemberBase = nullptr; + + if (const auto *DRE = dyn_cast(E)) + Decl = DRE->getDecl(); + else if (const auto *ME = dyn_cast(E)) { + Decl = ME->getMemberDecl(); + MemberBase = ME->getBase(); + } else + return std::nullopt; + + QualType Ty = Decl->getType(); + bool IsBoundsAttributedPointer = + Ty->isBoundsAttributedType() || + (Ty->isPointerType() && Ty->getPointeeType()->isBoundsAttributedType()); + if (IsBoundsAttributedPointer || Decl->isDependentCount()) + return {{Decl, MemberBase, DerefLevel}}; + + return std::nullopt; +} + +struct BoundsAttributedAssignmentGroup { + DependentDeclSetTy DeclClosure; + llvm::SmallVector Assignments; + llvm::SmallVector AssignedObjects; + using DeclUseTy = std::pair; + const Expr *MemberBase = nullptr; + + void Init(const BoundsAttributedObject &Object) { + DeclClosure = GetBoundsAttributedClosure(Object.Decl); + MemberBase = Object.MemberBase; + } + + void Clear() { + DeclClosure.clear(); + Assignments.clear(); + AssignedObjects.clear(); + MemberBase = nullptr; + } + + bool Empty() const { + return DeclClosure.empty(); + } + + bool IsPartOfGroup(const BoundsAttributedObject &Object) const { + if (!DeclClosure.contains(Object.Decl)) + return false; + if (MemberBase) + return Object.MemberBase && + isSameMemberBase(MemberBase, Object.MemberBase); + return true; + } + + void AddAssignment(const BoundsAttributedObject &Object, + const BinaryOperator *BO) { + Assignments.push_back(BO); + AssignedObjects.push_back(Object); + } +}; + +// Visitor that is responsible for finding bounds-attributed assignment groups +// and standalone assignments to bounds-attributed objects. +// +// Bounds-attributed groups must be inside of a CompoundStmt: +// void foo(int *__counted_by(count) p, int count) { +// p = ...; +// count = ...; +// } +// Here, the group consists of both assignments to p and count. Note that the +// function body is a CompoundStmt. +// +// Standalone assignments are modifications of bounds-attributed objects that +// are not simple assignments directly in a CompoundStmt: +// void foo(int *__counted_by(count) p, int count) { +// q = p = ...; +// ^ standalone assignment to __counted_by() pointer +// n = count += ...; +// ^ standalone assignment to dependent count +// } +struct BoundsAttributedGroupFinder + : public ConstStmtVisitor { + using GroupHandlerTy = void(const BoundsAttributedAssignmentGroup &Group); + using AssignHandlerTy = void(const Expr *, const ValueDecl *); + std::function GroupHandler; + std::function BadStandaloneAssignHandler; + BoundsAttributedAssignmentGroup CurGroup; + + explicit BoundsAttributedGroupFinder( + std::function GroupHandler, + std::function BadStandaloneAssignHandler) + : GroupHandler(std::move(GroupHandler)), + BadStandaloneAssignHandler(std::move(BadStandaloneAssignHandler)) {} + + void VisitChildren(const Stmt *S) { + for (const Stmt *Child : S->children()) + Visit(Child); + } + + void VisitStmt(const Stmt *S) { VisitChildren(S); } + + void VisitCompoundStmt(const CompoundStmt *CS) { + for (const Stmt *Child : CS->children()) { + const Stmt *E = Child; + + // See through `ExprWithCleanups`. Clang will attach those nodes when C++ + // temporary object needs to be materialized. In our case, this can + // happen when we create a temporary span with `sp.first()`. Then, the + // structure is going to be: + // CompoundStmt + // `-ExprWithCleanups + // `-BinaryOperator ... + if (const auto *EWC = dyn_cast(E)) + E = EWC->getSubExpr(); + + const auto *BO = dyn_cast(E); + if (BO && BO->getOpcode() == BO_Assign) + HandleAssignInCompound(BO); + else { + FinishGroup(); + Visit(Child); + } + } + + FinishGroup(); + } + + void HandleAssignInCompound(const BinaryOperator *AssignOp) { + const auto ObjectOpt = getBoundsAttributedObject(AssignOp->getLHS()); + if (!ObjectOpt.has_value()) { + FinishGroup(); + VisitChildren(AssignOp); + return; + } + + if (!CurGroup.IsPartOfGroup(*ObjectOpt)) { + FinishGroup(); + CurGroup.Init(*ObjectOpt); + } + + CurGroup.AddAssignment(*ObjectOpt, AssignOp); + VisitChildren(AssignOp->getRHS()); + } + + void FinishGroup() { + if (CurGroup.Empty()) + return; + GroupHandler(CurGroup); + CurGroup.Clear(); + } + + // Handle statements that might modify bounds-attributed pointer/count, but + // are not directly in a CompoundStmt. + + void VisitBinaryOperator(const BinaryOperator *BO) { + VisitChildren(BO); + + if (BO->isAssignmentOp()) + CheckBadStandaloneAssign(BO, BO->getLHS()); + } + + void VisitUnaryOperator(const UnaryOperator *UO) { + VisitChildren(UO); + + if (UO->isIncrementDecrementOp()) + CheckBadStandaloneAssign(UO, UO->getSubExpr()); + } + + void CheckBadStandaloneAssign(const Expr *E, const Expr *Sub) { + const auto DA = getBoundsAttributedObject(Sub); + if (DA.has_value()) + BadStandaloneAssignHandler(E, DA->Decl); + } +}; + +static void +diagnoseStandaloneAssignToBoundsAttributed(const Expr *E, const ValueDecl *VD, + UnsafeBufferUsageHandler &Handler, + ASTContext &Ctx) { + // Don't diagnose pointer arithmetic, since -Wunsafe-buffer-usage already does + // it. + bool IsPtrArith = false; + if (E->getType()->isPointerType()) { + if (const auto *BO = dyn_cast(E)) + IsPtrArith = BO->isCompoundAssignmentOp(); + else if (const auto *UO = dyn_cast(E)) { + assert(UO->isIncrementDecrementOp()); + IsPtrArith = true; + } + } + if (!IsPtrArith) + Handler.handleStandaloneAssign(E, VD, /*IsRelatedToDecl=*/false, Ctx); +} + +static void checkBoundsSafetyAssignments(const Stmt *S, + UnsafeBufferUsageHandler &Handler, + ASTContext &Ctx) { + BoundsAttributedGroupFinder Finder( + [&](const BoundsAttributedAssignmentGroup &Group) { + // TODO: Check group constraints. + }, + [&](const Expr *E, const ValueDecl *VD) { + diagnoseStandaloneAssignToBoundsAttributed(E, VD, Handler, Ctx); + }); + Finder.Visit(S); +} + void clang::checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler, - bool EmitSuggestions) { + bool EmitSuggestions, + bool BoundsSafetyAttributes) { #ifndef NDEBUG Handler.clearDebugNotes(); #endif @@ -5481,6 +5787,11 @@ void clang::checkUnsafeBufferUsage(const Decl *D, for (Stmt *S : Stmts) { findGadgets(S, D->getASTContext(), Handler, EmitSuggestions, FixableGadgets, WarningGadgets, Tracker); + + // Run the bounds-safety assignment analysis if the attributes are enabled, + // otherwise don't waste cycles. + if (BoundsSafetyAttributes) + checkBoundsSafetyAssignments(S, Handler, D->getASTContext()); } applyGadgets(D, std::move(FixableGadgets), std::move(WarningGadgets), std::move(Tracker), Handler, EmitSuggestions); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index b1cc9e79d74dc..2c87b23e20de0 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2608,6 +2608,16 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { ASTContext &Ctx) override { S.Diag(Arg->getBeginLoc(), diag::warn_unsafe_single_pointer_argument); } + + void handleStandaloneAssign(const Expr *E, const ValueDecl *VD, + bool IsRelatedToDecl, ASTContext &Ctx) override { + SourceLocation Loc = E->getBeginLoc(); + if (const auto *BO = dyn_cast(E)) + Loc = BO->getOperatorLoc(); + + S.Diag(Loc, diag::warn_standalone_assign_to_bounds_attributed_object) + << VD->isDependentCount() << VD->getName(); + } /* TO_UPSTREAM(BoundsSafety) OFF */ void handleUnsafeVariableGroup(const VarDecl *Variable, @@ -3454,6 +3464,7 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings( bool UnsafeBufferUsageShouldSuggestSuggestions = UnsafeBufferUsageCanEmitSuggestions && !DiagOpts.ShowSafeBufferUsageSuggestions; + bool BoundsSafetyAttributes = S.getLangOpts().BoundsSafetyAttributes; UnsafeBufferUsageReporter R(S, UnsafeBufferUsageShouldSuggestSuggestions); // The Callback function that performs analyses: @@ -3471,7 +3482,8 @@ void clang::sema::AnalysisBasedWarnings::IssueWarnings( !Diags.isIgnored(diag::warn_unsafe_buffer_libc_call, Node->getBeginLoc())) { clang::checkUnsafeBufferUsage(Node, R, - UnsafeBufferUsageShouldEmitSuggestions); + UnsafeBufferUsageShouldEmitSuggestions, + BoundsSafetyAttributes); } // More analysis ... diff --git a/clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-assignment.cpp b/clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-assignment.cpp new file mode 100644 index 0000000000000..c2bcd7555ba6a --- /dev/null +++ b/clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-assignment.cpp @@ -0,0 +1,57 @@ +// RUN: %clang_cc1 -fsyntax-only -std=c++20 -Wno-all -Wunsafe-buffer-usage -fexperimental-bounds-safety-attributes -verify %s + +#include +#include + +namespace std { +template struct span { + T *data() const noexcept; + size_t size() const noexcept; + size_t size_bytes() const noexcept; + span first(size_t count) const noexcept; + span last(size_t count) const noexcept; + span subspan(size_t offset, size_t count) const noexcept; + const T &operator[](const size_t idx) const; +}; +} // namespace std + +struct cb { + int *__counted_by(count) p; + size_t count; +}; + +// Simple pointer and count + +void good_null(int *__counted_by(count) p, int count) { + p = nullptr; + count = 0; +} + +// Simple pointer and count in struct + +void good_struct_self(cb *c) { + c->p = c->p; + c->count = c->count; +} + +// Standalone assigns to bounds-attributed + +void standalone_assign_to_ptr(int *__counted_by(count) p, int count, int *q) { + q = p; + q = p = q; // expected-warning{{assignment to count-attributed pointer 'p' must be a simple statement 'p = ...'}} + q = q = p = q; // expected-warning{{assignment to count-attributed pointer 'p' must be a simple statement 'p = ...'}} + + p++; // expected-warning{{unsafe pointer arithmetic}} expected-note{{pass -fsafe-buffer-usage-suggestions to receive code hardening suggestions}} +} + +void standalone_assign_to_count(int *__counted_by(count) p, int count, int n) { + n = count; + n = count = n; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} + n = n = count = n; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} + + count++; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} + --count; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} + count += 42; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} + count -= 42; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} + n = n + count++; // expected-warning{{assignment to dependent count 'count' must be a simple statement 'count = ...'}} +}