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

[Sergey Bugaev <bugaevc@gmail.com>]

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

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

> > +#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?

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.

__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?

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

Sergey