diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index 1c555a93765bb..e517fe3e39a86 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -164,6 +164,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 1242bf44eef80..83f12f5b42af8 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -14121,6 +14121,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 1fd38b54ffa4d..592492dc2fd4b 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -5725,6 +5725,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 @@ -5732,7 +5790,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 9fb25d29986e2..306743186b617 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2633,6 +2633,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) {