Re: [PATCH v2 0/3] fcntl fortification

[Sergey Bugaev <bugaevc@gmail.com>]

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