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

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

On 29/05/23 14:31, Sergey Bugaev wrote:
> Hello,
> 
> On Mon, May 29, 2023 at 7:54 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>> Some comments below.
> 
> thank you for the review :)
> 
>>> +/* Return VALUE, but do it in a way that the compiler cannot
>>> +   see that it's a compile-time constant.  */
>>> +static int __attribute_noinline__
>>> +hide_constant (int value)
>>> +{
>>> +  volatile int v = value;
>>> +  return v;
>>> +}
>>> +
>>
>> Interesting construct, but I wonder if 'volatile' is really required here.
>> Afaiu what really prevents compiler to return fortify compiler warning here
>> is the __attribute_noinline__.
> 
> 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.

> 
>>> +  struct flock flock;
>>
>> Maybe move the flock struct to within the block for where ofd_locks_supported
>> is true?
> 
> It's used in the following if (ofd_locks_supported) block -- the one
> checking for non-compile-time-const cmd -- too.

Yes, but the idea is to avoid the stack allocated variable to escape its
specific usage (Florian is the one that constantly nag about it).

> 
>>> +#if defined (__USE_LARGEFILE64) || defined (__USE_TIME_BITS64)
>>
>> There is no need to replicate the tests for the LFS support, this is already
>> done by tst-fortify-cc-lfs-X tests.
> 
> Hm, I don't think I understand your point, could you please expand?

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

> 
> The -lfs- tests are the ones that enable -D _FILE_OFFSET_BITS=64,
> right? Here, I'm testing that if __USE_LARGEFILE64 is enabled (i.e. -D
> _LARGEFILE64_SOURCE), which means fcntl64 () is available as a
> separate function, the fcntl64 fortification also behaves as expected.
> It's not enough to just test that fcntl () works as expected in 64-bit
> mode, we need to test that fcntl64 () also works as expected. These
> are separate functions (perhaps aliases of each other, but separate
> fortified macros for sure) that may or may not both work with 64-bit
> types.

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.  For non macro calls, pread for instance,
we fix this by using the __glibc_fortify version:

# ifndef __USE_FILE_OFFSET64
__fortify_function __wur ssize_t
pread (int __fd, void *__buf, size_t __nbytes, __off_t __offset)
{
  return __glibc_fortify (pread, __nbytes, sizeof (char),
                          __glibc_objsize0 (__buf),
                          __fd, __buf, __nbytes, __offset);
}
# else
__fortify_function __wur ssize_t
pread (int __fd, void *__buf, size_t __nbytes, __off64_t __offset)
{
  return __glibc_fortify (pread64, __nbytes, sizeof (char),
                          __glibc_objsize0 (__buf),
                          __fd, __buf, __nbytes, __offset);
}
# endif

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

#ifdef __USE_LARGEFILE64
# define fcntl64(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__, 1)                            \
             ? __fcntl64_alias (fd, cmd, __VA_ARGS__)                         \
             : __fcntl64_warn (fd, cmd, __VA_ARGS__)))
#endif

> 
> __USE_LARGEFILE64 and __USE_FILE_OFFSET64 are orthogonal in my
> understanding, aren't they? __USE_LARGEFILE64 means that xxxx64 APIs
> are declared as a separate set of APIs that explicitly work with
> 64-bit types; __USE_FILE_OFFSET64 means that the regular APIs (xxxx,
> not xxxx64) are declared to already use 64-bit. You can have neither
> enabled, or one of them enabled, or both. No?
> 
>>> +template<typename, typename>
>>> +struct __fcntl_types_compatible_helper
>>
>> This makes the C++ tests fail with GCC 13:
>>
>> [...]
>> ../include/bits/../../io/bits/fcntl2.h:460:1: error: template with C linkage
>>   460 | template<typename __T>
>>       | ^~~~~~~~
>> ../misc/sys/cdefs.h:140:25: note: ‘extern "C"’ linkage started here
>>   140 | # define __BEGIN_DECLS  extern "C" {
> 
> Interesting. Any idea why this wasn't failing in my testing?

My guess is because template with extern "C" are not really supported in all
configurations.

> 
>> You will will need to include bits/fcntl2.h after the __END_DECLS, and add
>> __BEGIN_DECLS/__END_DECLS on fcntl2.h.  Something like this on top this
>> patch
> 
> Right, makes sense, thank you.
> 
>> I think we will need to enable fcntl fortify only for gcc 8 or higher, since
>> __VA_OPT__ is not support on gcc 7.
> 
> So maybe it is a good idea to move this all into fcntl3.h like it was
> done in Florian's patch? Then I'd include that after __END_DECLS and
> only if GCC >= 8.

Yes, it would be better indeed.