https://gcc.gnu.org/bugzilla/show_bug.cgi?id=116016
--- Comment #57 from Bill Wendling <isanbard at gmail dot com> --- (In reply to Kees Cook from comment #47) > Yes, the counter must be manually set until Linux minimum compiler versions > are raised to include counted_by support, but this is about making the > transition to using counted_by easier and less prone to bugs. > > If the allocator is setting the counter (via the new builtin), then it will > always be correct for compilers that support counted_by. There will be a > later open-coded assignment as well, but that will be optimized away in most > cases. > > Basically, the situation is that when counted_by is added to a struct, all > allocations must be identified and corrected manually right now to avoid the > sanitizer tripping at runtime. When the allocator can do the setting, this > "bad code pattern" goes away. i.e. using counted_by currently introduces a > new bad code pattern that is only present for compilers that support > counted_by. So if we can solve it in the allocator, the problem goes away. > > e.g. this code pattern is valid today: > > p = alloc(count); > for (i = 0; i < count; i++) > fill(&p->array[i]); > p->counter = count; > > Adding counted_by(counter) to "array" means that suddenly the sanitizer will > trip at "fill". While obvious cases where we need to move "p->counter = > count" to immediate after the alloc can be found and changed at the time > counted_by is added, Linux has a lot of complicated corner cases where > allocation and array usage are distant or obfuscated. > > 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. 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. 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. 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. 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.) 5. I'm not sure if this builtin will be used outside of Linux, unless a project has a similar allocation method as Linux. 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.
