Skip to content

Commit 99b91ff

Browse files
[-Wunsafe-buffer-usage] Check count-attributed assignment groups
This change adds support for checking count-attributed assignment groups. This commit adds the following checks: 1. Standalone assignments to count-attributed objects (pointers/dependent counts) that are not directly inside of a compound statement. Our model rejects those and requires the user to simplify their code if necessary. For example: ``` void foo(int *__counted_by(count) p, int count) { q = p = ...; ^ this is rejected n = count = ...; ^ this is rejected // the following is fine: p = ...; count = ...; } ``` 2. Assignments to count-attributed objects that are implicitly immutable. For example, assigning to a dependent count that is used in an inout pointer is not allowed, since the update won't be visible on the call-site: ``` void foo(int *__counted_by(count) *out_p, int count) { *out_p = ...; count = ...; // immutable } ``` 3. Missing and duplicated assignments: ``` void foo(int *__counted_by(a + b) p, int a, int b) { p = ...; p = ...; // duplicated a = ...; // b missing } ``` 4. Count-attributed objects that are assigned and used in the same group. Allowing such assignments can cause the bounds-check to use the old dependent count, while updating the count to a new value. For example, 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<int> sp) { p = sp.first(b + 42).data(); b = 42; // b is assigned and used a = b; } ``` 5. Safe assignment patterns. This uses the infrastructure that is already available for count-attributed arguments, and checks for each assigned pointer in the group that the RHS has enough elements. This analysis is hidden behind `-fexperimental-bounds-safety-attributes` flag, so that we don't waste cycles traversing the AST when the attributes are not enabled. rdar://128160398 rdar://128161580
1 parent 16f18a3 commit 99b91ff

File tree

9 files changed

+1268
-30
lines changed

9 files changed

+1268
-30
lines changed

