[v2,0/3] fcntl fortification

Hello,

this is the v2. Changes compared to v1:

- The check for whether the kernel supports OFD locks is now in a
  separate libsupport function, support_fcntl_support_ofd_locks ().
- __fcntl64_2 () is gone, a single __fcntl_2 () is used for all the
  cases. This is because the 2-argument version doesn't deal with
  file sizes or times, because of there not being a third argument.
- Addressed misc review comments.
- The big one: it now does argument type checking!

So, let's talk about type checking for a bit. It is *not* implemented
with _Generic (or other type-based dispatch, such as C++ function
overloading). The crucial point here is not failing on custom/unknown
fcntl commands; for such commands we don't want to emit any errors
(either compile-time or runtime) if used in the 2-argument form, and
don't want to emit any type mismatch warnings either. So if we went off
the type like I was initially planning to, we'd have to list all known
commands *incompatible* with the type, and that would be unmaintainable.

So the way this is done is:

1. There are __fcntl_is_xxxx (cmd) inline functions, for example
   __fcntl_is_int (), that only return 1 if the command is known to
   require an argument of the given type. For flock types, there's a
   second argument, __is_fcntl64 -- since the OFD lock commands expect
   a struct flock64 when used in fcntl64, and a plain struct flock in
   plain fcntl -- unlike the non-OFD locks, where e.g. F_SETLK always
   references a struct flock, and F_SETLK64 always a struct flock64.

   Please correct me on this if my understanding is wrong!!

2. There is a __fcntl_types_compatible () macro which is a thin wrapper
   over __builtin_types_compatible_p () in plain C, and uses an
   std::is_same_v-like check (using partial template specialization) in
   C++. Importantly, it uses __typeof () even in C++ (not decltype ()),
   because we don't want the extra references appended to our type. For
   example, we want 'int', not 'const int &' or 'int &&'.

3. There are __fcntl_type_check_xxxx (arg) macros that check that the
   type of arg is compatible with the expected type. For simple cases
   this is only a matter of using __fcntl_types_compatible (arg, xxxx),
   and for those cases these macros only serve to reduce the boilerplate
   a bit. However for the const pointers we also have to check for the
   non-const type version (since __builtin_types_compatible_p () does not
   do this on its own). No attempt is made to check for volatile.

4. There's the __fcntl_type_check (cmd, arg, is_fcntl64) macro that
   evaluates to 0 or 1 depending on whether the types match. It consists
   of a chain of checks like this one:

   __fcntl_is_int (cmd) ? __fcntl_type_check_int (arg) :

   and terminates with a 1, so any unrecognized command always passes
   the type check. To avoid tons and tons of nested parenthesis, the
   implementation replies on the precedence of the ternary operator in
   C: specifically, it works just like an if / else if / else chain, so
   you can have

   __fcntl_is_foo (cmd) ? __fcntl_type_check_foo (arg) :
   __fcntl_is_bar (cmd) ? __fcntl_type_check_bar (arg) :

   and so on. I would not normally do this (better add explicit parens
   to make it clearer to the humans reading the code), but in this case
   it is more clear than adding many, many parens.

5. Here's the fcntl () macro in all of its horrible glory:

   #define fcntl(fd, cmd, ...)
     (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)
      __VA_OPT__ (:
        !__builtin_constant_p (cmd) ? __fcntl_alias (fd, cmd, __VA_ARGS__)
           : __fcntl_type_check (cmd, __VA_ARGS__, 0)
                ? __fcntl_alias (fd, cmd, __VA_ARGS__)
                : __fcntl_warn (fd, cmd, __VA_ARGS__)))

   First, the __VA_OPT__ () trick: if there only are two arguments, this
   expands to __fcntl_2_inline (fd, cmd), and that's it. Otherwise it
   expands to 0 ? __fcntl_2_inline (fd, cmd) : (the rest), and (the rest)
   is where the 3-argument logic is implemented. If cmd is not a
   compile-time const, it just forwards to __fcntl_alias (), otherwise
   it does the __fcntl_type_check (), and forwards to __fcntl_alias ()
   or __fcntl_warn () depending on the result.

