On Mon, Jun 10, 2024 at 02:46:09PM -0700, Kees Cook wrote: > > I really detest this thing because it makes what was trivially readable > > into something opaque. Get me that type qualifier that traps on overflow > > and write plain C. All this __builtin_overflow garbage is just that, > > unreadable nonsense. > > It's more readable than container_of(),
Yeah, no. container_of() is absolutely trivial and very readable. container_of_const() a lot less so. (one static_assert() removed) #define container_of(ptr, type, member) ({ \ void *__mptr = (void *)(ptr); \ ((type *)(__mptr - offsetof(type, member))); }) Which is very clear indeed in what it does. Compare with: #define struct_size(p, member, count) \ __builtin_choose_expr(__is_constexpr(count), \ sizeof(*(p)) + flex_array_size(p, member, count), \ size_add(sizeof(*(p)), flex_array_size(p, member, count))) And I still have no idea :-( > IMO. "give me the struct size > for variable VAR, which has a flexible array MEMBER, when we have COUNT > many of them": struct_size(VAR, MEMBER, COUNT). It's more readable, more > robust, and provides saturation in the face of potential wrap-around. I'm sure you know what it does. Thing is, I don't care because I can trivially write it myself and not have to care and I'll have forgotten all about it the moment I sent this email. It just doesn't make sense to wrap something as utterly trivial as: size = sizeof(*p) + num*sizeof(p->foo); We're going to have to agree to disagree on this. Note how I naturally get the order wrong? [[ There is the whole FMA angle to this, that is, fundamentally this is a multiply-accumulate, but the problem there is the same that I noted, there is no fixed order, a+b*c and a*b+c are both very common definitions -- although I lean towards the latter being the correct one, given the order in the naming. I suppose this is a long winded way of saying that: #define struct_size(p, member, num) \ mult_add_no_overflow(num, sizeof(p->member), sizeof(*p)) would be *FAR* more readable. And then I still think struct_size() is less readable than its expansion. ]] > > > This provides __counted_by coverage, and I think this is important to > > > gain in ever place we can. Given that this is part of a ring buffer > > > implementation that is arbitrarily sized, this is exactly the kind of > > > place I'd like to see __counted_by used. This is a runtime robustness > > > improvement, so I don't see this a "churn" at all. > > > > Again, mixed in with that other crap. Anyway, remind me wth this > > __counted_by thing actually does? > > It provides annotation for the compiler to perform run-time bounds > checking on dynamically sized arrays. i.e. CONFIG_FORTIFY_SOURCE and > CONFIG_UBSAN_BOUNDS can actually reason about annotated flexible arrays > instead of just saying "oh no a flexible array, I give up". Some day I'll have to look at this FORTIFY_SOURCE and see what it actually does I suppose :/ > > > Peter, for patches 1 and 3, if you'd prefer not to carry them, I could > > > put them in the hardening tree to keep them out of your way. It seems > > > clear you don't want patch 2 at all. > > > > I prefer to not have struct_size() anywhere at all. Please just write > > readable code. > > That ship has sailed, and it has been keeping things at bay for a while > now. As we make progress on making the compiler able to do this more > naturally, we can work on replacing struct_size(), but it's in use > globally and it's useful both for catching runtime mistakes and for > catching compile-time mistakes (the flexible array has to match the > variable's struct). I coulnd't quickly find a single instance in the code I care about. So nothing is sailing afaict.