clang/include/clang/AST/Decl.h

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -732,6 +732,10 @@ class ValueDecl : public NamedDecl {
732732
bool isInitCapture() const;
733733

734734
/* TO_UPSTREAM(BoundsSafety) ON */
735+
bool isDependentValue() const;
736+
bool isDependentValueWithoutDeref() const;
737+
bool isDependentValueWithDeref() const;
738+
bool isDependentValueThatIsUsedInInoutPointer() const;
735739
/// Whether this decl is a dependent parameter referred to by the return type
736740
/// that is a bounds-attributed type.
737741
bool isDependentParamOfReturnType(

clang/include/clang/AST/TypeBase.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2651,6 +2651,7 @@ class alignas(TypeAlignment) Type : public ExtQualsTypeCommonBase {
26512651
bool isAnyVaListType(ASTContext &) const;
26522652
bool isDynamicRangePointerType() const;
26532653
bool isBoundsAttributedType() const;
2654+
bool isBoundsAttributedTypeDependingOnInoutValue() const;
26542655
bool isValueTerminatedType() const;
26552656
bool isImplicitlyNullTerminatedType(const ASTContext &) const;
26562657
/* TO_UPSTREAM(BoundsSafety) OFF */

clang/include/clang/Analysis/Analyses/UnsafeBufferUsage.h

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
#include "clang/AST/Expr.h"
1919
#include "clang/AST/Stmt.h"
2020
#include "clang/Basic/SourceLocation.h"
21+
#include "llvm/ADT/SmallPtrSet.h"
2122
#include "llvm/Support/Debug.h"
2223
#include <set>
2324

@@ -142,6 +143,53 @@ class UnsafeBufferUsageHandler {
142143
ASTContext &Ctx) {
143144
handleUnsafeOperation(Arg, IsRelatedToDecl, Ctx);
144145
}
146+
147+
virtual void handleStandaloneAssign(const Expr *E, const ValueDecl *VD,
148+
bool IsRelatedToDecl, ASTContext &Ctx) {
149+
handleUnsafeOperation(E, IsRelatedToDecl, Ctx);
150+
}
151+
152+
enum class AssignToImmutableObjectKind {
153+
PointerToPointer,
154+
PointerToDependentValue,
155+
PointerDependingOnInoutValue,
156+
DependentValueUsedInInoutPointer,
157+
};
158+
159+
virtual void handleAssignToImmutableObject(const BinaryOperator *Assign,
160+
const ValueDecl *VD,
161+
AssignToImmutableObjectKind Kind,
162+
bool IsRelatedToDecl,
163+
ASTContext &Ctx) {
164+
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
165+
}
166+
167+
virtual void handleMissingAssignments(
168+
const Expr *LastAssignInGroup,
169+
const llvm::SmallPtrSetImpl<const ValueDecl *> &Required,
170+
const llvm::SmallPtrSetImpl<const ValueDecl *> &Missing,
171+
bool IsRelatedToDecl, ASTContext &Ctx) {
172+
handleUnsafeOperation(LastAssignInGroup, IsRelatedToDecl, Ctx);
173+
}
174+
175+
virtual void handleDuplicatedAssignment(const BinaryOperator *Assign,
176+
const BinaryOperator *PrevAssign,
177+
const ValueDecl *VD,
178+
bool IsRelatedToDecl,
179+
ASTContext &Ctx) {
180+
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
181+
}
182+
183+
virtual void handleAssignedAndUsed(const BinaryOperator *Assign,
184+
const Expr *Use, const ValueDecl *VD,
185+
bool IsRelatedToDecl, ASTContext &Ctx) {
186+
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
187+
}
188+
189+
virtual void handleUnsafeCountAttributedPointerAssignment(
190+
const BinaryOperator *Assign, bool IsRelatedToDecl, ASTContext &Ctx) {
191+
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
192+
}
145193
/* TO_UPSTREAM(BoundsSafety) OFF */
146194

147195
/// Invoked when a fix is suggested against a variable. This function groups
@@ -196,7 +244,8 @@ class UnsafeBufferUsageHandler {
196244
// This function invokes the analysis and allows the caller to react to it
197245
// through the handler class.
198246
void checkUnsafeBufferUsage(const Decl *D, UnsafeBufferUsageHandler &Handler,
199-
bool EmitSuggestions);
247+
bool EmitSuggestions,
248+
bool BoundsSafetyAttributes = false);
200249

201250
namespace internal {
202251
// Tests if any two `FixItHint`s in `FixIts` conflict. Two `FixItHint`s

clang/include/clang/Basic/DiagnosticSemaKinds.td

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14236,6 +14236,29 @@ def note_unsafe_count_attributed_pointer_argument_null_to_nonnull : Note<
1423614236
def warn_unsafe_single_pointer_argument : Warning<
1423714237
"unsafe assignment to function parameter of __single pointer type">,
1423814238
InGroup<UnsafeBufferUsage>, DefaultIgnore;
14239+
def warn_standalone_assign_to_bounds_attributed_object : Warning<
14240+
"assignment to %select{bounds-attributed pointer|dependent value}0 %1 "
14241+
"must be inside of a bounds-attributed group in a compound statement">,
14242+
InGroup<UnsafeBufferUsage>, DefaultIgnore;
14243+
def warn_cannot_assign_to_immutable_bounds_attributed_object : Warning<
14244+
"cannot assign to %select{variable|parameter|member}0 %1 because %select{"
14245+
"it points to a bounds-attributed pointer|"
14246+
"it points to a dependent value|"
14247+
"its type depends on an inout dependent value|"
14248+
"it's used as dependent value in an inout bounds-attributed pointer"
14249+
"}2">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
14250+
def warn_missing_assignments_in_bounds_attributed_group : Warning<
14251+
"bounds-attributed group requires assigning '%0', assignments to '%1' missing">,
14252+
InGroup<UnsafeBufferUsage>, DefaultIgnore;
14253+
def warn_duplicated_assignment_in_bounds_attributed_group : Warning<
14254+
"duplicated assignment to %select{variable|parameter|member}0 %1 in "
14255+
"bounds-attributed group">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
14256+
def warn_assigned_and_used_in_bounds_attributed_group : Warning<
14257+
"%select{variable|parameter|member}0 %1 is assigned and used in the same "
14258+
"bounds-attributed group">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
14259+
def warn_unsafe_count_attributed_pointer_assignment : Warning<
14260+
"unsafe assignment to count-attributed pointer">,
14261+
InGroup<UnsafeBufferUsage>, DefaultIgnore;
1423914262
#ifndef NDEBUG
1424014263
// Not a user-facing diagnostic. Useful for debugging false negatives in
1424114264
// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).

clang/lib/AST/Decl.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5502,6 +5502,27 @@ bool ValueDecl::isParameterPack() const {
55025502
}
55035503

55045504
/* TO_UPSTREAM(BoundsSafety) ON */
5505+
bool ValueDecl::isDependentValue() const {
5506+
return hasAttr<DependerDeclsAttr>();
5507+
}
5508+
5509+
bool ValueDecl::isDependentValueWithoutDeref() const {
5510+
const auto *Att = getAttr<DependerDeclsAttr>();
5511+
return Att && !Att->getIsDeref();
5512+
}
5513+
5514+
bool ValueDecl::isDependentValueWithDeref() const {
5515+
const auto *Att = getAttr<DependerDeclsAttr>();
5516+
return Att && Att->getIsDeref();
5517+
}
5518+
5519+
bool ValueDecl::isDependentValueThatIsUsedInInoutPointer() const {
5520+
const auto *Att = getAttr<DependerDeclsAttr>();
5521+
return Att &&
5522+
std::any_of(Att->dependerLevels_begin(), Att->dependerLevels_end(),
5523+
[](unsigned Level) { return Level > 0; });
5524+
}
5525+
55055526
bool ValueDecl::isDependentParamOfReturnType(
55065527
const BoundsAttributedType **RetType,
55075528
const TypeCoupledDeclRefInfo **Info) const {

clang/lib/AST/Type.cpp

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -829,6 +829,14 @@ bool Type::isBoundsAttributedType() const {
829829
return getAs<BoundsAttributedType>();
830830
}
831831

832+
bool Type::isBoundsAttributedTypeDependingOnInoutValue() const {
833+
const auto *BAT = getAs<BoundsAttributedType>();
834+
return BAT &&
835+
std::any_of(
836+
BAT->dependent_decl_begin(), BAT->dependent_decl_end(),
837+
[](const TypeCoupledDeclRefInfo &Info) { return Info.isDeref(); });
838+
}
839+
832840
bool Type::isValueTerminatedType() const {
833841
return getAs<ValueTerminatedType>();
834842
}

0 commit comments

Comments
 (0)