From 7f56bc0661a099452ba3a6910d6fd3d5d724e1e3 Mon Sep 17 00:00:00 2001 From: Patryk Stefanski Date: Wed, 22 Oct 2025 16:47:44 -0700 Subject: [PATCH] [-Wunsafe-buffer-usage] Check count-attributed objects that are used and assigned in the same group Check if the bounds-attributed group has assignments to objects that are also used in the same group. In those cases, the correctness of the group might depend on the order of assignments. We conservatively disallow such assignments. In the example below, the bounds-check in `sp.first()` uses the value of `b` before the later update, which can lead to OOB if `b` was less than 42. ``` void foo(int *__counted_by(a + b) p, int a, int b, std::span sp) { p = sp.first(b + 42).data(); b = 42; // b is assigned and used a = b; } ``` rdar://161608319 (cherry picked from commit 7e4ad9ddf6332d8ec48cbe3ad2a4f5d74b3f0e17) --- .../Analysis/Analyses/UnsafeBufferUsage.h | 6 ++ .../clang/Basic/DiagnosticSemaKinds.td | 3 + clang/lib/Analysis/UnsafeBufferUsage.cpp | 77 ++++++++++++++++++- clang/lib/Sema/AnalysisBasedWarnings.cpp | 10 +++ ...ge-count-attributed-pointer-assignment.cpp | 18 +++++ 5 files changed, 113 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index e517fe3e39a86..a087570ff9b90 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -180,6 +180,12 @@ class UnsafeBufferUsageHandler { ASTContext &Ctx) { handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx); } + + virtual void handleAssignedAndUsed(const BinaryOperator *Assign, + const Expr *Use, const ValueDecl *VD, + bool IsRelatedToDecl, ASTContext &Ctx) { + handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx); + } /* TO_UPSTREAM(BoundsSafety) OFF */ /// Invoked when a fix is suggested against a variable. This function groups diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 83f12f5b42af8..054596ca57959 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -14127,6 +14127,9 @@ def warn_missing_assignments_in_bounds_attributed_group : Warning< def warn_duplicated_assignment_in_bounds_attributed_group : Warning< "duplicated assignment to %select{variable|parameter|member}0 %1 in " "bounds-attributed group">, InGroup, DefaultIgnore; +def warn_assigned_and_used_in_bounds_attributed_group : Warning< + "%select{variable|parameter|member}0 %1 is assigned and used in the same " + "bounds-attributed group">, 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 592492dc2fd4b..9c95f6e4fd7e4 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -5456,6 +5456,16 @@ struct BoundsAttributedObject { const ValueDecl *Decl = nullptr; const Expr *MemberBase = nullptr; int DerefLevel = 0; + + bool operator==(const BoundsAttributedObject &Other) const { + if (Other.Decl != Decl || Other.DerefLevel != DerefLevel) + return false; + if (Other.MemberBase == MemberBase) + return true; + if (Other.MemberBase == nullptr || MemberBase == nullptr) + return false; + return isSameMemberBase(Other.MemberBase, MemberBase); + } }; static std::optional @@ -5499,7 +5509,7 @@ struct BoundsAttributedAssignmentGroup { DependentDeclSetTy DeclClosure; llvm::SmallVector Assignments; llvm::SmallVector AssignedObjects; - using DeclUseTy = std::pair; + llvm::SmallDenseMap Uses; const Expr *MemberBase = nullptr; void init(const BoundsAttributedObject &Object) { @@ -5511,6 +5521,7 @@ struct BoundsAttributedAssignmentGroup { DeclClosure.clear(); Assignments.clear(); AssignedObjects.clear(); + Uses.clear(); MemberBase = nullptr; } @@ -5652,6 +5663,22 @@ struct BoundsAttributedGroupFinder if (DA.has_value()) TooComplexAssignHandler(E, DA->Decl); } + + // Collect uses of decls belonging to the current group by visiting all DREs + // and MemberExprs. + + void addUseIfPartOfCurGroup(const Expr *E) { + const auto DA = getBoundsAttributedObject(E); + if (DA.has_value() && CurGroup.isPartOfGroup(*DA)) + CurGroup.Uses.insert({DA->Decl, E}); + } + + void VisitDeclRefExpr(const DeclRefExpr *DRE) { addUseIfPartOfCurGroup(DRE); } + + void VisitMemberExpr(const MemberExpr *ME) { + addUseIfPartOfCurGroup(ME); + Visit(ME->getBase()); + } }; // Checks if the bounds-attributed group does not assign to implicitly @@ -5783,6 +5810,52 @@ static bool checkMissingAndDuplicatedAssignments( return IsGroupSafe; } +// Checks if the bounds-attributed group has assignments to objects that are +// also used in the same group. In those cases, the correctness of the group +// might depend on the order of assignments. We conservatively disallow such +// assignments. +// +// In the example below, the bounds-check in `sp.first()` uses the value of `b` +// before the later update, which can lead to OOB if `b` was less than 42. +// void foo(int *__counted_by(a + b) p, int a, int b, std::span sp) { +// p = sp.first(b + 42).data(); +// b = 42; // b is assigned and used +// a = b; +// } +static bool checkAssignedAndUsed(const BoundsAttributedAssignmentGroup &Group, + UnsafeBufferUsageHandler &Handler, + ASTContext &Ctx) { + const auto &Uses = Group.Uses; + if (Uses.empty()) + return true; + + bool IsGroupSafe = true; + + for (size_t I = 0, N = Group.AssignedObjects.size(); I < N; ++I) { + const BoundsAttributedObject &LHSObj = Group.AssignedObjects[I]; + const BinaryOperator *Assign = Group.Assignments[I]; + + // Ignore self assignments, because they don't matter, since the value stays + // the same. + const auto RHSObj = getBoundsAttributedObject(Assign->getRHS()); + bool IsSelfAssign = RHSObj.has_value() && *RHSObj == LHSObj; + if (IsSelfAssign) + continue; + + const ValueDecl *VD = LHSObj.Decl; + const auto It = Uses.find(VD); + if (It == Uses.end()) + continue; + + const Expr *Use = It->second; + Handler.handleAssignedAndUsed(Assign, Use, VD, + /*IsRelatedToDecl=*/false, Ctx); + IsGroupSafe = false; + } + + return IsGroupSafe; +} + // Checks if the bounds-attributed group is safe. This function returns false // iff the assignment group is unsafe and diagnostics have been emitted. static bool @@ -5792,6 +5865,8 @@ checkBoundsAttributedGroup(const BoundsAttributedAssignmentGroup &Group, return false; if (!checkMissingAndDuplicatedAssignments(Group, Handler, Ctx)) return false; + if (!checkAssignedAndUsed(Group, Handler, Ctx)) + return false; // TODO: Add more checks. return true; } diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 306743186b617..0ca22cb84e738 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2675,6 +2675,16 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { S.Diag(PrevAssign->getOperatorLoc(), diag::note_bounds_safety_previous_assignment); } + + void handleAssignedAndUsed(const BinaryOperator *Assign, const Expr *Use, + const ValueDecl *VD, + [[maybe_unused]] bool IsRelatedToDecl, + [[maybe_unused]] ASTContext &Ctx) override { + S.Diag(Assign->getOperatorLoc(), + diag::warn_assigned_and_used_in_bounds_attributed_group) + << getBoundsAttributedObjectKind(VD) << VD; + S.Diag(Use->getBeginLoc(), diag::note_used_here); + } /* TO_UPSTREAM(BoundsSafety) OFF */ void handleUnsafeVariableGroup(const VarDecl *Variable, 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 index 226ec6e29a090..69125cdb75e55 100644 --- 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 @@ -339,6 +339,24 @@ void duplicated_complex(int *__counted_by(a + b) p, q = nullptr; // expected-warning{{duplicated assignment to parameter 'q' in bounds-attributed group}} } +// Assigned and used + +void good_assigned_and_used(int *__counted_by(count) p, int count, std::span sp) { + p = sp.first(count).data(); + count = count; +} + +void bad_assigned_and_used(int *__counted_by(count) p, int count, std::span sp, int new_count) { + p = sp.first(count).data(); // expected-note{{used here}} + count = new_count; // expected-warning{{parameter 'count' is assigned and used in the same bounds-attributed group}} +} + +void bad_assigned_and_used2(int *__counted_by(a + b) p, int a, int b, std::span sp) { + p = sp.first(b + 42).data(); // expected-note{{used here}} + b = 42; // expected-warning{{parameter 'b' is assigned and used in the same bounds-attributed group}} + a = b; +} + // Assigns to bounds-attributed that we consider too complex to analyze. void too_complex_assign_to_ptr(int *__counted_by(count) p, int count, int *q) {