From 9fe79db271a87dba3173b72e456fcd32d371d47a Mon Sep 17 00:00:00 2001 From: Patryk Stefanski Date: Fri, 17 Oct 2025 17:25:59 -0700 Subject: [PATCH] [-Wunsafe-buffer-usage] Check missing and duplicated assignments Add checks for missing and duplicated assignments in bounds-attributed groups. This model requires each mutable decl in a group to be assigned exactly once. rdar://161608243 --- .../Analysis/Analyses/UnsafeBufferUsage.h | 16 +++ .../clang/Basic/DiagnosticSemaKinds.td | 6 + clang/lib/Analysis/UnsafeBufferUsage.cpp | 61 ++++++++- clang/lib/Sema/AnalysisBasedWarnings.cpp | 42 ++++++ ...ge-count-attributed-pointer-assignment.cpp | 128 ++++++++++++++++++ 5 files changed, 252 insertions(+), 1 deletion(-) diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index da020ba915090..eccb64fd4d3ed 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -165,6 +165,22 @@ class UnsafeBufferUsageHandler { ASTContext &Ctx) { handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx); } + + virtual void handleMissingAssignments( + const Expr *LastAssignInGroup, + const llvm::SmallPtrSetImpl &Required, + const llvm::SmallPtrSetImpl &Missing, + bool IsRelatedToDecl, ASTContext &Ctx) { + handleUnsafeOperation(LastAssignInGroup, IsRelatedToDecl, Ctx); + } + + virtual void handleDuplicatedAssignment(const BinaryOperator *Assign, + const BinaryOperator *PrevAssign, + 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 ea395e5ab1d6f..3b0838d4c49ca 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -14280,6 +14280,12 @@ def warn_cannot_assign_to_immutable_bounds_attributed_object : Warning< "its type depends on an inout dependent count|" "it's used as dependent count in an inout count-attributed pointer" "}2">, InGroup, DefaultIgnore; +def warn_missing_assignments_in_bounds_attributed_group : Warning< + "bounds-attributed group requires assigning '%0', assignments to '%1' missing">, + InGroup, DefaultIgnore; +def warn_duplicated_assignment_in_bounds_attributed_group : Warning< + "duplicated assignment to %select{variable|parameter|member}0 %1 in " + "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 e93a7e3b67493..87d9c1c2bf4a5 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -5889,6 +5889,64 @@ static bool checkImmutableBoundsAttributedObjects( return IsGroupSafe; } +// Checks if the bounds-attributed group has missing or duplicated assignments. +// Each assignable decl in the group must be assigned exactly once. +// +// For example: +// void foo(int *__counted_by(a + b) p, int a, int b) { +// p = ...; +// p = ...; // duplicated +// a = ...; +// // b missing +// } +// Note that the group consists of a, b, and p. +// +// The function returns true iff each assignable decl in the group is assigned +// exactly once. +static bool checkMissingAndDuplicatedAssignments( + const BoundsAttributedAssignmentGroup &Group, + UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) { + llvm::SmallDenseMap + AssignedDecls; + + DependentDeclSetTy RequiredDecls = Group.DeclClosure; + for (const ValueDecl *VD : Group.DeclClosure) { + if (VD->isDependentCountWithoutDeref() && + VD->isDependentCountThatIsUsedInInoutPointer()) { + // This value is immutable, so it's not required to be assigned. + RequiredDecls.erase(VD); + } + } + + DependentDeclSetTy MissingDecls = RequiredDecls; + + bool IsGroupSafe = true; + + for (size_t I = 0, N = Group.Assignments.size(); I < N; ++I) { + const BinaryOperator *Assign = Group.Assignments[I]; + const ValueDecl *VD = Group.AssignedObjects[I].Decl; + + const auto [It, Inserted] = AssignedDecls.insert({VD, Assign}); + if (Inserted) + MissingDecls.erase(VD); + else { + const BinaryOperator *PrevAssign = It->second; + Handler.handleDuplicatedAssignment(Assign, PrevAssign, VD, + /*IsRelatedToDecl=*/false, Ctx); + IsGroupSafe = false; + } + } + + if (!MissingDecls.empty()) { + const Expr *LastAssign = Group.Assignments.back(); + Handler.handleMissingAssignments(LastAssign, RequiredDecls, MissingDecls, + /*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 @@ -5896,7 +5954,8 @@ checkBoundsAttributedGroup(const BoundsAttributedAssignmentGroup &Group, UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) { if (!checkImmutableBoundsAttributedObjects(Group, Handler, Ctx)) return false; - + if (!checkMissingAndDuplicatedAssignments(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 29669f26343c2..61a48546989ca 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2643,6 +2643,48 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { diag::warn_cannot_assign_to_immutable_bounds_attributed_object) << getBoundsAttributedObjectKind(VD) << VD << Kind; } + + static llvm::SmallString<64> + DeclSetToStr(const llvm::SmallPtrSetImpl &Set) { + llvm::SmallVector V(Set.begin(), Set.end()); + llvm::sort(V.begin(), V.end(), [](const ValueDecl *A, const ValueDecl *B) { + return A->getName().compare(B->getName()) < 0; + }); + llvm::SmallString<64> Str; + for (const ValueDecl *VD : V) { + if (!Str.empty()) + Str += ", "; + Str += VD->getName(); + } + return Str; + } + + void handleMissingAssignments( + const Expr *LastAssignInGroup, + const llvm::SmallPtrSetImpl &Required, + const llvm::SmallPtrSetImpl &Missing, + bool IsRelatedToDecl, ASTContext &Ctx) override { + + llvm::SmallString<64> RequiredAssignments = DeclSetToStr(Required); + llvm::SmallString<64> MissingAssignments = DeclSetToStr(Missing); + auto Loc = + CharSourceRange::getTokenRange(LastAssignInGroup->getSourceRange()) + .getEnd(); + + S.Diag(Loc, diag::warn_missing_assignments_in_bounds_attributed_group) + << RequiredAssignments << MissingAssignments; + } + + void handleDuplicatedAssignment(const BinaryOperator *Assign, + const BinaryOperator *PrevAssign, + const ValueDecl *VD, bool IsRelatedToDecl, + ASTContext &Ctx) override { + S.Diag(Assign->getOperatorLoc(), + diag::warn_duplicated_assignment_in_bounds_attributed_group) + << getBoundsAttributedObjectKind(VD) << VD; + S.Diag(PrevAssign->getOperatorLoc(), + diag::note_bounds_safety_previous_assignment); + } /* 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 f4f6c391743af..226ec6e29a090 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 @@ -211,6 +211,134 @@ class immutable_class { } }; +// Missing assignments + +void missing_ptr(int *__counted_by(count) p, int count) { + count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}} +} + +void missing_count(int *__counted_by(count) p, int count) { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} +} + +void missing_structure(int *__counted_by(count) p, int count) { + { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} + } + { + count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}} + } +} + +void missing_structure2(int *__counted_by(count) p, int count) { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} + { + count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}} + } +} + +void missing_structure3(int *__counted_by(count) p, int count) { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} + if (count > 0) { + count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}} + } +} + +void missing_unrelated(int *__counted_by(count) p, int count, int *__counted_by(len) q, int len) { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} + len = 0; // expected-warning{{bounds-attributed group requires assigning 'len, q', assignments to 'q' missing}} +} + +void missing_complex_count1(int *__counted_by(a + b) p, int a, int b) { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'a, b' missing}} +} + +void missing_complex_count2(int *__counted_by(a + b) p, int a, int b) { + p = nullptr; + a = 0; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'b' missing}} +} + +void missing_complex_count3(int *__counted_by(a + b) p, int a, int b) { + b = 0; + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'a' missing}} +} + +void missing_complex_count4(int *__counted_by(a + b) p, int a, int b) { + a = 0; + b = 0; // expected-warning{{bounds-attributed group requires assigning 'a, b, p', assignments to 'p' missing}} +} + +void missing_complex_ptr1(int *__counted_by(count) p, int *__counted_by(count) q, int count) { + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'count, q' missing}} +} + +void missing_complex_ptr2(int *__counted_by(count) p, int *__counted_by(count) q, int count) { + p = nullptr; + q = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'count' missing}} +} + +void missing_complex_ptr3(int *__counted_by(count) p, int *__counted_by(count) q, int count) { + count = 0; + p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'q' missing}} +} + +void missing_complex_ptr4(int *__counted_by(count) p, int *__counted_by(count) q, int count) { + q = nullptr; + count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p, q', assignments to 'p' missing}} +} + +// Missing assignments in struct + +void missing_struct_ptr(cb *c) { + c->count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}} +} + +void missing_struct_count(cb *c) { + c->p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} +} + +void missing_struct_unrelated(cb *c, cb *d) { + c->p = nullptr; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'count' missing}} + d->count = 0; // expected-warning{{bounds-attributed group requires assigning 'count, p', assignments to 'p' missing}} +} + +// Duplicated assignments + +void duplicated_ptr(int *__counted_by(count) p, int count) { + p = nullptr; // expected-note{{previously assigned here}} + p = nullptr; // expected-warning{{duplicated assignment to parameter 'p' in bounds-attributed group}} + count = 0; +} + +void duplicated_ptr2(int *__counted_by(count) p, int count) { + p = nullptr; // expected-note{{previously assigned here}} + count = 0; + p = nullptr; // expected-warning{{duplicated assignment to parameter 'p' in bounds-attributed group}} +} + +void duplicated_count(int *__counted_by(count) p, int count) { + p = nullptr; + count = 0; // expected-note{{previously assigned here}} + count = 0; // expected-warning{{duplicated assignment to parameter 'count' in bounds-attributed group}} +} + +void duplicated_count2(int *__counted_by(count) p, int count) { + count = 0; // expected-note{{previously assigned here}} + p = nullptr; + count = 0; // expected-warning{{duplicated assignment to parameter 'count' in bounds-attributed group}} +} + +void duplicated_complex(int *__counted_by(a + b) p, + int *__counted_by(a + b + c) q, + int a, int b, int c) { + p = nullptr; + q = nullptr; // expected-note{{previously assigned here}} + a = 0; + b = 0; + c = 0; + q = nullptr; // expected-warning{{duplicated assignment to parameter 'q' in bounds-attributed group}} +} + // 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) {