Skip to content

Commit ef7cbc7

Browse files
patrykstefanskipatrykstefanski
committed
[-Wunsafe-buffer-usage] Check assignments to implicitly immutable count-attributed objects
Check 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 } ``` rdar://161608157 (cherry picked from commit 7f73488)
1 parent 1ce6f47 commit ef7cbc7

File tree

5 files changed

+272
-1
lines changed

5 files changed

+272
-1
lines changed

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,21 @@ class UnsafeBufferUsageHandler {
149149
ASTContext &Ctx) {
150150
handleUnsafeOperation(E, IsRelatedToDecl, Ctx);
151151
}
152+
153+
enum class AssignToImmutableObjectKind {
154+
PointerToPointer,
155+
PointerToDependentCount,
156+
PointerDependingOnInoutCount,
157+
DependentCountUsedInInoutPointer,
158+
};
159+
160+
virtual void handleAssignToImmutableObject(const BinaryOperator *Assign,
161+
const ValueDecl *VD,
162+
AssignToImmutableObjectKind Kind,
163+
bool IsRelatedToDecl,
164+
ASTContext &Ctx) {
165+
handleUnsafeOperation(Assign, IsRelatedToDecl, Ctx);
166+
}
152167
/* TO_UPSTREAM(BoundsSafety) OFF */
153168

154169
/// Invoked when a fix is suggested against a variable. This function groups

‎clang/include/clang/Basic/DiagnosticSemaKinds.td‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14114,6 +14114,13 @@ def warn_assign_to_count_attributed_must_be_simple_stmt : Warning<
1411414114
"assignment to %select{count-attributed pointer|dependent count}0 '%1' "
1411514115
"must be a simple statement '%1 = ...'">,
1411614116
InGroup<UnsafeBufferUsage>, DefaultIgnore;
14117+
def warn_cannot_assign_to_immutable_bounds_attributed_object : Warning<
14118+
"cannot assign to %select{variable|parameter|member}0 %1 because %select{"
14119+
"it points to a count-attributed pointer|"
14120+
"it points to a dependent count|"
14121+
"its type depends on an inout dependent count|"
14122+
"it's used as dependent count in an inout count-attributed pointer"
14123+
"}2">, InGroup<UnsafeBufferUsage>, DefaultIgnore;
1411714124
#ifndef NDEBUG
1411814125
// Not a user-facing diagnostic. Useful for debugging false negatives in
1411914126
// -fsafe-buffer-usage-suggestions (i.e. lack of -Wunsafe-buffer-usage fixits).

‎clang/lib/Analysis/UnsafeBufferUsage.cpp‎

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5654,6 +5654,89 @@ struct BoundsAttributedGroupFinder
56545654
}
56555655
};
56565656

5657+
// Checks if the bounds-attributed group does not assign to implicitly
5658+
// immutable objects (check below which patterns are considered immutable).
5659+
// This function returns false iff at least one assignment modifies
5660+
// bounds-attributed object that should be immutable.
5661+
static bool checkImmutableBoundsAttributedObjects(
5662+
const BoundsAttributedAssignmentGroup &Group,
5663+
UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) {
5664+
bool IsGroupSafe = true;
5665+
5666+
for (size_t I = 0, N = Group.Assignments.size(); I < N; ++I) {
5667+
const BinaryOperator *Assign = Group.Assignments[I];
5668+
const BoundsAttributedObject &Object = Group.AssignedObjects[I];
5669+
5670+
const ValueDecl *VD = Object.Decl;
5671+
QualType Ty = VD->getType();
5672+
int DerefLevel = Object.DerefLevel;
5673+
5674+
using AssignKind = UnsafeBufferUsageHandler::AssignToImmutableObjectKind;
5675+
5676+
// void foo(int *__counted_by(*len) *p, int *len) {
5677+
// p = ...;
5678+
// }
5679+
if (DerefLevel == 0 && Ty->isPointerType() &&
5680+
Ty->getPointeeType()->isBoundsAttributedType()) {
5681+
Handler.handleAssignToImmutableObject(Assign, VD,
5682+
AssignKind::PointerToPointer,
5683+
/*IsRelatedToDecl=*/false, Ctx);
5684+
IsGroupSafe = false;
5685+
}
5686+
5687+
// void foo(int *__counted_by(*len) *p, int *len) {
5688+
// len = ...;
5689+
// }
5690+
if (DerefLevel == 0 && VD->isDependentCountWithDeref()) {
5691+
Handler.handleAssignToImmutableObject(Assign, VD,
5692+
AssignKind::PointerToDependentCount,
5693+
/*IsRelatedToDecl=*/false, Ctx);
5694+
IsGroupSafe = false;
5695+
}
5696+
5697+
// `p` below should be immutable, because updating `p` would not be visible
5698+
// on the call-site to `foo`, but `*len` would, which invalidates the
5699+
// relation between the pointer and its count.
5700+
// void foo(int *__counted_by(*len) p, int *len) {
5701+
// p = ...;
5702+
// }
5703+
if (DerefLevel == 0 && Ty->isCountAttributedTypeDependingOnInoutCount()) {
5704+
Handler.handleAssignToImmutableObject(
5705+
Assign, VD, AssignKind::PointerDependingOnInoutCount,
5706+
/*IsRelatedToDecl=*/false, Ctx);
5707+
IsGroupSafe = false;
5708+
}
5709+
5710+
// Same as above, we cannot update `len`, because it won't be visible on
5711+
// the call-site.
5712+
// void foo(int *__counted_by(len) *p, int *__counted_by(len) q, int len) {
5713+
// len = ...; // bad because of p
5714+
// }
5715+
if (VD->isDependentCountWithoutDeref() &&
5716+
VD->isDependentCountThatIsUsedInInoutPointer()) {
5717+
assert(DerefLevel == 0);
5718+
Handler.handleAssignToImmutableObject(
5719+
Assign, VD, AssignKind::DependentCountUsedInInoutPointer,
5720+
/*IsRelatedToDecl=*/false, Ctx);
5721+
IsGroupSafe = false;
5722+
}
5723+
}
5724+
5725+
return IsGroupSafe;
5726+
}
5727+
5728+
// Checks if the bounds-attributed group is safe. This function returns false
5729+
// iff the assignment group is unsafe and diagnostics have been emitted.
5730+
static bool
5731+
checkBoundsAttributedGroup(const BoundsAttributedAssignmentGroup &Group,
5732+
UnsafeBufferUsageHandler &Handler, ASTContext &Ctx) {
5733+
if (!checkImmutableBoundsAttributedObjects(Group, Handler, Ctx))
5734+
return false;
5735+
5736+
// TODO: Add more checks.
5737+
return true;
5738+
}
5739+
56575740
static void
56585741
diagnoseTooComplexAssignToBoundsAttributed(const Expr *E, const ValueDecl *VD,
56595742
UnsafeBufferUsageHandler &Handler,
@@ -5679,7 +5762,7 @@ static void checkBoundsSafetyAssignments(const Stmt *S,
56795762
ASTContext &Ctx) {
56805763
BoundsAttributedGroupFinder Finder(
56815764
[&](const BoundsAttributedAssignmentGroup &Group) {
5682-
// TODO: Check group constraints.
5765+
checkBoundsAttributedGroup(Group, Handler, Ctx);
56835766
},
56845767
[&](const Expr *E, const ValueDecl *VD) {
56855768
diagnoseTooComplexAssignToBoundsAttributed(E, VD, Handler, Ctx);

‎clang/lib/Sema/AnalysisBasedWarnings.cpp‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2609,6 +2609,30 @@ class UnsafeBufferUsageReporter : public UnsafeBufferUsageHandler {
26092609
S.Diag(Loc, diag::warn_assign_to_count_attributed_must_be_simple_stmt)
26102610
<< VD->isDependentCount() << VD->getName();
26112611
}
2612+
2613+
static int getBoundsAttributedObjectKind(const ValueDecl *VD) {
2614+
switch (VD->getKind()) {
2615+
case Decl::Var:
2616+
return 0;
2617+
case Decl::ParmVar:
2618+
return 1;
2619+
case Decl::Field:
2620+
return 2;
2621+
default:
2622+
llvm_unreachable("unexpected bounds-attributed decl kind");
2623+
return 0;
2624+
}
2625+
}
2626+
2627+
void handleAssignToImmutableObject(const BinaryOperator *Assign,
2628+
const ValueDecl *VD,
2629+
AssignToImmutableObjectKind Kind,
2630+
bool IsRelatedToDecl,
2631+
ASTContext &Ctx) override {
2632+
S.Diag(Assign->getOperatorLoc(),
2633+
diag::warn_cannot_assign_to_immutable_bounds_attributed_object)
2634+
<< getBoundsAttributedObjectKind(VD) << VD << Kind;
2635+
}
26122636
/* TO_UPSTREAM(BoundsSafety) OFF */
26132637

26142638
void handleUnsafeVariableGroup(const VarDecl *Variable,

‎clang/test/SemaCXX/warn-unsafe-buffer-usage-count-attributed-pointer-assignment.cpp‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,148 @@ void good_struct_self_loop(cb *c) {
6969
}
7070
}
7171

72+
// Inout pointer and count
73+
74+
void good_inout_span(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
75+
*p = sp.data();
76+
*count = sp.size();
77+
}
78+
79+
void bad_inout_span(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
80+
*p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}}
81+
*count = 42;
82+
}
83+
84+
void good_inout_subspan_const(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
85+
*p = sp.first(42).data();
86+
*count = 42;
87+
}
88+
89+
void good_inout_subspan_var(int *__counted_by(*count) *p, size_t *count, std::span<int> sp, size_t new_count) {
90+
*p = sp.first(new_count).data();
91+
*count = new_count;
92+
}
93+
94+
void good_inout_subspan_complex(int *__counted_by(*count) *p, size_t *count, std::span<int> sp, size_t i, size_t j) {
95+
*p = sp.first(i + j * 2).data();
96+
*count = i + j * 2;
97+
}
98+
99+
void good_inout_span_if(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
100+
if (p && count) {
101+
*p = sp.data();
102+
*count = sp.size();
103+
}
104+
}
105+
106+
void bad_inout_span_if(int *__counted_by(*count) *p, size_t *count, std::span<int> sp, size_t size) {
107+
if (p && count) {
108+
*p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}}
109+
*count = size;
110+
}
111+
}
112+
113+
class inout_class {
114+
void good_inout_span(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
115+
*p = sp.data();
116+
*count = sp.size();
117+
}
118+
119+
void bad_inout_span(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
120+
*p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}}
121+
*count = 42;
122+
}
123+
124+
void good_inout_subspan_const(int *__counted_by(*count) *p, size_t *count, std::span<int> sp) {
125+
*p = sp.first(42).data();
126+
*count = 42;
127+
}
128+
};
129+
130+
// Inout pointer
131+
132+
void bad_inout_ptr_span(int *__counted_by(count) *p, int count, std::span<int> sp) {
133+
*p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}}
134+
}
135+
136+
void good_inout_ptr_subspan(int *__counted_by(count) *p, size_t count, std::span<int> sp) {
137+
*p = sp.first(count).data();
138+
}
139+
140+
void good_inout_ptr_const_subspan(int *__counted_by(42) *p, std::span<int> sp) {
141+
*p = sp.first(42).data();
142+
}
143+
144+
void good_inout_ptr_multi_subspan(int *__counted_by(a + b) *p, size_t a, size_t b, std::span<int> sp) {
145+
*p = sp.first(a + b).data();
146+
}
147+
148+
class inout_ptr_class {
149+
void bad_inout_ptr_span(int *__counted_by(count) *p, int count, std::span<int> sp) {
150+
*p = sp.data(); // TODO-expected-warning{{unsafe assignment to count-attributed pointer}}
151+
}
152+
153+
void good_inout_ptr_subspan(int *__counted_by(count) *p, size_t count, std::span<int> sp) {
154+
*p = sp.first(count).data();
155+
}
156+
};
157+
158+
// Immutable pointers/dependent values
159+
160+
void immutable_ptr_to_ptr(int *__counted_by(*count) *p, int *count) {
161+
p = nullptr; // expected-warning{{cannot assign to parameter 'p' because it points to a count-attributed pointer}}
162+
*count = 0;
163+
}
164+
165+
void immutable_ptr_to_value(int *__counted_by(*count) *p, int *count) {
166+
*p = nullptr;
167+
count = nullptr; // expected-warning{{cannot assign to parameter 'count' because it points to a dependent count}}
168+
}
169+
170+
void immutable_ptr_with_inout_value(int *__counted_by(*count) p, int *count) {
171+
p = nullptr; // expected-warning{{cannot assign to parameter 'p' because its type depends on an inout dependent count}}
172+
*count = 0;
173+
}
174+
175+
void immutable_ptr_with_inout_value2(int *__counted_by(*count) p, int *__counted_by(*count) *q, int *count) {
176+
p = nullptr; // expected-warning{{cannot assign to parameter 'p' because its type depends on an inout dependent count}}
177+
*q = nullptr;
178+
*count = 0;
179+
}
180+
181+
void immutable_value_with_inout_ptr(int *__counted_by(count) *p, int count) {
182+
*p = nullptr;
183+
count = 0; // expected-warning{{cannot assign to parameter 'count' because it's used as dependent count in an inout count-attributed pointer}}
184+
}
185+
186+
void immutable_value_with_inout_ptr2(int *__counted_by(count) p, int *__counted_by(count) *q, int count) {
187+
p = nullptr;
188+
*q = nullptr;
189+
count = 0; // expected-warning{{cannot assign to parameter 'count' because it's used as dependent count in an inout count-attributed pointer}}
190+
}
191+
192+
class immutable_class {
193+
void immutable_ptr_to_ptr(int *__counted_by(*count) *p, int *count) {
194+
p = nullptr; // expected-warning{{cannot assign to parameter 'p' because it points to a count-attributed pointer}}
195+
*count = 0;
196+
}
197+
198+
void immutable_ptr_to_value(int *__counted_by(*count) *p, int *count) {
199+
*p = nullptr;
200+
count = nullptr; // expected-warning{{cannot assign to parameter 'count' because it points to a dependent count}}
201+
}
202+
203+
void immutable_ptr_with_inout_value(int *__counted_by(*count) p, int *count) {
204+
p = nullptr; // expected-warning{{cannot assign to parameter 'p' because its type depends on an inout dependent count}}
205+
*count = 0;
206+
}
207+
208+
void immutable_value_with_inout_ptr(int *__counted_by(count) *p, int count) {
209+
*p = nullptr;
210+
count = 0; // expected-warning{{cannot assign to parameter 'count' because it's used as dependent count in an inout count-attributed pointer}}
211+
}
212+
};
213+
72214
// Assigns to bounds-attributed that we consider too complex to analyze.
73215

74216
void too_complex_assign_to_ptr(int *__counted_by(count) p, int count, int *q) {

0 commit comments

Comments
 (0)