Thanks for putting up a clear RFC. Unfortunately from the perspective of -fbounds-safety we for see some problems with the proposed design. I collaborated with @fay59 @dan_liew to compose the response.
Taking the address of a count variable is forbidden
In -fbounds-safety we have forbidden taking the address of the count variable, unless it’s used as a function out parameter. Here’s an example:
struct Foo {
int count; // "count variable"
char* __counted_by(count) buf;
};
void setter(int* count, char* __counted_by(*count) * buf) {
*count = 0;
*buf = 0;
}
void test(struct Foo* f) {
int* count_ptr = &f->count; // Error
setter(&f->count, &f->buf); // Allowed
}
The reason we have disallowed taking the address of the count variable is that it would allow for assignments to made to the count variable that would bypass checks we enforce on the __counted_by pointer and corresponding count.
In -fbounds-safety we enforce that both the pointer and count are always updated together and some additional checks to ensure an update to the count doesn’t create a partially out-of-bounds pointer.
void test1(struct Foo* f) {
f->count = 3; // error: requires corresponding assignment to f->buf
}
void test2(struct Foo* f) {
f->buf = f->buf;
f->count++; // error: incrementing 'count' without updating 'buf' always traps
}
void test3(struct Foo* f, int size) {
// run-time check inserted
// if (size < 100) trap();
f->buf = malloc(size);
f->count = 100;
}
These checks are in place to ensure that the pointer and count stay correct through the entire lifetime of the pointer.
The same applies to pointer to flexible array members. Consider the following example:
struct Foo {
int count; // "count variable"
char fam[__counted_by(count)];
};
void test(void) {
p->count = 10; // Error
}
void test2(void) {
// Ok. Run-time checks are inserted to ensure the correctness of counted_by info
struct Foo *p = alloc(offsetof(struct Foo, fam) + 10);
p->count = 10;
}
void test3(struct Foo *p) {
int *count_p = &p->count; // Error
*count_p = SIZE_MAX; // Without the above error, count would've been updated without checks
}
To update the count value used by the __counted_by on a flexible array member, it must be immediately after the pointer is updated. If we allow creating a pointer to the count, the count can be updated through the pointer bypassing the necessary checks for __counted_by .
The proposal in this RFC is problematic because __builtin_get_counted_by is providing an escape hatch by returning a pointer to the count variable. With that pointer the count can easily be changed to be incorrect.
We’ve also clarified the restriction for counted_by in [BoundsSafety][NFC] Specify taking address of a variable referred to by '__counted_by' is forbidden by rapidsna · Pull Request #106147 · llvm/llvm-project · GitHub.
The count expression isn’t always a simple decl
In -fbounds-safety the argument to the counted_by attribute isn’t restricted to referring to a single decl. It can refer to multiple decls and include arbitrary arithmetic. Here are some examples:
// The count expression refers to two decls and performs arithmetic
void __sized_by(size*count) calloc(size_t size, size_t count);
#define BUF_SIZE 24
struct BufferHandle {
// The count expression is a constant
char* __counted_by(BUF_SIZE) buf;
};
The proposal says:
If the flexible array member doesn’t have the
counted_byattribute, the builtin returns(void *)0.
This is problematic for counted_by expressions that contain arithmetic and/or multiple variables because there is no address to return that contains a count.
-fbounds-safety compatible proposal
Instead we propose __builtin_bounds_attr_arg(P) , which behaves as a macro define to substitute itself with the spelling of the __counted_by argument (with the necessary member access prefix if applicable). This means the builtin can return a value even if the count expressions refers to multiple variables or has arithmetic. The builtin can still be used for assignment, if the __counted_by argument is a simple reference of a field or a variable name. The assignment will naturally result in an error if the __counted_by has a complex or non-assignable expression (e.g., count + 1 , count * size , etc.). We chose the name __builtin_bounds_attr_arg to be generally applicable for other bounds annotations such as __counted_by_or_null , __sized_by , __sized_by_or_null , __ended_by .
Please note that support for __counted_by_or_null , __sized_by , and __sized_by_or_null (not yet __ended_by ) was landed in [Bounds-Safety] Add sized_by, counted_by_or_null & sized_by_or_null by hnrklssn · Pull Request #93231 · llvm/llvm-project · GitHub.
Here are a few examples:
#define BUF_SIZE 24
struct BufferHandle {
// The count expression is a constant
char* __counted_by(BUF_SIZE) buf;
};
struct Foo {
int count;
char* __counted_by(count) buf;
};
struct Image {
int width;
int height;
char* __counted_by(width*height) buf;
};
void test(struct Foo* f, struct* BufferHandle bh, struct Image* i) {
--__builtin_bounds_attr_arg(f->buf); // substituted to: --f->count
f->buf = f->buf;
__builtin_bounds_attr_arg(f->buf) = 0; // substituted to: f->count = 0
f->buf = 0x0;
printf("%p has count %d\n", f->buf, __builtin_bounds_attr_arg(f->buf)); // substituted to: f->count
// __builtin_bounds_attr_arg(bh->buf) returns 24
printf("%p has count %d\n", bh->buf, __builtin_bounds_attr_arg(bh->buf)); // substituted to: BUF_SIZE
__builtin_bounds_attr_arg(bh->buf) = 0x0; // 24 = 0x0; error: can't assign to a constant
__builtin_bounds_attr_arg(i->buf) = 0x0; // i->width * i->height = 0; error: expression is not assignable
i->buf = 0x0;
// __builtin_bounds_attr_arg(i->buf) returns i->width*i->height
printf("%p has count %d\n", i->buf, __builtin_bounds_attr_arg(i->buf)); // ok
}
Since the compiler reports an error for invalid use of __builtin_bounds_attr_arg , we need a way to skip the evaluation of the builtin to support the use cases like in the alloc macro example. In the example, we suggest to use __builtin_choose_expr to skip the assignment if the attribute doesn’t exist.
#define alloc(P, FAM, COUNT) ({ \
size_t __size = sizeof(*P) + sizeof(*P->FAM) * COUNT; \
typeof(P) __p = kmalloc(__size, GFP); \
__builtin_choose_expr(
__builtin_has_attribute(__p->FAM, "counted_by"), \
__builtin_bounds_attr_arg(__p->FAM) = COUNT, \
(void)
);
__p;
})
As Bill mentioned in RFC, this requires support for __builtin_has_attribute in Clang and we propose to introduce it in Clang.
Alternative approach
Another potential alternative is to make __builtin_bounds_attr_arg to return a pointer to count as proposed, but prevent it from being assigned to another variable or passed as an argument. Here’s the example:
void test(struct foo *p) {
int *count = __builtin_bounds_attr_arg(p->fam); // error: cannot pass a pointer to count to another variable
foo(__builtin_bounds_attr_arg(p->fam)); // error: cannot pass a pointer to count as an argument
}
However, this doesn’t solve the problem that the builtin cannot be used to return a count expression, if the expression is not a simple decl reference, so we cannot create a pointer to it (e.g., arithmetic expression, constant value).