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

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 29/05/23 16:57, Sergey Bugaev wrote:
> 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.

Now that you brought Rust black_box, we already something similar on
benchtests: DO_NOT_OPTIMIZE_OUT.

> 
>> 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.
> 

The LFS names are not considered a namespace pollution, so I think that's
why it always provided (just check tst-fortify.c LFS name usage, like
pread64).

>> 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.

Yeah, I forgot __fcntl_alias is in fact ... an alias.  So in fact there is no
issue here.