Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments

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

On 23/05/23 16:43, Sergey Bugaev wrote:
> On Tue, May 23, 2023 at 10:09 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>> +# ifndef __USE_FILE_OFFSET64
>>> +/* Just fcntl ().  */
>>
>> Does this comment adds anything?  Also I think we avoid to use '()' on function
>> definition on comments.
> 
> These comments were mainly for myself to keep track of the various
> possible configurations -- I decided to leave them in since the reader
> of the code might get a little overwhelmed by the combinatorial
> explosion of the possible configuration variants as well.
> 
> I can remove these comments (or just this one?) if you would prefer that.

I am chiming in because it is an installed header, but this should be ok.

> 
>>> +__extern_always_inline int
>>
>> Should we use  __fortify_function also for this case?
> 
> I think __fortify_function is supposed to be used on the replacement
> functions themselves, not their helpers? if that's not the case, sure.

The __fortify_function mainly adds the artificial attribute, which improves
the error message to make on the call site instead of the decorated function.
If the error messages from wrong fnctl usage are ok, I think we can skip
the __fortify_function usage here.

> 
>>> +#ifdef __F_GETOWN_EX
>>> +    case __F_GETOWN_EX:
>>> +    case __F_SETOWN_EX:
>>> +    case __F_SETSIG:
>>> +#endif
>>
>> I think it would be better to condicionalize using __USE_GNU:
>>
>> #ifdef __USE_GNU
>>     case F_GETOWN_EX:
>>     case F_SETOWN_EX:
>>     case F_SETSIG:
>> #endif
>>
>> Since the double underscore symbols are implementation defined one and should
>> not be used by the application.
> 
> But that would break on the Hurd, no?
> 
> I could do #ifdef F_GETOWN_EX (without the double underscore), but I
> wanted to handle __F_GETOWN_EX etc. even if the user has not enabled
> them (better handle them than not). In other words, this ifdef is
> meant to check for Hurd vs Linux, not for __USE_GNU vs not.

But user will actually use F_GETOWN_EX, not __F_GETOWN_EX; so I think 
check for F_GETOWN_EX define as well.

> 
>> Should be worried that since we don't have const expressions like newer C++
>> that large switch might ended being extra code on every fcntl call? I think
>> the __builtin_constant_p check below gives compiler enough information to
>> avoid it, but I am not sure.
> 
> Yes, __builtin_constant_p is, AFAIK, some compiler magic that
> "guarantees" (quoted since I'm not sure if it's actually guaranteed,
> but I've never seen it behave otherwise) that in the true branch the
> value is treated as a compile-time constant.
> 
> In practice (see what the tst-fortify compiles to!), with -O2 this
> whole thing is fully optimized away, and the generated code contains
> the very same calls to fcntl (or fcntl64, or whatever) that it would
> if there never was any fortification -- except you get compile-time
> errors if you forget the argument when it's required. So there's zero
> run-time cost.
> 
> If the cmd argument is a c-t const, that is -- otherwise it calls the
> _2 versions and does the check at runtime, yes. But that's only extra
> code (as in code size) once, in glibc, not at the call sites.

Fair enough.

> 
>>> +      if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1)
>>
>> No implicit check for function that do not return bool:
>>
>>   if (__fcntl_cmd_needs_arg (__cmd) == 1 ...)
> 
> Why? Is it just a code style thing?

Yes, it is from the glibc code style (check 'Boolean Coercions' [1]).

[1] https://sourceware.org/glibc/wiki/Style_and_Conventions

> 
>> I think we need also to handle __USE_TIME_BITS64 and add another fcntl debug
>> symbol to handle; even though for Linux now __fcntl_time64 is just a alias
>> to __libc_fcntl64.
>>
>> It is because we even need to add a proper __fcntl_time64 implementation,
>> the redirection now does:
>>
>>   fcntl -> __fcntl64_2 -> __libc_fcntl64
>>
>> Where it should be something like:
>>
>>   fcntl -> __fcntl_time64_2 -> __fcntl_time64
> 
> The idea here was that the 2-argument version of fcntl clearly does
> not deal with time, either 32- or 64-bit...

And that's why we don't have a __fcntl_time64 implementation.  But Florian
has asked to add one anyway if we even eventually need to handle some time
related structure.

I think we can skip this for now, but at least add a comment stating that
if we even need to handle time related field with fcntl we will a new
redirection.