https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116016
--- Comment #59 from Kees Cook <kees at outflux dot net> --- (In reply to Bill Wendling from comment #57) > (In reply to Kees Cook from comment #47) > > So, with the builtin being used within the allocator to set counter, now the > > old code pattern still works (as counter has been initialized early enough), > > and the duplicate store to p->counter doesn't matter (and may even get > > optimized away). > > I get that. My question then becomes why a diagnostic isn't sufficient to > get programmers to "do the right thing" (similar to the "uninitialized > variable" diagnostic). Cases where setting 'count' isn't local and/or is > obfuscated are "easily" handled by simply adding the assignment directly > after the allocation,because that's what this solution is doing. A diagnostic only works in the simple cases, and the simple cases can already be trivially located when applying 'counted_by' in the first place. I think using the "uninitialized variable" example is, actually, a perfect mapping for this. After decades of trying to get uninit warnings working, it was never complete, and there continued to be endless uninit flaws in Linux. The introduction of -ftrivial-auto-var-init=zero finally solved it, allowing for reliable semantics in the face of bugs. I view this identically: the programmer _should_ be setting 'counter' manually, but because the allocation actually _contains_ the correct value to start with, better to help the human. We want to shift the burden of correctness into the toolchain as much as possible, especially for potential memory safety issues. > I'm pushing back so much for a couple of reasons: > > 1. Updating current Linux to use 'counted_by' already forces you to fix the > bad code patterns, because of minimum compiler versions. So by the time > Linux's minimal compiler versions support the 'counted_by' feature, this > builtin won't have as much of an impact. We can't fix all of them correctly (as we've already seen in Linux). But having the builtin exactly maps to the cases where enforcing the new 'counted_by' would break a program that got the initialization wrong, and allows use to get the initialization correct "automatically" (via the updated allocator). Once the compiler minimum version is updated, the open-coded assignments of 'counter' will no longer be needed, and can be removed globally, leaving the centralized assignment in the allocator. > 2. I believe a good warning would be more effective. It forces the > programmer to get the initialization sequence "correct" and doesn't hide the > initialization of one of the fields. In GCC we have already encountered the uninit warning being thrown in simple cases. But we have evidence to show that a warning will never give complete coverage. > 3. Having the allocator initialize a field (beyond zeroing out memory) is > unexpected in C. I know this happens in C++, but initialing a class member > happens with non-default c'tors that are explicitly called by the > programmer. The initialization of the 'count' field will be "hidden" by > macros which eventually calls some kind of 'kmalloc' which before the > 'counted_by' feature didn't pre-set programmer-defined fields. It's certainly novel, but so is 'counted_by'. Having a fixed-sized array is very common, and C programmers already depend on `sizeof` and `__builtin_object_size` to get those array sizes, and allocations get sized correctly, etc. Adding 'counted_by' means we gain similar semantics for dynamically sized arrays, so it's not much of a stretch for C to have the size "preinitialized". And that said, the Linux kernel is very much a specialized C "dialect", that does all kinds of "unexpected" things. It has been gaining more and more "modern language features" over the years, so this is, again, not strange for Linux. Additionally, moving to a common allocator is actively under way in Linux too, as the other slab variants have been removed, and there is now a single heap allocator. > 4. The utility of this builtin seems very limited to this anti-pattern. > There aren't any savings to using '__builtin_get_counted_by(ptr->FAM) = val' > rather than 'ptr->count = val' in any other situation. (I understand *why* > you need the builtin, because of the difficulty finding the 'count' field > during parsing in a generic way.) I don't disagree there, but I can't find any other way to handle this in a clean way in Linux to have a common allocator method for all structs, whether or not they have 'counted_by' applied. > 5. I'm not sure if this builtin will be used outside of Linux, unless a > project has a similar allocation method as Linux. I can't say for sure either, but I do need it for Linux, and that is sufficient in my opinion. In the past, many other projects will follow Linux's lead, though. Many new compiler features added lately get picked up by systemd, for example. > Personally, I like having the programmer think about their code rather than > simply slapping on the 'counted_by' attribute and calling it done. Sure it's > more convenient to set this field in the allocator, but the attribute does > so much under the covers and the requirements placed on the 'count' value > are very strict. The programmer needs to audit their code to ensure it > follows those requirements. I get that, but I'd rather they _not_ have to think about it: they already provided an array size to the allocator and declared the 'counted_by' association. Why should they be forced to repeat redundant information?