6. __fcntl_warn () is basically the same as __fcntl_alias (), except
   it's defined with __warnattr. So you get a warning (not a hard error)
   on type mismatch. This is in line with how pointer type mismatch is
   handled elsewhere in C / GCC. You can of course escalate this to an
   error with -Werror if you want to. Forgeting an argument when it's
   required is still a hard error (__errordecl).

   To make the warning (from __warnattr) to show up when building
   against fcntl2.h as a "system header", I had to wrap it in a
   "pragma gcc diagnostic"; please see the patch for a longer
   explanation.

Here's what it looks like in practice:

Forgetting a required argument:

------------------------------------------------------------------
In file included from /usr/include/fcntl.h:342,
                 from demo.c:3:
In function ‘__fcntl_2_inline’,
    inlined from ‘main’ at demo.c:6:10:
/usr/include/bits/fcntl2.h:541:5: error: call to ‘__fcntl_missing_arg’ declared with attribute error: fcntl with with this command needs 3 arguments
  541 |     __fcntl_missing_arg ();
      |     ^~~~~~~~~~~~~~~~~~~~~~
------------------------------------------------------------------

Type mismatch:

------------------------------------------------------------------
In file included from /usr/include/fcntl.h:342,
                 from demo.c:3:
demo.c: In function ‘main’:
demo.c:7:10: warning: call to ‘__fcntl_warn’ declared with attribute warning: fcntl argument has wrong type for this command [-Wattribute-warning]
    7 |   return fcntl (0, F_DUPFD, ptr);
      |          ^~~~~
------------------------------------------------------------------

I have made no attempt to guard all of this with a compiler version
prereq! This seems appropriate because it's not using _Generic, but
maybe it's still worth doing. If it is, please tell me what the correct
check would be and I'll put it in.

I have only really tested with (modern) GCC. I briefly checked with
Clang, but the fortification doesn't seem to get enabled at all; perhaps
it's failing some other check.

Sergey

* Sergey Bugaev via Libc-alpha:

> 2. There is a __fcntl_types_compatible () macro which is a thin wrapper
>    over __builtin_types_compatible_p () in plain C, and uses an
>    std::is_same_v-like check (using partial template specialization) in
>    C++. Importantly, it uses __typeof () even in C++ (not decltype ()),
>    because we don't want the extra references appended to our type. For
>    example, we want 'int', not 'const int &' or 'int &&'.

I think you should avoid using __typeof, otherwise we need to add
another GCC check.  If you need to use decltype, you'll have to add a
__cplusplus version check.

> 5. Here's the fcntl () macro in all of its horrible glory:
>
>    #define fcntl(fd, cmd, ...)
>      (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)

I think we should avoid the new __fcntl_2 symbol because it an
unnecessary optimization.

> 6. __fcntl_warn () is basically the same as __fcntl_alias (), except
>    it's defined with __warnattr. So you get a warning (not a hard error)
>    on type mismatch. This is in line with how pointer type mismatch is
>    handled elsewhere in C / GCC. You can of course escalate this to an
>    error with -Werror if you want to. Forgeting an argument when it's
>    required is still a hard error (__errordecl).

It would very nice if we could generate the appropriate warning for C
(-Wincompatible-pointer-types).  This is what I tried to do, but it
might actually be impossible.

Should we generate errors for C++?  It requires compatible pointer
types, after all.

> I have only really tested with (modern) GCC. I briefly checked with
> Clang, but the fortification doesn't seem to get enabled at all; perhaps
> it's failing some other check.

Which Clang version?

Siddhesh, maybe you could look at this?

Thanks,
Florian

On Tue, May 30, 2023 at 11:09 AM Florian Weimer <fweimer@redhat.com> wrote:
> > 2. There is a __fcntl_types_compatible () macro which is a thin wrapper
> >    over __builtin_types_compatible_p () in plain C, and uses an
> >    std::is_same_v-like check (using partial template specialization) in
> >    C++. Importantly, it uses __typeof () even in C++ (not decltype ()),
> >    because we don't want the extra references appended to our type. For
> >    example, we want 'int', not 'const int &' or 'int &&'.
>
> I think you should avoid using __typeof, otherwise we need to add
> another GCC check.  If you need to use decltype, you'll have to add a
> __cplusplus version check.

I don't think I understand your point about the GCC check, could you
please expand?

__typeof seems to have already been supported in very ancient GCC
versions [0], certainly much earlier than __VA_OPT__ support has
appeared. Clang supports typeof too.

[0]: https://gcc.gnu.org/onlinedocs/gcc-2.95.3/gcc_4.html#SEC68

