On Tue, Aug 30, 2022 at 01:53:08PM -0700, Nick Desaulniers wrote:

Two things I noticed in __compiletime_strlen:

Four? :)

1. A temporary, __p, is created+used to avoid repeated side effects from
   multiple evaluation of the macro parameter, but the macro parameter
   was being used accidentally in __builtin_object_size.

__builtin_object_size(), like sizeof() but unlike __builtin_strlen(),
will not evaluate side-effects: https://godbolt.org/z/Yaa1z7YvK
And using bos on __p will sometimes mask the actual object, so p needs to
stay the argument.

2. The temporary has a curious signedness and const-less qualification.
   Just use __auto_type.

__auto_type is pretty rare in the kernel, but does provide the removal
of "const". Even though the kernel builds with -Wno-pointer-sign, the
explicit case does fix a potential warnings about signedness differences,
not just const differences, for __builtin_strlen() which requires "const
char *", but many arguments are "unsigned char *", "u8 *", etc.

Is __auto_type more readable than the explicit cast? It does seem to
work fine.

3. (size_t)-1 is perhaps more readable as -1UL.

That's true, though I kind of prefer (size_t)-1, though yes, it appears
to be the extreme minority in the kernel.

4. __p_size == -1UL when __builtin_object_size can't evaluate the
   object size at compile time. We could just reuse __ret and use one
   less variable here.

This seems to get entire optimized away by the compiler? I think it's
more readable to keep the explicit variable.

-Kees

-- 
Kees Cook