public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] fcntl fortification
@ 2023-05-28 17:20 Sergey Bugaev
  2023-05-28 17:20 ` [PATCH v2 1/3] support: Add support_fcntl_support_ofd_locks () Sergey Bugaev
                   ` (3 more replies)
  0 siblings, 4 replies; 27+ messages in thread
From: Sergey Bugaev @ 2023-05-28 17:20 UTC (permalink / raw)
  To: libc-alpha

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

^ permalink raw reply	[flat|nested] 27+ messages in thread
end of thread, other threads:[~2023-05-30 12:26 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-28 17:20 [PATCH v2 0/3] fcntl fortification Sergey Bugaev
2023-05-28 17:20 ` [PATCH v2 1/3] support: Add support_fcntl_support_ofd_locks () Sergey Bugaev
2023-05-29 13:18   ` Adhemerval Zanella Netto
2023-05-28 17:20 ` [PATCH v2 2/3] cdefs.h: Define __glibc_warn_system_headers_{begin,end} Sergey Bugaev
2023-05-29 14:50   ` [PATCH v2 2/3] cdefs.h: Define __glibc_warn_system_headers_{begin, end} Adhemerval Zanella Netto
2023-05-28 17:20 ` [PATCH v2 3/3] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev
2023-05-29 16:54   ` Adhemerval Zanella Netto
2023-05-29 17:31     ` Sergey Bugaev
2023-05-29 18:09       ` Adhemerval Zanella Netto
2023-05-29 19:57         ` Sergey Bugaev
2023-05-29 20:14           ` Adhemerval Zanella Netto
2023-05-29 20:49             ` Sergey Bugaev
2023-05-29 21:09               ` Adhemerval Zanella Netto
2023-05-29 21:59                 ` Sergey Bugaev
2023-05-30 11:34                   ` Adhemerval Zanella Netto
2023-05-30  7:41         ` Florian Weimer
2023-05-30  9:07           ` Sergey Bugaev
2023-05-30  9:50             ` Florian Weimer
2023-05-30 11:35               ` Adhemerval Zanella Netto
2023-05-30  8:09 ` [PATCH v2 0/3] fcntl fortification Florian Weimer
2023-05-30 10:46   ` Sergey Bugaev
2023-05-30 11:08     ` Florian Weimer
2023-05-30 11:34       ` Sergey Bugaev
2023-05-30 11:50         ` Florian Weimer
2023-05-30 11:51         ` Florian Weimer
2023-05-30 12:15           ` Sergey Bugaev
2023-05-30 12:26             ` Florian Weimer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).