> > 5. Here's the fcntl () macro in all of its horrible glory:
> >
> >    #define fcntl(fd, cmd, ...)
> >      (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)
>
> I think we should avoid the new __fcntl_2 symbol because it an
> unnecessary optimization.

And again I don't think I understand your point :|
Could you please expand here as well? What's the (unnecessary)
optimization here?

__fcntl_2 is required to do runtime checking of whether the runtime
value of CMD indeed does not require a third argument.
__fcntl_2_inline does the check at compile time if possible, and
either emits an error or calls the __fcntl_alias, otherwise it calls
through to the __fcntl_2. This all could be done inside the fcntl
macro as well, I just chose to move it to the inline function because:

- this is way more readable, the macros are unreadable enough already;
- this way it can be shared between fcntl and fcntl64;
- it *is* possible to do this part in a function rather than a macro,
  unlike everything else, because it doesn't require us to pass through
  the untyped 'arg'.

> It would very nice if we could generate the appropriate warning for C
> (-Wincompatible-pointer-types).  This is what I tried to do, but it
> might actually be impossible.
>
> Should we generate errors for C++?  It requires compatible pointer
> types, after all.

So the way I think about this, what this is doing is not making fcntl
into a proper type-safe overloaded function, but adding some
safeguards on top of the existing vararg definition that catch some
mistakes.

