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) {