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

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella Netto:

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

The noinline attribute does exactly what it says: it prevents inlining,
but it does not prevent inter-procedural analysis.  So if a function
returns a constrant, the compiler will still use that constant
elsewhere.  Newer GCC versions support the noipa attribute.  If you drop
the static, you can use the weak attribute for compatibility with pretty
much all GCC versions.

But I think using volatile (without noinline for clarity) is fine here.

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

It's possible to use extern "C++" around the template.

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

Even then, we may still have to use extern "C++", to preserve
compatibility with C++ applications which include <fcntl.h> in an extern
"C" block.  See this commit:

commit e37208ce86916af9510ffb9ce7b3c187986f07de
Author: Florian Weimer <fweimer@redhat.com>
Date:   Sat Oct 22 17:33:26 2016 +0200

    math.h: Wrap C++ bits in extern "C++"
    
    It is still common to include system header files in an extern "C"
    block.  This means that exiting <math.h>'s own extern "C" block
    is not sufficient to get back to C++ mode.  Use an extern "C++"
    wrapper instead.

Thanks,
Florian