The diagnostics emitted are different, and this is fine. Another
instance of this: you get the "error: call to '__fcntl_missing_arg'
declared with attribute error" and not the error you would otherwise
get from GCC upon forgetting a required function argument ("error: too
few arguments to function 'foo'").

I would prefer to keep type mismatch a warning in C++ too (because
this is best-effort additional safeguards, not a real type system),
but this would be easy to change if you want me to.

> > I have only really tested with (modern) GCC. I briefly checked with
> > Clang, but the fortification doesn't seem to get enabled at all; perhaps
> > it's failing some other check.
>
> Which Clang version?

$ clang --version
clang version 16.0.4 (Fedora 16.0.4-1.fc38)

I now checked again, and what's happening is Clang doesn't seem to
have __builtin_va_arg_pack -- which is no longer required for the
fcntl fortification, so this would be resolved once I move it to a
separate fcntl3.h header.

Sergey

* Sergey Bugaev:

> On Tue, May 30, 2023 at 11:09 AM Florian Weimer <fweimer@redhat.com> wrote:
>> > 2. There is a __fcntl_types_compatible () macro which is a thin wrapper
>> >    over __builtin_types_compatible_p () in plain C, and uses an
>> >    std::is_same_v-like check (using partial template specialization) in
>> >    C++. Importantly, it uses __typeof () even in C++ (not decltype ()),
>> >    because we don't want the extra references appended to our type. For
>> >    example, we want 'int', not 'const int &' or 'int &&'.
>>
>> I think you should avoid using __typeof, otherwise we need to add
>> another GCC check.  If you need to use decltype, you'll have to add a
>> __cplusplus version check.
>
> I don't think I understand your point about the GCC check, could you
> please expand?

Ahh, maybe that check is implied by doing this for fortification only?

>> > 5. Here's the fcntl () macro in all of its horrible glory:
>> >
>> >    #define fcntl(fd, cmd, ...)
>> >      (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)
>>
>> I think we should avoid the new __fcntl_2 symbol because it an
>> unnecessary optimization.
>
> And again I don't think I understand your point :|
> Could you please expand here as well? What's the (unnecessary)
> optimization here?
>
> __fcntl_2 is required to do runtime checking of whether the runtime
> value of CMD indeed does not require a third argument.

Oh, I'm not sure if the run-time check is really that useful.

There's no vfcntl function, so I expect that we will have accurate type
information at the callsite in most cases, and the compile-time check
works.

>> It would very nice if we could generate the appropriate warning for C
>> (-Wincompatible-pointer-types).  This is what I tried to do, but it
>> might actually be impossible.
>>
>> Should we generate errors for C++?  It requires compatible pointer
>> types, after all.
>
> So the way I think about this, what this is doing is not making fcntl
> into a proper type-safe overloaded function, but adding some
> safeguards on top of the existing vararg definition that catch some
> mistakes.
>
> The diagnostics emitted are different, and this is fine. Another
> instance of this: you get the "error: call to '__fcntl_missing_arg'
> declared with attribute error" and not the error you would otherwise
> get from GCC upon forgetting a required function argument ("error: too
> few arguments to function 'foo'").
>
> I would prefer to keep type mismatch a warning in C++ too (because
> this is best-effort additional safeguards, not a real type system),
> but this would be easy to change if you want me to.

Hmm.  I certainly don't see this as a blocker, what you are proposing is
way better than what we have today.

Thanks,
Florian

On Tue, May 30, 2023 at 2:08 PM Florian Weimer <fweimer@redhat.com> wrote:
> Ahh, maybe that check is implied by doing this for fortification only?

All of this is only happening within the fortification header, yes,
that does not even get #included if the preconditions for including it
fail. The preconditions are currently the same as for the open ()
fortification due to sharing the same header file, but they are going
to be different (in v3) because this now needs __VA_OPT__ and __typeof
but does not need __builtin_va_arg_pack.

> Oh, I'm not sure if the run-time check is really that useful.
>
> There's no vfcntl function, so I expect that we will have accurate type
> information at the callsite in most cases, and the compile-time check
> works.

I see. Well, I copied what the open () fortification was doing, and I
see that many other fortifications have a runtime-checked version in
addition to compile-time checks.

There is no vopen either, but it's not hard to imagine someone doing

open (path, O_WRITE | O_CREAT | (cloexec ? O_CLOEXEC : 0))

and similarly

fcntl (fd, cloexec ? F_DUPFD_CLOEXEC : F_DUPFD)

in both cases __builtin_constant_p will be false, and the user will
miss out on the fortification, and won't notice they forgot the
required 3rd argument.

Sergey

* Sergey Bugaev:

>> Oh, I'm not sure if the run-time check is really that useful.
>>
>> There's no vfcntl function, so I expect that we will have accurate type
>> information at the callsite in most cases, and the compile-time check
>> works.
>
> I see. Well, I copied what the open () fortification was doing, and I
> see that many other fortifications have a runtime-checked version in
> addition to compile-time checks.
>
> There is no vopen either, but it's not hard to imagine someone doing
>
> open (path, O_WRITE | O_CREAT | (cloexec ? O_CLOEXEC : 0))
>
> and similarly
>
> fcntl (fd, cloexec ? F_DUPFD_CLOEXEC : F_DUPFD)
>
> in both cases __builtin_constant_p will be false, and the user will
> miss out on the fortification, and won't notice they forgot the
> required 3rd argument.

That suggests that we should apply __builtin_constant_p to the result of
the cmd check, and not the cmd value.  So something like

  (__builtin_constant_p ((cmd) == F_DUPFD || (cmd) == F_DUPFD_CLOEXEC)
   && ((cmd) == F_DUPFD || (cmd) == F_DUPFD_CLOEXEC))

with a potential guard against evaluating cmd multiple times.

Thanks,
Florian

* Sergey Bugaev:

> On Tue, May 30, 2023 at 2:08 PM Florian Weimer <fweimer@redhat.com> wrote:
>> Ahh, maybe that check is implied by doing this for fortification only?
>
> All of this is only happening within the fortification header, yes,
> that does not even get #included if the preconditions for including it
> fail. The preconditions are currently the same as for the open ()
> fortification due to sharing the same header file, but they are going
> to be different (in v3) because this now needs __VA_OPT__ and __typeof
> but does not need __builtin_va_arg_pack.

Forgot to mention that this suggests that __typeof is actually okay in
the C++ case.

Thanks,
Florian

On Tue, May 30, 2023 at 2:50 PM Florian Weimer <fweimer@redhat.com> wrote:
> That suggests that we should apply __builtin_constant_p to the result of
> the cmd check, and not the cmd value.  So something like
>
>   (__builtin_constant_p ((cmd) == F_DUPFD || (cmd) == F_DUPFD_CLOEXEC)
>    && ((cmd) == F_DUPFD || (cmd) == F_DUPFD_CLOEXEC))

Interesting; we could probably just do __builtin_constant_p
(__fcntl_requires_arg (cmd)). I'll see whether that works.

Do I understand it right that you don't want there to be any runtime
check for this at all? In other words, should I just drop __fcntl_2
and always call __fcntl_alias in case we can't decide statically?

Sergey

* Sergey Bugaev:

> Do I understand it right that you don't want there to be any runtime
> check for this at all? In other words, should I just drop __fcntl_2
> and always call __fcntl_alias in case we can't decide statically?

Yes, that's what I prefer.

Thanks,
Florian