-
Notifications
You must be signed in to change notification settings - Fork 349
[DO NOT MERGE][-Wunsafe-buffer-usage] Check count-attributed assignment groups #11490
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
[DO NOT MERGE][-Wunsafe-buffer-usage] Check count-attributed assignment groups #11490
Conversation
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
4239eee
to
99b91ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
q = p = q; // warn: assignment to bounds-attributed pointer 'p' must be inside of a bounds-attributed group in a compound statement
I feel like the diagnostic message may confuse users. What is a bounds-attributed group
? How about just say
assignment to bounds-attributed pointer 'p' must be a simple statement 'p = RHS;' followed by a simple statement 'count = RHS;'.
When the count expression is a constant, we hide followed by a simple statement 'count = RHS;'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just came up with some questions after reading the description.
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
}
Will out_p = ... ;
be warned about?
void foo(int *__counted_by(a + b) p, int a, int b) {
p = ...;
p = ...; // duplicated
a = ...;
// b missing
}
So if the pointer is updated, the count variables must also be explicitly updated regardless of whether their values change?
I'll split this PR into multiple parts, so that reviewing is easier and we can have more focused discussion. |
I agree that my initial diagnostic needs rework. I think your diagnostic is better. Though, does |
Yes, you have to update the count explicitly, unless the count is immutable (then you have to omit the assignment). Those are the rules of The reason behind this rule in void foo(int *__counted_by(count) p, int count) {
p = NULL; count = 0;
// ...
p = malloc(100 * sizeof(int));
// count is 0, so the assignment succeeds
// ...
int x = p[42]; // oops, runtime trap, count is 0
} However, this is not the case for our interop, since we always verify the assignment at compile-time, and in our case skipping the assignment would work just fine. Unsure if we should just port this rule from |
This change adds support for checking count-attributed assignment
groups. This commit adds the following checks:
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:
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:
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 ofb
before the later update, which can lead to OOB if
b
was less than42:
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