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

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

On 23/05/23 17:24, Sergey Bugaev wrote:
> On Tue, May 23, 2023 at 10:57 PM Adhemerval Zanella Netto
> <adhemerval.zanella@linaro.org> wrote:
>>>>> +      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 don't mean to argue, and surely you know better, and not that I
> care about this], but I read that as saying that implicit
> integer-to-bool conversions are frowned upon, not ints used as bools
> (because C used to lack a bool type). In other words: if you have an
> integer variable that can have many different values and you want to
> check it against 0, you're supposed to write out the != 0 explicitly.
> But if what you have is essentially a boolean, but you use int/1/0
> instead of bool/true/false, then just checking if (my_bool) should be
> fine, and doing != 0 would be awkward -- no?
> 
> I've surely seen glibc code use if (my_bool) without the != 0 when
> my_bool is declared as an int. __libc_enable_secure is one example.

The interger as bool is not the problem, specially on a installed header
that won't be easy to use 'bool' (due namespace pollution, standard 
conformance, etc.).  And sure some usage of implicit conversions might
have slip in review, but I think it should follow the code guide lines
anyway (unless we revise it).

> 
>>> 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.
> 
> Hm, either I'm failing to understand what you're saying (which is
> possible: it is late at night), or maybe I didn't make my point clear:
> 
> if what you're concerned is what the _2 functions end up calling
> (__fcntl_time64 vs __libc_fcntl64), then it does not matter because
> they're only used for 2-argument variant of fcntl, which can not
> possibly deal with any time-related structures (nor any structures),
> exactly because there's no third argument where a structure could be
> passed. The 2-arg variant is basically for simple getters that return
> an int.
> 
> But if something is off (wrt time64 vs not) in the main fortification
> / redirection code, then I surely need to fix that -- then please
> clarify what specific configuration / case you're talking about.

Right, I see your point now.

> Ah, no, I see what you mean: you mean we don't strictly speaking need
> the __fcntl64_2 call immediately following __fcntl_missing_arg.
> 
> I guess we don't -- maybe the corresponding change needs to be done to
> the open* fortification as well then.

Agree.