diff --git a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h index db114ad2aee2c..1c555a93765bb 100644 --- a/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h +++ b/clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h @@ -149,6 +149,21 @@ class UnsafeBufferUsageHandler { ASTContext &Ctx) { handleUnsafeOperation(E, IsRelatedToDecl, Ctx); } + + enum class AssignToImmutableObjectKind { + PointerToPointer, + PointerToDependentCount, + PointerDependingOnInoutCount, + DependentCountUsedInInoutPointer, + }; + + virtual void handleAssignToImmutableObject(const BinaryOperator *Assign, + const ValueDecl *VD, + AssignToImmutableObjectKind Kind, + 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 b416c5c8c79bc..1242bf44eef80 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -14114,6 +14114,13 @@ def warn_assign_to_count_attributed_must_be_simple_stmt : Warning< "assignment to %select{count-attributed pointer|dependent count}0 '%1' " "must be a simple statement '%1 = ...'">, InGroup, DefaultIgnore; +def warn_cannot_assign_to_immutable_bounds_attributed_object : Warning< + "cannot assign to %select{variable|parameter|member}0 %1 because %select{" + "it points to a count-attributed pointer|" + "it points to a dependent count|" + "its type depends on an inout dependent count|" + "it's used as dependent count in an inout count-attributed pointer" + "}2">, 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 f6cbc3c87cc35..1fd38b54ffa4d 100644 --- a/clang/lib/Analysis/UnsafeBufferUsage.cpp +++ b/clang/lib/Analysis/UnsafeBufferUsage.cpp @@ -5654,6 +5654,89 @@ struct BoundsAttributedGroupFinder } }; +// Checks if the bounds-attributed group does not assign to implicitly +// immutable objects (check below which patterns are considered immutable). +// This function returns false iff at least one assignment modifies +// bounds-attributed object that should be immutable. +static bool checkImmutableBoundsAttributedObjects( + const BoundsAttributedAssignmentGroup &Group, + UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) { + bool IsGroupSafe = true; + + for (size_t I = 0, N = Group.Assignments.size(); I < N; ++I) { + const BinaryOperator *Assign = Group.Assignments[I]; + const BoundsAttributedObject &Object = Group.AssignedObjects[I]; + + const ValueDecl *VD = Object.Decl; + QualType Ty = VD->getType(); + int DerefLevel = Object.DerefLevel; + + using AssignKind = UnsafeBufferUsageHandler::AssignToImmutableObjectKind; + + // void foo(int *__counted_by(*len) *p, int *len) { + // p = ...; + // } + if (DerefLevel == 0 && Ty->isPointerType() && + Ty->getPointeeType()->isBoundsAttributedType()) { + Handler.handleAssignToImmutableObject(Assign, VD, + AssignKind::PointerToPointer, + /*IsRelatedToDecl=*/false, Ctx); + IsGroupSafe = false; + } + + // void foo(int *__counted_by(*len) *p, int *len) { + // len = ...; + // } + if (DerefLevel == 0 && VD->isDependentCountWithDeref()) { + Handler.handleAssignToImmutableObject(Assign, VD, + AssignKind::PointerToDependentCount, + /*IsRelatedToDecl=*/false, Ctx); + IsGroupSafe = false; + } + + // `p` below should be immutable, because updating `p` would not be visible + // on the call-site to `foo`, but `*len` would, which invalidates the + // relation between the pointer and its count. + // void foo(int *__counted_by(*len) p, int *len) { + // p = ...; + // } + if (DerefLevel == 0 && Ty->isCountAttributedTypeDependingOnInoutCount()) { + Handler.handleAssignToImmutableObject( + Assign, VD, AssignKind::PointerDependingOnInoutCount, + /*IsRelatedToDecl=*/false, Ctx); + IsGroupSafe = false; + } + + // Same as above, we cannot update `len`, because it won't be visible on + // the call-site. + // void foo(int *__counted_by(len) *p, int *__counted_by(len) q, int len) { + // len = ...; // bad because of p + // } + if (VD->isDependentCountWithoutDeref() && + VD->isDependentCountThatIsUsedInInoutPointer()) { + assert(DerefLevel == 0); + Handler.handleAssignToImmutableObject( + Assign, VD, AssignKind::DependentCountUsedInInoutPointer, + /*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 +checkBoundsAttributedGroup(const BoundsAttributedAssignmentGroup &Group, + UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) { + if (!checkImmutableBoundsAttributedObjects(Group, Handler, Ctx)) + return false; + + // TODO: Add more checks. + return true; +} + static void diagnoseTooComplexAssignToBoundsAttributed(const Expr *E, const ValueDecl *VD, UnsafeBufferUsageHandler &Handler, @@ -5679,7 +5762,7 @@ static void checkBoundsSafetyAssignments(const Stmt *S, ASTContext &Ctx) { BoundsAttributedGroupFinder Finder( [&](const BoundsAttributedAssignmentGroup &Group) { - // TODO: Check group constraints. + checkBoundsAttributedGroup(Group, Handler, Ctx); }, [&](const Expr *E, const ValueDecl *VD) { diagnoseTooComplexAssignToBoundsAttributed(E, VD, Handler, Ctx); diff --git a/clang/lib/Sema/AnalysisBasedWarnings.cpp b/clang/lib/Sema/AnalysisBasedWarnings.cpp index 996a3c32874b5..9fb25d29986e2 100644 --- a/clang/lib/Sema/AnalysisBasedWarnings.cpp +++ b/clang/lib/Sema/AnalysisBasedWarnings.cpp @@ -2609,6 +2609,30 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler { S.Diag(Loc, diag::warn_assign_to_count_attributed_must_be_simple_stmt) << VD->isDependentCount() << VD->getName(); } + + static int getBoundsAttributedObjectKind(const ValueDecl *VD) { + switch (VD->getKind()) { + case Decl::Var: + return 0; + case Decl::ParmVar: + return 1; + case Decl::Field: + return 2; + default: + llvm_unreachable("unexpected bounds-attributed decl kind"); + return 0; + } + } + + void handleAssignToImmutableObject(const BinaryOperator *Assign, + const ValueDecl *VD, + AssignToImmutableObjectKind Kind, + bool IsRelatedToDecl, + ASTContext &Ctx) override { + S.Diag(Assign->getOperatorLoc(), + diag::warn_cannot_assign_to_immutable_bounds_attributed_object) + << getBoundsAttributedObjectKind(VD) << VD << Kind; + } /* 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 d463a02495700..f4f6c391743af 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 @@ -69,6 +69,148 @@ void good_struct_self_loop(cb *c) { } } +// Inout pointer and count + +void good_inout_span(int *__counted_by(*count) *p, size_t *count, std::span sp) { + *p = sp.data(); + *count = sp.size(); +} + +void bad_inout_span(int *__counted_by(*count) *p, size_t *count, std::span sp) { + *p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}} + *count = 42; +} + +void good_inout_subspan_const(int *__counted_by(*count) *p, size_t *count, std::span sp) { + *p = sp.first(42).data(); + *count = 42; +} + +void good_inout_subspan_var(int *__counted_by(*count) *p, size_t *count, std::span sp, size_t new_count) { + *p = sp.first(new_count).data(); + *count = new_count; +} + +void good_inout_subspan_complex(int *__counted_by(*count) *p, size_t *count, std::span sp, size_t i, size_t j) { + *p = sp.first(i + j * 2).data(); + *count = i + j * 2; +} + +void good_inout_span_if(int *__counted_by(*count) *p, size_t *count, std::span sp) { + if (p && count) { + *p = sp.data(); + *count = sp.size(); + } +} + +void bad_inout_span_if(int *__counted_by(*count) *p, size_t *count, std::span sp, size_t size) { + if (p && count) { + *p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}} + *count = size; + } +} + +class inout_class { + void good_inout_span(int *__counted_by(*count) *p, size_t *count, std::span sp) { + *p = sp.data(); + *count = sp.size(); + } + + void bad_inout_span(int *__counted_by(*count) *p, size_t *count, std::span sp) { + *p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}} + *count = 42; + } + + void good_inout_subspan_const(int *__counted_by(*count) *p, size_t *count, std::span sp) { + *p = sp.first(42).data(); + *count = 42; + } +}; + +// Inout pointer + +void bad_inout_ptr_span(int *__counted_by(count) *p, int count, std::span sp) { + *p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}} +} + +void good_inout_ptr_subspan(int *__counted_by(count) *p, size_t count, std::span sp) { + *p = sp.first(count).data(); +} + +void good_inout_ptr_const_subspan(int *__counted_by(42) *p, std::span sp) { + *p = sp.first(42).data(); +} + +void good_inout_ptr_multi_subspan(int *__counted_by(a + b) *p, size_t a, size_t b, std::span sp) { + *p = sp.first(a + b).data(); +} + +class inout_ptr_class { + void bad_inout_ptr_span(int *__counted_by(count) *p, int count, std::span sp) { + *p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}} + } + + void good_inout_ptr_subspan(int *__counted_by(count) *p, size_t count, std::span sp) { + *p = sp.first(count).data(); + } +}; + +// Immutable pointers/dependent values + +void immutable_ptr_to_ptr(int *__counted_by(*count) *p, int *count) { + p = nullptr; // expected-warning{{cannot assign to parameter 'p' because it points to a count-attributed pointer}} + *count = 0; +} + +void immutable_ptr_to_value(int *__counted_by(*count) *p, int *count) { + *p = nullptr; + count = nullptr; // expected-warning{{cannot assign to parameter 'count' because it points to a dependent count}} +} + +void immutable_ptr_with_inout_value(int *__counted_by(*count) p, int *count) { + p = nullptr; // expected-warning{{cannot assign to parameter 'p' because its type depends on an inout dependent count}} + *count = 0; +} + +void immutable_ptr_with_inout_value2(int *__counted_by(*count) p, int *__counted_by(*count) *q, int *count) { + p = nullptr; // expected-warning{{cannot assign to parameter 'p' because its type depends on an inout dependent count}} + *q = nullptr; + *count = 0; +} + +void immutable_value_with_inout_ptr(int *__counted_by(count) *p, int count) { + *p = nullptr; + count = 0; // expected-warning{{cannot assign to parameter 'count' because it's used as dependent count in an inout count-attributed pointer}} +} + +void immutable_value_with_inout_ptr2(int *__counted_by(count) p, int *__counted_by(count) *q, int count) { + p = nullptr; + *q = nullptr; + count = 0; // expected-warning{{cannot assign to parameter 'count' because it's used as dependent count in an inout count-attributed pointer}} +} + +class immutable_class { + void immutable_ptr_to_ptr(int *__counted_by(*count) *p, int *count) { + p = nullptr; // expected-warning{{cannot assign to parameter 'p' because it points to a count-attributed pointer}} + *count = 0; + } + + void immutable_ptr_to_value(int *__counted_by(*count) *p, int *count) { + *p = nullptr; + count = nullptr; // expected-warning{{cannot assign to parameter 'count' because it points to a dependent count}} + } + + void immutable_ptr_with_inout_value(int *__counted_by(*count) p, int *count) { + p = nullptr; // expected-warning{{cannot assign to parameter 'p' because its type depends on an inout dependent count}} + *count = 0; + } + + void immutable_value_with_inout_ptr(int *__counted_by(count) *p, int count) { + *p = nullptr; + count = 0; // expected-warning{{cannot assign to parameter 'count' because it's used as dependent count in an inout count-attributed pointer}} + } +}; + // 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) {