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

[Florian Weimer <fweimer@redhat.com>]

* Sergey Bugaev:

> On Wed, May 24, 2023 at 11:29 AM Florian Weimer <fweimer@redhat.com> wrote:>
>> I'm attaching my broken patch.  It's based on commit ef4f97648dc9584
>> (from 2016).
>
> Thank you, that's helpful.
>
> A couple more issues with your patch (I understand that it's WIP and broken):
>
> 1. You're doing
>
> int __fcntl_chk (int, int, ...);
> int __REDIRECT (__fcntl_int, (int, int, int __arg), __fcntl_chk);
>
> This is not OK, the ABI may be different between vararg- and
> non-vararg functions.

Right, I planned to make __fcntl_chk non-variadic in its implementation.
We unconditionally read the argument already.

Variadic calls to non-variadic functions are okay for our ABIs, we need
that for K&R compatibility anyway.

> 2. It doesn't do the runtime check if called with 2 args and
> non-c-t-const cmd.

That's possible, I don't remember.

>> IIRC, it doesn't quite work because __builtin_choose_expression only
>> suppresses errors, but not warnings, in the branch that wasn't chosen. 8-(
>>
>> Maybe this is something that could be fixed with _Generic, using
>> __builtin_choose_expression for the __fcntl_is_void check only.
>
> Yes -- I've sketched something up using _Generic and it appears to
> work great: https://godbolt.org/z/8zdzo3T5Y
>
> It does do preprocessor trickery ("friendship ended with
> __builtin_va_arg_pack_len, now __VA_OPT__ is my best friend") and does
> not use __builtin_choose_expr at all, so should be C++-compatible too
> (try with -xc++).

Right, it doesn't look too bad actually.

> This, too, is obviously a prototype, and does not do runtime _2
> checking nor 64-bit handling and so on.
>
> What do you think? Is this direction worth pursuing?

I think so, yes.

Thanks,
Florian