Re: [PATCH v2 3/3] io: Add FORTIFY_SOURCE check for fcntl arguments

[Sergey Bugaev <bugaevc@gmail.com>]

On Mon, May 29, 2023 at 9:09 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> > It's not really required. Apparently even an 'inline' function call is
> > enough to to disarm __builtin_constant_p [0]; I just threw both
> > volatile and noinline in for good measure.>
> > [0] https://godbolt.org/z/v9569KGTT
>
> It is because afaiu 'volatile' does not really match the semantics of what you
> are trying to do here.  Maybe if you really need the compile to force lvalue
> evaluation, it should use something similar to math_opt_barrier.

So, an asm block that does nothing, but is said to read and modify the
value? Sounds better indeed, and reminds me of how
std::hint::black_box is implemented.

> Nevermind, you do need to check for __USE_LARGEFILE64 symbols.  But
> you can omit the '#if defined", they are always defined.

Uhh that's unfortunate actually. I hoped the test would check the
non-__USE_LARGEFILE64 configuration as well, to catch some
__USE_LARGEFILE64-only definition being used unconditionally, for
example.

> And debugging a bit, it seems that I found another issue. Building
> tst-fortify-c-lfs-1-def.c the post-processed output (-E -dD) for fcntl call is:
>
> res = (0 ? __fcntl_2_inline (0, 2) : !__builtin_constant_p (2) ? __fcntl_alias (0, 2, 0) : (__fcntl_is_int (2) ? __builtin_types_compatible_p (__typeof (0), int) : __fcntl_is_const_uint64_t_ptr (2) ? (__builtin_types_compatible_p (__typeof (0), const __uint64_t *) || __builtin_types_compatible_p (__typeof (0), __uint64_t *)) : __fcntl_is_uint64_t_ptr (2) ? __builtin_types_compatible_p (__typeof (0), __uint64_t *) : __fcntl_is_const_fowner_ex (2) ? (__builtin_types_compatible_p (__typeof (0), const struct f_owner_ex *) || __builtin_types_compatible_p (__typeof (0), struct f_owner_ex *)) : __fcntl_is_fowner_ex (2) ? __builtin_types_compatible_p (__typeof (0), struct f_owner_ex *) : __fcntl_is_const_flock (2, 0) ? (__builtin_types_compatible_p (__typeof (0), const struct flock *) || __builtin_types_compatible_p (__typeof (0), struct flock *)) : __fcntl_is_flock (2, 0) ? __builtin_types_compatible_p (__typeof (0), struct flock *) : __fcntl_is_const_flock64 (2, 0) ? (__builtin_types_compatible_p (__typeof (0), const struct flock64 *) || __builtin_types_compatible_p (__typeof (0), struct flock64 *)) : __fcntl_is_flock64 (2, 0) ? __builtin_types_compatible_p (__typeof (0), struct flock64 *) : 1) ? __fcntl_alias (0, 2, 0) : __fcntl_warn (0, 2, 0));
>
> Which is the non-LFS version.

Hmm, I don't see the issue? And the test passes, doesn't it? :)
(If it passes *despite* the 32-vs-64 confusion, that is a problem in
itself, that'd mean it's not checking what it's supposed to be
checking.)

The fcntl macro will always expand to the same thing (modulo C vs C++
type check differences). But then __fcntl_alias and __fcntl_warn both
reference the same symbol that fcntl would reference had it not been
for the fortification -- which is going to be the 'fcntl64' symbol
with __USE_FILE_OFFSET64.

> So I think this patch should do the same:
>
> #ifndef __USE_FILE_OFFSET64
> # define fcntl(fd, cmd, ...)                                                  \
>   (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)                                \
>    __VA_OPT__ (:                                                              \
>      !__builtin_constant_p (cmd) ? __fcntl_alias (fd, cmd, __VA_ARGS__)       \
>         : __fcntl_type_check (cmd, __VA_ARGS__, 0)                            \
>              ? __fcntl_alias (fd, cmd, __VA_ARGS__)                           \
>              : __fcntl_warn (fd, cmd, __VA_ARGS__)))
> #else
> # define fcntl(fd, cmd, ...)                                                  \
>   (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)                                \
>    __VA_OPT__ (:                                                              \
>      !__builtin_constant_p (cmd) ? __fcntl64_alias (fd, cmd, __VA_ARGS__)      \
>         : __fcntl_type_check (cmd, __VA_ARGS__, 0)                            \
>              ? __fcntl64_alias (fd, cmd, __VA_ARGS__)                         \
>              : __fcntl64_warn (fd, cmd, __VA_ARGS__)))
> #endif

What's the point? -- __fcntl_alias and __fcntl64_alias are supposed to
both reference the same thing (fcntl64 or __fcntl_time64) with
__USE_FILE_OFFSET64. Or more generally: do the same thing as the
non-fortified definition does.

Hope this makes sense.

Sergey