* [RFC PATCH 0/1] Attempt to detect missing fcntl argument at compile time
@ 2023-05-19 21:30 Sergey Bugaev
2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev
0 siblings, 1 reply; 31+ messages in thread
From: Sergey Bugaev @ 2023-05-19 21:30 UTC (permalink / raw)
To: libc-alpha
Hello,
here goes another attempt to step out of the comfort zone into the
"I don't know what I'm doing" zone -- this time with fcntl and inline
wrappers and FORTIFY_SOURCE.
It *appears* to build and work and not break anything; I've tried
x86_64-gnu, i686-gnu, i686-linux-gnu, and x86_64-linux-gnu. On
i686-linux-gnu I've tried defining various macros, such as
_LARGEFILE64_SOURCE and _FILE_OFFSET_BITS (to 32 or 64). Things *appear*
to work just as I expect them to; the build breaks when I try to compile
an fcntl () without a required argument, and I do get an error at
runtime as expected too. All the right symbols appear to get referenced
in all configurations, too.
Still, I'm not very sure I've handled all the possible configurations
correctly.
The reasoning for __fcntl_cmd_needs_arg () being a function and not a
macro is that the definition needs to use multiple #ifdefs and that
would be awkward with a macro. Hopefully I have handled various
combinations of fcntl commands being defined / not defined correctly
too, so the user never gets a compilation error just because
__fcntl_cmd_needs_arg () wants to handle some command that is not
defined in their configuration. When building fcntl{,64}_2.c, all the
commands (that are supported on this kernel, i.e. not Linux specifics
on the Hurd) should be defined, and so the runtime calls to
__fcntl_cmd_needs_arg () get this full version. When building user code,
some commands may not be defined (depending on enabled feature macros),
but then the expectation is the user cannot use those commands they do
not have enabled.
Sergey
^ permalink raw reply [flat|nested] 31+ messages in thread* [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-19 21:30 [RFC PATCH 0/1] Attempt to detect missing fcntl argument at compile time Sergey Bugaev @ 2023-05-19 21:30 ` Sergey Bugaev 2023-05-19 21:55 ` Joseph Myers ` (3 more replies) 0 siblings, 4 replies; 31+ messages in thread From: Sergey Bugaev @ 2023-05-19 21:30 UTC (permalink / raw) To: libc-alpha; +Cc: Hector Martin Both open () and fcntl () are "overloaded" to accept either 2 or 3 arguments; whether the last argument is required depends on the values of the previous arguments. Since C provides no native support for function overloading, this is implemented by making these functions vararg. Unfortunately, this means the compiler is unable to check the number of arguments and their types for correctness at compile time, and will not diagnose any mistakes. To help with this, when FORTIFY_SOURCE is enabled, the special fcntl2.h header replaces open () with a wrapper that checks the passed number of arguments, raising a compile-time or run-time error on mismatch. This commit adds similar handling for fcntl (). Recently, Hector Martin <marcan@marcan.st> has identified an issue in libwebrtc where fcntl (fd, F_DUPFD_CLOEXEC) was invoked without a third argument [0]. With the patch, the bug would have been detected at compile time, assuming libwebrtc is built with FORTIFY_SOURCE. Hopefully this will help detecting similar bugs in existing software, and prevent more instances of similar bugs from being introduced in the future. [0]: https://social.treehouse.systems/@marcan/110355001391961285 The abilists have been modified with 'make update-abi-all'. Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- include/fcntl.h | 3 + io/Makefile | 2 +- io/Versions | 4 + io/bits/fcntl2.h | 139 ++++++++++++++++++ io/fcntl64_2.c | 33 +++++ io/fcntl_2.c | 33 +++++ sysdeps/mach/hurd/i386/libc.abilist | 2 + sysdeps/mach/hurd/x86_64/libc.abilist | 2 + sysdeps/unix/sysv/linux/aarch64/libc.abilist | 2 + sysdeps/unix/sysv/linux/alpha/libc.abilist | 2 + sysdeps/unix/sysv/linux/arc/libc.abilist | 2 + sysdeps/unix/sysv/linux/arm/be/libc.abilist | 2 + sysdeps/unix/sysv/linux/arm/le/libc.abilist | 2 + sysdeps/unix/sysv/linux/csky/libc.abilist | 2 + sysdeps/unix/sysv/linux/hppa/libc.abilist | 2 + sysdeps/unix/sysv/linux/i386/libc.abilist | 2 + sysdeps/unix/sysv/linux/ia64/libc.abilist | 2 + .../sysv/linux/loongarch/lp64/libc.abilist | 2 + .../sysv/linux/m68k/coldfire/libc.abilist | 2 + .../unix/sysv/linux/m68k/m680x0/libc.abilist | 2 + .../sysv/linux/microblaze/be/libc.abilist | 2 + .../sysv/linux/microblaze/le/libc.abilist | 2 + .../sysv/linux/mips/mips32/fpu/libc.abilist | 2 + .../sysv/linux/mips/mips32/nofpu/libc.abilist | 2 + .../sysv/linux/mips/mips64/n32/libc.abilist | 2 + .../sysv/linux/mips/mips64/n64/libc.abilist | 2 + sysdeps/unix/sysv/linux/nios2/libc.abilist | 2 + sysdeps/unix/sysv/linux/or1k/libc.abilist | 2 + .../linux/powerpc/powerpc32/fpu/libc.abilist | 2 + .../powerpc/powerpc32/nofpu/libc.abilist | 2 + .../linux/powerpc/powerpc64/be/libc.abilist | 2 + .../linux/powerpc/powerpc64/le/libc.abilist | 2 + .../unix/sysv/linux/riscv/rv32/libc.abilist | 2 + .../unix/sysv/linux/riscv/rv64/libc.abilist | 2 + .../unix/sysv/linux/s390/s390-32/libc.abilist | 2 + .../unix/sysv/linux/s390/s390-64/libc.abilist | 2 + sysdeps/unix/sysv/linux/sh/be/libc.abilist | 2 + sysdeps/unix/sysv/linux/sh/le/libc.abilist | 2 + .../sysv/linux/sparc/sparc32/libc.abilist | 2 + .../sysv/linux/sparc/sparc64/libc.abilist | 2 + .../unix/sysv/linux/x86_64/64/libc.abilist | 2 + .../unix/sysv/linux/x86_64/x32/libc.abilist | 2 + 42 files changed, 285 insertions(+), 1 deletion(-) create mode 100644 io/fcntl64_2.c create mode 100644 io/fcntl_2.c diff --git a/include/fcntl.h b/include/fcntl.h index d788db2e..7742c002 100644 --- a/include/fcntl.h +++ b/include/fcntl.h @@ -32,6 +32,9 @@ extern int __open64_2 (const char *__path, int __oflag); extern int __openat_2 (int __fd, const char *__path, int __oflag); extern int __openat64_2 (int __fd, const char *__path, int __oflag); +extern int __fcntl_2 (int __fd, int __cmd); +extern int __fcntl64_2 (int __fd, int __cmd); + /* Makes open () & friends faster on the Hurd, but can only be used (without altering user-visible behavior) when we're sure that the file we're opening diff --git a/io/Makefile b/io/Makefile index f72571cd..350807ee 100644 --- a/io/Makefile +++ b/io/Makefile @@ -41,7 +41,7 @@ routines := \ mkdir mkdirat \ open open_2 open64 open64_2 openat openat_2 openat64 openat64_2 \ read write lseek lseek64 access euidaccess faccessat \ - fcntl fcntl64 flock lockf lockf64 \ + fcntl fcntl_2 fcntl64 fcntl64_2 flock lockf lockf64 \ close dup dup2 dup3 pipe pipe2 \ creat creat64 \ chdir fchdir \ diff --git a/io/Versions b/io/Versions index 4e195408..8409e105 100644 --- a/io/Versions +++ b/io/Versions @@ -140,6 +140,10 @@ libc { GLIBC_2.34 { closefrom; } + GLIBC_2.38 { + __fcntl_2; + __fcntl64_2; + } GLIBC_PRIVATE { __libc_fcntl64; __fcntl_nocancel; diff --git a/io/bits/fcntl2.h b/io/bits/fcntl2.h index bdb48fa8..beb3a628 100644 --- a/io/bits/fcntl2.h +++ b/io/bits/fcntl2.h @@ -170,3 +170,142 @@ openat64 (int __fd, const char *__path, int __oflag, ...) } # endif #endif + +#ifndef __USE_TIME_BITS64 + +# ifndef __USE_FILE_OFFSET64 +/* Just fcntl (). */ +extern int __fcntl_2 (int __fd, int __cmd); +extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl); +# else +/* fcntl.h redirects fcntl () to fcntl64 (). */ +extern int __REDIRECT (__fcntl_2, (int __fd, int __cmd), __fcntl64_2); +extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl64); +# endif /* __USE_FILE_OFFSET64 */ + +# ifdef __USE_LARGEFILE64 +/* Just fcntl64 (). */ +extern int __fcntl64_2 (int __fd, int __cmd); +extern int __REDIRECT (__fcntl64_alias, (int __fd, int __cmd, ...), fcntl64); +# endif + +#else /* __USE_TIME_BITS64 */ + +/* fcntl.h redirects both fcntl () and fcntl64 () to __fcntl_time64 (). */ +extern int __fcntl64_2 (int __fd, int __cmd); +extern int __REDIRECT (__fcntl_2, (int __fd, int __cmd), __fcntl64_2); +extern int __REDIRECT_NTH (__fcntl_alias, (int __fd, int __cmd, ...), + __fcntl_time64); +extern int __REDIRECT_NTH (__fcntl64_alias, (int __fd, int __cmd, ...), + __fcntl_time64); +#endif /* __USE_TIME_BITS64 */ + +__errordecl (__fcntl_too_many_args, + "fcntl can be called either with 2 or 3 arguments, not more"); +__errordecl (__fcntl_missing_arg, + "fcntl with with this cmd needs 3 arguments"); + +__extern_always_inline int +__fcntl_cmd_needs_arg (int __cmd) +{ + switch (__cmd) + { + case F_DUPFD: + case F_DUPFD_CLOEXEC: + case F_SETFD: + case F_SETFL: +#ifdef F_SETLK + case F_SETLK: + case F_SETLKW: + case F_GETLK: +#endif +#ifdef F_OFD_SETLK + case F_OFD_SETLK: + case F_OFD_SETLKW: + case F_OFD_GETLK: +#endif +#ifdef F_SETOWN + case F_SETOWN: +#endif +#ifdef __F_GETOWN_EX + case __F_GETOWN_EX: + case __F_SETOWN_EX: + case __F_SETSIG: +#endif +#ifdef F_SETLEASE + case F_SETLEASE: + case F_NOTIFY: + case F_SETPIPE_SZ: + case F_ADD_SEALS: + case F_GET_RW_HINT: + case F_SET_RW_HINT: + case F_GET_FILE_RW_HINT: + case F_SET_FILE_RW_HINT: +#endif + return 1; + + case F_GETFD: + case F_GETFL: +#ifdef F_GETOWN + case F_GETOWN: +#endif +#ifdef __F_GETSIG + case __F_GETSIG: +#endif +#ifdef F_GETLEASE + case F_GETLEASE: + case F_GETPIPE_SZ: + case F_GET_SEALS: +#endif + /* For any cmd value we don't know about, + default to not requiring an argument. */ + default: + return 0; + } +} + +__fortify_function int +fcntl (int __fd, int __cmd, ...) +{ + if (__va_arg_pack_len () > 1) + __fcntl_too_many_args (); + + if (__builtin_constant_p (__cmd)) + { + if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1) + { + __fcntl_missing_arg (); + return __fcntl_2 (__fd, __cmd); + } + return __fcntl_alias (__fd, __cmd, __va_arg_pack ()); + } + + if (__va_arg_pack_len () < 1) + return __fcntl_2 (__fd, __cmd); + + return __fcntl_alias (__fd, __cmd, __va_arg_pack ()); +} + +#ifdef __USE_LARGEFILE64 +__fortify_function int +fcntl64 (int __fd, int __cmd, ...) +{ + if (__va_arg_pack_len () > 1) + __fcntl_too_many_args (); + + if (__builtin_constant_p (__cmd)) + { + if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1) + { + __fcntl_missing_arg (); + return __fcntl64_2 (__fd, __cmd); + } + return __fcntl64_alias (__fd, __cmd, __va_arg_pack ()); + } + + if (__va_arg_pack_len () < 1) + return __fcntl64_2 (__fd, __cmd); + + return __fcntl64_alias (__fd, __cmd, __va_arg_pack ()); +} +#endif diff --git a/io/fcntl64_2.c b/io/fcntl64_2.c new file mode 100644 index 00000000..6352f524 --- /dev/null +++ b/io/fcntl64_2.c @@ -0,0 +1,33 @@ +/* _FORTIFY_SOURCE wrapper for fcntl64. + Copyright (C) 2013-2023 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +/* Make sure to get __fcntl_cmd_needs_arg from bits/fcntl2.h */ +#undef _FORTIFY_SOURCE +#define _FORTIFY_SOURCE 1 + +#include <fcntl.h> +#include <stdio.h> + +int +__fcntl64_2 (int fd, int cmd) +{ + if (__fcntl_cmd_needs_arg (cmd)) + __fortify_fail ("invalid fcntl64 call: this cmd requires an argument"); + + return __libc_fcntl64 (fd, cmd); +} diff --git a/io/fcntl_2.c b/io/fcntl_2.c new file mode 100644 index 00000000..fc9303e4 --- /dev/null +++ b/io/fcntl_2.c @@ -0,0 +1,33 @@ +/* _FORTIFY_SOURCE wrapper for fcntl. + Copyright (C) 2013-2023 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + +/* Make sure to get __fcntl_cmd_needs_arg from bits/fcntl2.h */ +#undef _FORTIFY_SOURCE +#define _FORTIFY_SOURCE 1 + +#include <fcntl.h> +#include <stdio.h> + +int +__fcntl_2 (int fd, int cmd) +{ + if (__fcntl_cmd_needs_arg (cmd)) + __fortify_fail ("invalid fcntl call: this cmd requires an argument"); + + return __libc_fcntl (fd, cmd); +} diff --git a/sysdeps/mach/hurd/i386/libc.abilist b/sysdeps/mach/hurd/i386/libc.abilist index 6925222f..fa24d577 100644 --- a/sysdeps/mach/hurd/i386/libc.abilist +++ b/sysdeps/mach/hurd/i386/libc.abilist @@ -2294,6 +2294,8 @@ GLIBC_2.36 arc4random_buf F GLIBC_2.36 arc4random_uniform F GLIBC_2.36 c8rtomb F GLIBC_2.36 mbrtoc8 F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/mach/hurd/x86_64/libc.abilist b/sysdeps/mach/hurd/x86_64/libc.abilist index a0be5c1a..efb83b16 100644 --- a/sysdeps/mach/hurd/x86_64/libc.abilist +++ b/sysdeps/mach/hurd/x86_64/libc.abilist @@ -194,6 +194,8 @@ GLIBC_2.38 __errno_location F GLIBC_2.38 __explicit_bzero_chk F GLIBC_2.38 __fbufsize F GLIBC_2.38 __fcntl F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __fdelt_chk F GLIBC_2.38 __fdelt_warn F GLIBC_2.38 __fentry__ F diff --git a/sysdeps/unix/sysv/linux/aarch64/libc.abilist b/sysdeps/unix/sysv/linux/aarch64/libc.abilist index 0e2d9c30..86144979 100644 --- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist +++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist @@ -2633,6 +2633,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/alpha/libc.abilist b/sysdeps/unix/sysv/linux/alpha/libc.abilist index f1bec197..646c11e3 100644 --- a/sysdeps/unix/sysv/linux/alpha/libc.abilist +++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist @@ -2730,6 +2730,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/arc/libc.abilist b/sysdeps/unix/sysv/linux/arc/libc.abilist index aa874b88..37e9129f 100644 --- a/sysdeps/unix/sysv/linux/arc/libc.abilist +++ b/sysdeps/unix/sysv/linux/arc/libc.abilist @@ -2394,6 +2394,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/arm/be/libc.abilist b/sysdeps/unix/sysv/linux/arm/be/libc.abilist index afbd57da..4c9b937e 100644 --- a/sysdeps/unix/sysv/linux/arm/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/arm/be/libc.abilist @@ -514,6 +514,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/arm/le/libc.abilist b/sysdeps/unix/sysv/linux/arm/le/libc.abilist index e7364cd3..8dbf72c5 100644 --- a/sysdeps/unix/sysv/linux/arm/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/arm/le/libc.abilist @@ -511,6 +511,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/csky/libc.abilist b/sysdeps/unix/sysv/linux/csky/libc.abilist index 913fa592..b58b2b9c 100644 --- a/sysdeps/unix/sysv/linux/csky/libc.abilist +++ b/sysdeps/unix/sysv/linux/csky/libc.abilist @@ -2670,6 +2670,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/hppa/libc.abilist b/sysdeps/unix/sysv/linux/hppa/libc.abilist index 43af3a98..c80bb835 100644 --- a/sysdeps/unix/sysv/linux/hppa/libc.abilist +++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist @@ -2619,6 +2619,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/i386/libc.abilist b/sysdeps/unix/sysv/linux/i386/libc.abilist index af72f8fa..5a737b3d 100644 --- a/sysdeps/unix/sysv/linux/i386/libc.abilist +++ b/sysdeps/unix/sysv/linux/i386/libc.abilist @@ -2803,6 +2803,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/ia64/libc.abilist b/sysdeps/unix/sysv/linux/ia64/libc.abilist index 48cbb0fa..aab6d422 100644 --- a/sysdeps/unix/sysv/linux/ia64/libc.abilist +++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist @@ -2568,6 +2568,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist index c15884bb..82da98d2 100644 --- a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist +++ b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist @@ -2154,6 +2154,8 @@ GLIBC_2.36 wprintf F GLIBC_2.36 write F GLIBC_2.36 writev F GLIBC_2.36 wscanf F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist index 3738db81..f6c80b46 100644 --- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist +++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist @@ -515,6 +515,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist index ed136277..dc34ea56 100644 --- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist +++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist @@ -2746,6 +2746,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist index 83577386..466e4092 100644 --- a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist @@ -2719,6 +2719,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist index 58c5da58..dc27b4e7 100644 --- a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist @@ -2716,6 +2716,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist index d3741945..a8dfa647 100644 --- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist @@ -2711,6 +2711,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist index 5319fdc2..4defd8dc 100644 --- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist @@ -2709,6 +2709,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist index 1743ea6e..824b6b4c 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist @@ -2717,6 +2717,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist index 9b1f53c6..ff4341a8 100644 --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist +++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist @@ -2619,6 +2619,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/nios2/libc.abilist b/sysdeps/unix/sysv/linux/nios2/libc.abilist index ae1c6ca1..ede0e6eb 100644 --- a/sysdeps/unix/sysv/linux/nios2/libc.abilist +++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist @@ -2758,6 +2758,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/or1k/libc.abilist b/sysdeps/unix/sysv/linux/or1k/libc.abilist index a7c572c9..22525fe8 100644 --- a/sysdeps/unix/sysv/linux/or1k/libc.abilist +++ b/sysdeps/unix/sysv/linux/or1k/libc.abilist @@ -2140,6 +2140,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist index 074fa031..2938361e 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist @@ -2773,6 +2773,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist index dfcb4bd2..e1c70a25 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist @@ -2806,6 +2806,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist index 63bbccf3..2ce2d974 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist @@ -2527,6 +2527,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist index ab85fd61..b1024501 100644 --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist @@ -2829,6 +2829,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fscanfieee128 F GLIBC_2.38 __isoc23_fwscanf F diff --git a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist index b716f5c7..833fb747 100644 --- a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist +++ b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist @@ -2396,6 +2396,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist index 774e777b..cbd85454 100644 --- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist @@ -2596,6 +2596,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist index 8625135c..1442a0b9 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist +++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist @@ -2771,6 +2771,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist index d00c7eb2..1a7f4fc7 100644 --- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist +++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist @@ -2564,6 +2564,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/sh/be/libc.abilist b/sysdeps/unix/sysv/linux/sh/be/libc.abilist index b6303724..3146ce99 100644 --- a/sysdeps/unix/sysv/linux/sh/be/libc.abilist +++ b/sysdeps/unix/sysv/linux/sh/be/libc.abilist @@ -2626,6 +2626,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/sh/le/libc.abilist b/sysdeps/unix/sysv/linux/sh/le/libc.abilist index d8005561..31360691 100644 --- a/sysdeps/unix/sysv/linux/sh/le/libc.abilist +++ b/sysdeps/unix/sysv/linux/sh/le/libc.abilist @@ -2623,6 +2623,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist index 5be55c11..026e9581 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist @@ -2766,6 +2766,8 @@ GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F GLIBC_2.37 __ppoll64_chk F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist index 475fdaae..6ecb0d46 100644 --- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist @@ -2591,6 +2591,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist index 6cfb928b..a37f4ab4 100644 --- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist @@ -2542,6 +2542,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist index c7350971..3b8790f4 100644 --- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist +++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist @@ -2648,6 +2648,8 @@ GLIBC_2.36 pidfd_open F GLIBC_2.36 pidfd_send_signal F GLIBC_2.36 process_madvise F GLIBC_2.36 process_mrelease F +GLIBC_2.38 __fcntl64_2 F +GLIBC_2.38 __fcntl_2 F GLIBC_2.38 __isoc23_fscanf F GLIBC_2.38 __isoc23_fwscanf F GLIBC_2.38 __isoc23_scanf F -- 2.40.1 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev @ 2023-05-19 21:55 ` Joseph Myers 2023-05-20 11:46 ` Sergey Bugaev 2023-05-23 19:09 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Adhemerval Zanella Netto ` (2 subsequent siblings) 3 siblings, 1 reply; 31+ messages in thread From: Joseph Myers @ 2023-05-19 21:55 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha, Hector Martin I'm concerned about the lack of testcases in this patch. I'm not sure of how we should be checking for compile-time errors (there are several existing tests for runtime fortification), but having this functionality without testcases seems risky. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-19 21:55 ` Joseph Myers @ 2023-05-20 11:46 ` Sergey Bugaev 2023-05-20 18:21 ` [RFC PATCH] debug: Add tests for fortified fcntl () Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-20 11:46 UTC (permalink / raw) To: Joseph Myers; +Cc: libc-alpha Hello, On Sat, May 20, 2023 at 12:55 AM Joseph Myers <joseph@codesourcery.com> wrote: > I'm concerned about the lack of testcases in this patch. I'm not sure of > how we should be checking for compile-time errors (there are several > existing tests for runtime fortification), but having this functionality > without testcases seems risky. Yes, absolutely, I agree. Let me attempt to add some tests then! Considering it's unclear how to test for compilation errors, maybe we don't have to (despite these errors being the main point of the patch). What's important is ensuring that the fortification doesn't break legitimate programs, either because of me messing up how fcntl/fcntl64 are defined in some particular configuration vs how they are now defined in fcntl2.h, or because of some undefined F_COMMAND being used in __fcntl_cmd_needs_arg. I see that there's already debug/tst-fortify.c that gets built and tested in various configurations (exactly what I need!) and even has the CHK_FAIL_START / CHK_FAIL_END facilities for catching the SIGABRT and recording test success. So should I just add a bunch of fcntl/fcntl64 tests there? What would be a good way to test for 32-bit vs 64-bit confusion? Something involving F_OFD_*LK, perhaps? Is there an existing test for open{at,64,at64} fortification somewhere that I could copy-paste^W^W take inspiration from? Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* [RFC PATCH] debug: Add tests for fortified fcntl () 2023-05-20 11:46 ` Sergey Bugaev @ 2023-05-20 18:21 ` Sergey Bugaev 2023-05-23 18:40 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-20 18:21 UTC (permalink / raw) To: libc-alpha; +Cc: Joseph Myers Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> --- debug/tst-fortify.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 158 insertions(+) diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c index 7850a4e5..17a15de7 100644 --- a/debug/tst-fortify.c +++ b/debug/tst-fortify.c @@ -78,6 +78,15 @@ do_prepare (void) } } +/* Return VALUE, but do it in a way that the compiler cannot + see that it's a compile-time constant. */ +static int __attribute_noinline__ +hide_constant (int value) +{ + volatile int v = value; + return v; +} + volatile int chk_fail_ok; volatile int ret; jmp_buf chk_fail_buf; @@ -1763,6 +1772,155 @@ do_test (void) ppoll (fds, l0 + 2, NULL, NULL); CHK_FAIL_END # endif +#endif + + /* Check that we can do basic fcntl operations, both ones that require + the third argument, and ones that do not. */ + res = fcntl (STDIN_FILENO, F_GETFD); + TEST_COMPARE (res, 0); + res = fcntl (STDIN_FILENO, F_SETFD, 0); + TEST_COMPARE (res, 0); + +#ifdef F_OFD_GETLK + /* Check for confusion between 32- and 64-bit versions of the fcntl + interface. But first, check that the kernel supports OFD locks at all, + using a non-fortified function. */ + int lockfd1 = xopen (temp_filename, O_RDWR, 0); + int lockfd2 = xopen (temp_filename, O_RDWR, 0); + + struct flock flock; + memset (&flock, 0, sizeof (flock)); + flock.l_type = F_WRLCK; + flock.l_whence = SEEK_SET; + flock.l_start = 0; + flock.l_len = INT32_MAX; + flock.l_pid = 0; + + res = __fcntl (lockfd1, F_OFD_GETLK, &flock); + int ofd_locks_supported = (res != -1 || errno != EINVAL); + + if (ofd_locks_supported) + { + TEST_COMPARE (res, 0); + TEST_COMPARE (flock.l_type, F_UNLCK); + + memset (&flock, 0, sizeof (flock)); + flock.l_type = F_WRLCK; + flock.l_whence = SEEK_SET; + flock.l_start = 1234; + flock.l_len = 5678; + flock.l_pid = 0; + + res = fcntl (lockfd1, F_OFD_SETLK, &flock); + TEST_COMPARE (res, 0); + + memset (&flock, 0, sizeof (flock)); + flock.l_type = F_RDLCK; + flock.l_whence = SEEK_SET; + flock.l_start = 3542; + flock.l_len = 411; + flock.l_pid = 0; + + res = fcntl (lockfd2, F_OFD_GETLK, &flock); + TEST_COMPARE (res, 0); + /* Check that we get the expected values. */ + TEST_COMPARE (flock.l_type, F_WRLCK); + TEST_COMPARE (flock.l_whence, SEEK_SET); + TEST_COMPARE (flock.l_start, 1234); + TEST_COMPARE (flock.l_len, 5678); + TEST_COMPARE (flock.l_pid, -1); + } +#endif + + /* Check that we can do fcntl operations with CMD that is not constant + at compile time. */ + res = fcntl (STDIN_FILENO, hide_constant (F_GETFD)); + TEST_COMPARE (res, 0); + res = fcntl (STDIN_FILENO, hide_constant (F_SETFD), 0); + TEST_COMPARE (res, 0); + +#ifdef F_OFD_GETLK + if (ofd_locks_supported) + { + memset (&flock, 0, sizeof (flock)); + flock.l_type = F_RDLCK; + flock.l_whence = SEEK_SET; + flock.l_start = 3542; + flock.l_len = 411; + flock.l_pid = 0; + + res = fcntl (lockfd2, hide_constant (F_OFD_GETLK), &flock); + TEST_COMPARE (res, 0); + /* Check that we get the expected values. */ + TEST_COMPARE (flock.l_type, F_WRLCK); + TEST_COMPARE (flock.l_whence, SEEK_SET); + TEST_COMPARE (flock.l_start, 1234); + TEST_COMPARE (flock.l_len, 5678); + TEST_COMPARE (flock.l_pid, -1); + } +#endif + +#if __USE_FORTIFY_LEVEL >= 1 + CHK_FAIL_START + fcntl (STDIN_FILENO, hide_constant (F_SETFD)); + CHK_FAIL_END +#endif + +#if defined (__USE_LARGEFILE64) || defined (__USE_TIME_BITS64) + /* Also check fcntl64 (). */ + res = fcntl64 (STDIN_FILENO, F_GETFD); + TEST_COMPARE (res, 0); + res = fcntl64 (STDIN_FILENO, F_SETFD, 0); + TEST_COMPARE (res, 0); + res = fcntl64 (STDIN_FILENO, hide_constant (F_GETFD)); + TEST_COMPARE (res, 0); + res = fcntl64 (STDIN_FILENO, hide_constant (F_SETFD), 0); + TEST_COMPARE (res, 0); + +#ifdef F_OFD_GETLK + if (ofd_locks_supported) + { + struct flock64 flock64; + + memset (&flock64, 0, sizeof (flock64)); + flock64.l_type = F_RDLCK; + flock64.l_whence = SEEK_SET; + flock64.l_start = 3542; + flock64.l_len = 411; + flock64.l_pid = 0; + + res = fcntl64 (lockfd2, F_OFD_GETLK, &flock64); + TEST_COMPARE (res, 0); + /* Check that we get the expected values. */ + TEST_COMPARE (flock64.l_type, F_WRLCK); + TEST_COMPARE (flock64.l_whence, SEEK_SET); + TEST_COMPARE (flock64.l_start, 1234); + TEST_COMPARE (flock64.l_len, 5678); + TEST_COMPARE (flock64.l_pid, -1); + + memset (&flock64, 0, sizeof (flock64)); + flock64.l_type = F_RDLCK; + flock64.l_whence = SEEK_SET; + flock64.l_start = 3542; + flock64.l_len = 411; + flock64.l_pid = 0; + + res = fcntl64 (lockfd2, hide_constant (F_OFD_GETLK), &flock64); + TEST_COMPARE (res, 0); + /* Check that we get the expected values. */ + TEST_COMPARE (flock64.l_type, F_WRLCK); + TEST_COMPARE (flock64.l_whence, SEEK_SET); + TEST_COMPARE (flock64.l_start, 1234); + TEST_COMPARE (flock64.l_len, 5678); + TEST_COMPARE (flock64.l_pid, -1); + } +#endif + +# if __USE_FORTIFY_LEVEL >= 1 + CHK_FAIL_START + fcntl64 (STDIN_FILENO, hide_constant (F_SETFD)); + CHK_FAIL_END +# endif #endif return ret; -- 2.40.1 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH] debug: Add tests for fortified fcntl () 2023-05-20 18:21 ` [RFC PATCH] debug: Add tests for fortified fcntl () Sergey Bugaev @ 2023-05-23 18:40 ` Adhemerval Zanella Netto 2023-05-23 19:19 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-23 18:40 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha; +Cc: Joseph Myers I think it would be better to include this test on the fcntl fortify patch itself, or at least on a patchset so the first one is set as prerequisite. On 20/05/23 15:21, Sergey Bugaev via Libc-alpha wrote: > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> > --- > debug/tst-fortify.c | 158 ++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 158 insertions(+) > > diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c > index 7850a4e5..17a15de7 100644 > --- a/debug/tst-fortify.c > +++ b/debug/tst-fortify.c > @@ -78,6 +78,15 @@ do_prepare (void) > } > } > > +/* Return VALUE, but do it in a way that the compiler cannot > + see that it's a compile-time constant. */ > +static int __attribute_noinline__ > +hide_constant (int value) > +{ > + volatile int v = value; > + return v; > +} > + > volatile int chk_fail_ok; > volatile int ret; > jmp_buf chk_fail_buf; > @@ -1763,6 +1772,155 @@ do_test (void) > ppoll (fds, l0 + 2, NULL, NULL); > CHK_FAIL_END > # endif > +#endif > + > + /* Check that we can do basic fcntl operations, both ones that require > + the third argument, and ones that do not. */ > + res = fcntl (STDIN_FILENO, F_GETFD); > + TEST_COMPARE (res, 0); > + res = fcntl (STDIN_FILENO, F_SETFD, 0); > + TEST_COMPARE (res, 0); > + > +#ifdef F_OFD_GETLK > + /* Check for confusion between 32- and 64-bit versions of the fcntl > + interface. But first, check that the kernel supports OFD locks at all, > + using a non-fortified function. */ > + int lockfd1 = xopen (temp_filename, O_RDWR, 0); > + int lockfd2 = xopen (temp_filename, O_RDWR, 0); It misses the inclusion of 'support/xunistd.h>' (the testcase fails for build it). > + > + struct flock flock; > + memset (&flock, 0, sizeof (flock)); > + flock.l_type = F_WRLCK; > + flock.l_whence = SEEK_SET; > + flock.l_start = 0; > + flock.l_len = INT32_MAX; > + flock.l_pid = 0; > + > + res = __fcntl (lockfd1, F_OFD_GETLK, &flock); This is an external case, so use the external default name. > + int ofd_locks_supported = (res != -1 || errno != EINVAL); > + > + if (ofd_locks_supported) > + { > + TEST_COMPARE (res, 0); > + TEST_COMPARE (flock.l_type, F_UNLCK); > + > + memset (&flock, 0, sizeof (flock)); > + flock.l_type = F_WRLCK; > + flock.l_whence = SEEK_SET; > + flock.l_start = 1234; > + flock.l_len = 5678; > + flock.l_pid = 0; > + > + res = fcntl (lockfd1, F_OFD_SETLK, &flock); > + TEST_COMPARE (res, 0); > + > + memset (&flock, 0, sizeof (flock)); > + flock.l_type = F_RDLCK; > + flock.l_whence = SEEK_SET; > + flock.l_start = 3542; > + flock.l_len = 411; > + flock.l_pid = 0; > + > + res = fcntl (lockfd2, F_OFD_GETLK, &flock); > + TEST_COMPARE (res, 0); > + /* Check that we get the expected values. */ > + TEST_COMPARE (flock.l_type, F_WRLCK); > + TEST_COMPARE (flock.l_whence, SEEK_SET); > + TEST_COMPARE (flock.l_start, 1234); > + TEST_COMPARE (flock.l_len, 5678); > + TEST_COMPARE (flock.l_pid, -1); > + } > +#endif > + > + /* Check that we can do fcntl operations with CMD that is not constant > + at compile time. */ > + res = fcntl (STDIN_FILENO, hide_constant (F_GETFD)); > + TEST_COMPARE (res, 0); > + res = fcntl (STDIN_FILENO, hide_constant (F_SETFD), 0); > + TEST_COMPARE (res, 0); > + > +#ifdef F_OFD_GETLK > + if (ofd_locks_supported) > + { > + memset (&flock, 0, sizeof (flock)); > + flock.l_type = F_RDLCK; > + flock.l_whence = SEEK_SET; > + flock.l_start = 3542; > + flock.l_len = 411; > + flock.l_pid = 0; > + > + res = fcntl (lockfd2, hide_constant (F_OFD_GETLK), &flock); > + TEST_COMPARE (res, 0); > + /* Check that we get the expected values. */ > + TEST_COMPARE (flock.l_type, F_WRLCK); > + TEST_COMPARE (flock.l_whence, SEEK_SET); > + TEST_COMPARE (flock.l_start, 1234); > + TEST_COMPARE (flock.l_len, 5678); > + TEST_COMPARE (flock.l_pid, -1); > + } > +#endif > + > +#if __USE_FORTIFY_LEVEL >= 1 > + CHK_FAIL_START > + fcntl (STDIN_FILENO, hide_constant (F_SETFD)); > + CHK_FAIL_END > +#endif > + > +#if defined (__USE_LARGEFILE64) || defined (__USE_TIME_BITS64) > + /* Also check fcntl64 (). */ > + res = fcntl64 (STDIN_FILENO, F_GETFD); > + TEST_COMPARE (res, 0); > + res = fcntl64 (STDIN_FILENO, F_SETFD, 0); > + TEST_COMPARE (res, 0); > + res = fcntl64 (STDIN_FILENO, hide_constant (F_GETFD)); > + TEST_COMPARE (res, 0); > + res = fcntl64 (STDIN_FILENO, hide_constant (F_SETFD), 0); > + TEST_COMPARE (res, 0); > + > +#ifdef F_OFD_GETLK > + if (ofd_locks_supported) > + { > + struct flock64 flock64; > + > + memset (&flock64, 0, sizeof (flock64)); > + flock64.l_type = F_RDLCK; > + flock64.l_whence = SEEK_SET; > + flock64.l_start = 3542; > + flock64.l_len = 411; > + flock64.l_pid = 0; > + > + res = fcntl64 (lockfd2, F_OFD_GETLK, &flock64); > + TEST_COMPARE (res, 0); > + /* Check that we get the expected values. */ > + TEST_COMPARE (flock64.l_type, F_WRLCK); > + TEST_COMPARE (flock64.l_whence, SEEK_SET); > + TEST_COMPARE (flock64.l_start, 1234); > + TEST_COMPARE (flock64.l_len, 5678); > + TEST_COMPARE (flock64.l_pid, -1); > + > + memset (&flock64, 0, sizeof (flock64)); > + flock64.l_type = F_RDLCK; > + flock64.l_whence = SEEK_SET; > + flock64.l_start = 3542; > + flock64.l_len = 411; > + flock64.l_pid = 0; > + > + res = fcntl64 (lockfd2, hide_constant (F_OFD_GETLK), &flock64); > + TEST_COMPARE (res, 0); > + /* Check that we get the expected values. */ > + TEST_COMPARE (flock64.l_type, F_WRLCK); > + TEST_COMPARE (flock64.l_whence, SEEK_SET); > + TEST_COMPARE (flock64.l_start, 1234); > + TEST_COMPARE (flock64.l_len, 5678); > + TEST_COMPARE (flock64.l_pid, -1); > + } > +#endif > + > +# if __USE_FORTIFY_LEVEL >= 1 > + CHK_FAIL_START > + fcntl64 (STDIN_FILENO, hide_constant (F_SETFD)); > + CHK_FAIL_END > +# endif > #endif > > return ret; ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH] debug: Add tests for fortified fcntl () 2023-05-23 18:40 ` Adhemerval Zanella Netto @ 2023-05-23 19:19 ` Sergey Bugaev 2023-05-23 19:48 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-23 19:19 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha, Joseph Myers Hello, thank you for taking a look. On Tue, May 23, 2023 at 9:40 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > + res = __fcntl (lockfd1, F_OFD_GETLK, &flock); > > This is an external case, so use the external default name. Do you mean that this should be s/__fcntl/fcntl/? If that is what you mean -- here's why I used __fcntl here, but not in all the other places: The point of the OFD tests in the patch is to check for 32-bit vs 64-bit confusion in the fortification. If, for example, in some configuration the plain fcntl is supposed to be 32-bit (and struct flock is 32-bit), but the fortified version of fcntl mistakenly forwards to fcntl64, then the 32-bit struct flock will not be converted to struct flock64, and will be passed to the kernel as-is. It will yield some garbage when interpreted as a struct flock64, and the kernel will likely reject it with an error (EINVAL, perhaps) -- which is what the test checks against. But the test also needs to work on the kernel versions that do not support OFD locks, so it does this check for support being present at all first, and skips running the OFD tests if the kernel does not support it. But how do you check for OFD locks support? -- you try to F_OFD_GETLK and see if the kernel returns EINVAL. If it does return that, OFD locks are unsupported. And herein lies the problem, we cannot differentiate between EINVALs caused by missing kernel support from EINVALs caused by the very kind of bugs this test checks against! We'd mistake the actual issue we wanted to catch for missing kernel support, skip the bulk of the test, and report success. So in this first check of whether or not OFD locks are supported at all, I've used __fcntl, which does not get fortified and so cannot suffer from the 32-bit vs 64-bit confusion bug. The rest of the code uses the regular fcntl. There's a comment above briefly mentioning this, too -- clearly it has to be expanded. So, with this in mind, should I be using __fcntl here, or is there a better option? Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH] debug: Add tests for fortified fcntl () 2023-05-23 19:19 ` Sergey Bugaev @ 2023-05-23 19:48 ` Adhemerval Zanella Netto 2023-05-24 7:15 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-23 19:48 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha, Joseph Myers On 23/05/23 16:19, Sergey Bugaev wrote: > Hello, > > thank you for taking a look. > > On Tue, May 23, 2023 at 9:40 PM Adhemerval Zanella Netto > <adhemerval.zanella@linaro.org> wrote: >>> + res = __fcntl (lockfd1, F_OFD_GETLK, &flock); >> >> This is an external case, so use the external default name. > > Do you mean that this should be s/__fcntl/fcntl/? The __fnctl is not provided in all build configurations, so debug tests fail to build: debug/tst-fortify-cc-default-1-def.o: ./tst-fortify.c:1800:9: error: ‘__fcntl’ was not declared in this scope; did you mean ‘fcntl’? 1800 | res = __fcntl (lockfd1, F_OFD_GETLK, &flock); | ^~~~~~~ | fcntl > > If that is what you mean -- here's why I used __fcntl here, but not in > all the other places: > > The point of the OFD tests in the patch is to check for 32-bit vs > 64-bit confusion in the fortification. If, for example, in some > configuration the plain fcntl is supposed to be 32-bit (and struct > flock is 32-bit), but the fortified version of fcntl mistakenly > forwards to fcntl64, then the 32-bit struct flock will not be > converted to struct flock64, and will be passed to the kernel as-is. > It will yield some garbage when interpreted as a struct flock64, and > the kernel will likely reject it with an error (EINVAL, perhaps) -- > which is what the test checks against. But we already have different tests for non-LFS and LFS on debug, so any issue will be reported a test failure (including wrong fortification redirection). > > But the test also needs to work on the kernel versions that do not > support OFD locks, so it does this check for support being present at > all first, and skips running the OFD tests if the kernel does not > support it. But how do you check for OFD locks support? -- you try to > F_OFD_GETLK and see if the kernel returns EINVAL. If it does return > that, OFD locks are unsupported. > > And herein lies the problem, we cannot differentiate between EINVALs > caused by missing kernel support from EINVALs caused by the very kind > of bugs this test checks against! We'd mistake the actual issue we > wanted to catch for missing kernel support, skip the bulk of the test, > and report success. > > So in this first check of whether or not OFD locks are supported at > all, I've used __fcntl, which does not get fortified and so cannot > suffer from the 32-bit vs 64-bit confusion bug. The rest of the code > uses the regular fcntl. There's a comment above briefly mentioning > this, too -- clearly it has to be expanded. > > So, with this in mind, should I be using __fcntl here, or is there a > better option? I think a better option would to move the kernel probe support to libsupport and call it instead. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH] debug: Add tests for fortified fcntl () 2023-05-23 19:48 ` Adhemerval Zanella Netto @ 2023-05-24 7:15 ` Sergey Bugaev 2023-05-24 12:15 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-24 7:15 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha, Joseph Myers Hello, On Tue, May 23, 2023 at 10:49 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > The __fnctl is not provided in all build configurations, so debug tests > fail to build: > > debug/tst-fortify-cc-default-1-def.o: > > ./tst-fortify.c:1800:9: error: ‘__fcntl’ was not declared in this scope; did you mean ‘fcntl’? > 1800 | res = __fcntl (lockfd1, F_OFD_GETLK, &flock); > | ^~~~~~~ > | fcntl I see, thanks :| > But we already have different tests for non-LFS and LFS on debug, > so any issue will be reported a test failure (including wrong > fortification redirection). Could you please point out which tests you're talking about? I've only seen tst-fortify get built/checked in all the various configurations (_FORTIFY_SOURCE or not, _GNU_SOURCE or not, _FILE_OFFSET_BITS=64 or not, _LARGEFILE64_SOURCE or not). > > So, with this in mind, should I be using __fcntl here, or is there a > > better option? > > I think a better option would to move the kernel probe support to libsupport > and call it instead. That makes a lot of sense, will do, thanks. Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH] debug: Add tests for fortified fcntl () 2023-05-24 7:15 ` Sergey Bugaev @ 2023-05-24 12:15 ` Adhemerval Zanella Netto 0 siblings, 0 replies; 31+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-24 12:15 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha, Joseph Myers On 24/05/23 04:15, Sergey Bugaev wrote: > Hello, > > On Tue, May 23, 2023 at 10:49 PM Adhemerval Zanella Netto > <adhemerval.zanella@linaro.org> wrote: >> The __fnctl is not provided in all build configurations, so debug tests >> fail to build: >> >> debug/tst-fortify-cc-default-1-def.o: >> >> ./tst-fortify.c:1800:9: error: ‘__fcntl’ was not declared in this scope; did you mean ‘fcntl’? >> 1800 | res = __fcntl (lockfd1, F_OFD_GETLK, &flock); >> | ^~~~~~~ >> | fcntl > > I see, thanks :| > >> But we already have different tests for non-LFS and LFS on debug, >> so any issue will be reported a test failure (including wrong >> fortification redirection). > > Could you please point out which tests you're talking about? I've only > seen tst-fortify get built/checked in all the various configurations > (_FORTIFY_SOURCE or not, _GNU_SOURCE or not, _FILE_OFFSET_BITS=64 or > not, _LARGEFILE64_SOURCE or not). For ABI with non native 64 bit time_t (i686-linux-gnu for instance) we also have _TIME_BITS=64. But I think the issues of wrong redirection by the fortify itself is not easily caught without dumping the binary symbol table and check which function is actually generated by compiler. > >>> So, with this in mind, should I be using __fcntl here, or is there a >>> better option? >> >> I think a better option would to move the kernel probe support to libsupport >> and call it instead. > > That makes a lot of sense, will do, thanks. > > Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev 2023-05-19 21:55 ` Joseph Myers @ 2023-05-23 19:09 ` Adhemerval Zanella Netto 2023-05-23 19:43 ` Sergey Bugaev 2023-05-23 19:15 ` Siddhesh Poyarekar 2023-05-23 21:46 ` Florian Weimer 3 siblings, 1 reply; 31+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-23 19:09 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha; +Cc: Hector Martin On 19/05/23 18:30, Sergey Bugaev via Libc-alpha wrote: > Both open () and fcntl () are "overloaded" to accept either 2 or 3 > arguments; whether the last argument is required depends on the values > of the previous arguments. Since C provides no native support for > function overloading, this is implemented by making these functions > vararg. Unfortunately, this means the compiler is unable to check the > number of arguments and their types for correctness at compile time, and > will not diagnose any mistakes. > > To help with this, when FORTIFY_SOURCE is enabled, the special fcntl2.h > header replaces open () with a wrapper that checks the passed number of > arguments, raising a compile-time or run-time error on mismatch. This > commit adds similar handling for fcntl (). > > Recently, Hector Martin <marcan@marcan.st> has identified an issue in > libwebrtc where fcntl (fd, F_DUPFD_CLOEXEC) was invoked without a third > argument [0]. With the patch, the bug would have been detected at > compile time, assuming libwebrtc is built with FORTIFY_SOURCE. Hopefully > this will help detecting similar bugs in existing software, and prevent > more instances of similar bugs from being introduced in the future. > > [0]: https://social.treehouse.systems/@marcan/110355001391961285 > > The abilists have been modified with 'make update-abi-all'. > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> > --- > include/fcntl.h | 3 + > io/Makefile | 2 +- > io/Versions | 4 + > io/bits/fcntl2.h | 139 ++++++++++++++++++ > io/fcntl64_2.c | 33 +++++ > io/fcntl_2.c | 33 +++++ > sysdeps/mach/hurd/i386/libc.abilist | 2 + > sysdeps/mach/hurd/x86_64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/aarch64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/alpha/libc.abilist | 2 + > sysdeps/unix/sysv/linux/arc/libc.abilist | 2 + > sysdeps/unix/sysv/linux/arm/be/libc.abilist | 2 + > sysdeps/unix/sysv/linux/arm/le/libc.abilist | 2 + > sysdeps/unix/sysv/linux/csky/libc.abilist | 2 + > sysdeps/unix/sysv/linux/hppa/libc.abilist | 2 + > sysdeps/unix/sysv/linux/i386/libc.abilist | 2 + > sysdeps/unix/sysv/linux/ia64/libc.abilist | 2 + > .../sysv/linux/loongarch/lp64/libc.abilist | 2 + > .../sysv/linux/m68k/coldfire/libc.abilist | 2 + > .../unix/sysv/linux/m68k/m680x0/libc.abilist | 2 + > .../sysv/linux/microblaze/be/libc.abilist | 2 + > .../sysv/linux/microblaze/le/libc.abilist | 2 + > .../sysv/linux/mips/mips32/fpu/libc.abilist | 2 + > .../sysv/linux/mips/mips32/nofpu/libc.abilist | 2 + > .../sysv/linux/mips/mips64/n32/libc.abilist | 2 + > .../sysv/linux/mips/mips64/n64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/nios2/libc.abilist | 2 + > sysdeps/unix/sysv/linux/or1k/libc.abilist | 2 + > .../linux/powerpc/powerpc32/fpu/libc.abilist | 2 + > .../powerpc/powerpc32/nofpu/libc.abilist | 2 + > .../linux/powerpc/powerpc64/be/libc.abilist | 2 + > .../linux/powerpc/powerpc64/le/libc.abilist | 2 + > .../unix/sysv/linux/riscv/rv32/libc.abilist | 2 + > .../unix/sysv/linux/riscv/rv64/libc.abilist | 2 + > .../unix/sysv/linux/s390/s390-32/libc.abilist | 2 + > .../unix/sysv/linux/s390/s390-64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/sh/be/libc.abilist | 2 + > sysdeps/unix/sysv/linux/sh/le/libc.abilist | 2 + > .../sysv/linux/sparc/sparc32/libc.abilist | 2 + > .../sysv/linux/sparc/sparc64/libc.abilist | 2 + > .../unix/sysv/linux/x86_64/64/libc.abilist | 2 + > .../unix/sysv/linux/x86_64/x32/libc.abilist | 2 + > 42 files changed, 285 insertions(+), 1 deletion(-) > create mode 100644 io/fcntl64_2.c > create mode 100644 io/fcntl_2.c > > diff --git a/include/fcntl.h b/include/fcntl.h > index d788db2e..7742c002 100644 > --- a/include/fcntl.h > +++ b/include/fcntl.h > @@ -32,6 +32,9 @@ extern int __open64_2 (const char *__path, int __oflag); > extern int __openat_2 (int __fd, const char *__path, int __oflag); > extern int __openat64_2 (int __fd, const char *__path, int __oflag); > > +extern int __fcntl_2 (int __fd, int __cmd); > +extern int __fcntl64_2 (int __fd, int __cmd); > + > > /* Makes open () & friends faster on the Hurd, but can only be used (without > altering user-visible behavior) when we're sure that the file we're opening > diff --git a/io/Makefile b/io/Makefile > index f72571cd..350807ee 100644 > --- a/io/Makefile > +++ b/io/Makefile > @@ -41,7 +41,7 @@ routines := \ > mkdir mkdirat \ > open open_2 open64 open64_2 openat openat_2 openat64 openat64_2 \ > read write lseek lseek64 access euidaccess faccessat \ > - fcntl fcntl64 flock lockf lockf64 \ > + fcntl fcntl_2 fcntl64 fcntl64_2 flock lockf lockf64 \ > close dup dup2 dup3 pipe pipe2 \ > creat creat64 \ > chdir fchdir \ > diff --git a/io/Versions b/io/Versions > index 4e195408..8409e105 100644 > --- a/io/Versions > +++ b/io/Versions > @@ -140,6 +140,10 @@ libc { > GLIBC_2.34 { > closefrom; > } > + GLIBC_2.38 { > + __fcntl_2; > + __fcntl64_2; > + } > GLIBC_PRIVATE { > __libc_fcntl64; > __fcntl_nocancel; > diff --git a/io/bits/fcntl2.h b/io/bits/fcntl2.h > index bdb48fa8..beb3a628 100644 > --- a/io/bits/fcntl2.h > +++ b/io/bits/fcntl2.h > @@ -170,3 +170,142 @@ openat64 (int __fd, const char *__path, int __oflag, ...) > } > # endif > #endif > + > +#ifndef __USE_TIME_BITS64 > + > +# ifndef __USE_FILE_OFFSET64 > +/* Just fcntl (). */ Does this comment adds anything? Also I think we avoid to use '()' on function definition on comments. > +extern int __fcntl_2 (int __fd, int __cmd); > +extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl); > +# else > +/* fcntl.h redirects fcntl () to fcntl64 (). */ > +extern int __REDIRECT (__fcntl_2, (int __fd, int __cmd), __fcntl64_2); > +extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl64); > +# endif /* __USE_FILE_OFFSET64 */ > + > +# ifdef __USE_LARGEFILE64 > +/* Just fcntl64 (). */ > +extern int __fcntl64_2 (int __fd, int __cmd); > +extern int __REDIRECT (__fcntl64_alias, (int __fd, int __cmd, ...), fcntl64); > +# endif > + > +#else /* __USE_TIME_BITS64 */ > + > +/* fcntl.h redirects both fcntl () and fcntl64 () to __fcntl_time64 (). */ > +extern int __fcntl64_2 (int __fd, int __cmd); > +extern int __REDIRECT (__fcntl_2, (int __fd, int __cmd), __fcntl64_2); > +extern int __REDIRECT_NTH (__fcntl_alias, (int __fd, int __cmd, ...), > + __fcntl_time64); > +extern int __REDIRECT_NTH (__fcntl64_alias, (int __fd, int __cmd, ...), > + __fcntl_time64); > +#endif /* __USE_TIME_BITS64 */ > + > +__errordecl (__fcntl_too_many_args, > + "fcntl can be called either with 2 or 3 arguments, not more"); > +__errordecl (__fcntl_missing_arg, > + "fcntl with with this cmd needs 3 arguments"); > + > +__extern_always_inline int Should we use __fortify_function also for this case? > +__fcntl_cmd_needs_arg (int __cmd) > +{ > + switch (__cmd) > + { > + case F_DUPFD: > + case F_DUPFD_CLOEXEC: > + case F_SETFD: > + case F_SETFL: > +#ifdef F_SETLK > + case F_SETLK: > + case F_SETLKW: > + case F_GETLK: > +#endif > +#ifdef F_OFD_SETLK > + case F_OFD_SETLK: > + case F_OFD_SETLKW: > + case F_OFD_GETLK: > +#endif > +#ifdef F_SETOWN > + case F_SETOWN: > +#endif > +#ifdef __F_GETOWN_EX > + case __F_GETOWN_EX: > + case __F_SETOWN_EX: > + case __F_SETSIG: > +#endif I think it would be better to condicionalize using __USE_GNU: #ifdef __USE_GNU case F_GETOWN_EX: case F_SETOWN_EX: case F_SETSIG: #endif Since the double underscore symbols are implementation defined one and should not be used by the application. > +#ifdef F_SETLEASE > + case F_SETLEASE: > + case F_NOTIFY: > + case F_SETPIPE_SZ: > + case F_ADD_SEALS: > + case F_GET_RW_HINT: > + case F_SET_RW_HINT: > + case F_GET_FILE_RW_HINT: > + case F_SET_FILE_RW_HINT: > +#endif > + return 1; > + > + case F_GETFD: > + case F_GETFL: > +#ifdef F_GETOWN > + case F_GETOWN: > +#endif > +#ifdef __F_GETSIG > + case __F_GETSIG: > +#endif > +#ifdef F_GETLEASE > + case F_GETLEASE: > + case F_GETPIPE_SZ: > + case F_GET_SEALS: > +#endif > + /* For any cmd value we don't know about, > + default to not requiring an argument. */ > + default: > + return 0; > + } > +} Should be worried that since we don't have const expressions like newer C++ that large switch might ended being extra code on every fcntl call? I think the __builtin_constant_p check below gives compiler enough information to avoid it, but I am not sure. > + > +__fortify_function int > +fcntl (int __fd, int __cmd, ...) > +{ > + if (__va_arg_pack_len () > 1) > + __fcntl_too_many_args (); > + > + if (__builtin_constant_p (__cmd)) > + { > + 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 ...) > + { > + __fcntl_missing_arg (); > + return __fcntl_2 (__fd, __cmd); > + } > + return __fcntl_alias (__fd, __cmd, __va_arg_pack ()); > + } > + > + if (__va_arg_pack_len () < 1) > + return __fcntl_2 (__fd, __cmd); > + > + return __fcntl_alias (__fd, __cmd, __va_arg_pack ()); > +} > + > +#ifdef __USE_LARGEFILE64 > +__fortify_function int > +fcntl64 (int __fd, int __cmd, ...) > +{ > + if (__va_arg_pack_len () > 1) > + __fcntl_too_many_args (); > + > + if (__builtin_constant_p (__cmd)) > + { > + if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1) > + { > + __fcntl_missing_arg (); > + return __fcntl64_2 (__fd, __cmd); > + } > + return __fcntl64_alias (__fd, __cmd, __va_arg_pack ()); > + } > + > + if (__va_arg_pack_len () < 1) > + return __fcntl64_2 (__fd, __cmd); > + > + return __fcntl64_alias (__fd, __cmd, __va_arg_pack ()); > +} > +#endif I think we need also to handle __USE_TIME_BITS64 and add another fcntl debug symbol to handle; even though for Linux now __fcntl_time64 is just a alias to __libc_fcntl64. It is because we even need to add a proper __fcntl_time64 implementation, the redirection now does: fcntl -> __fcntl64_2 -> __libc_fcntl64 Where it should be something like: fcntl -> __fcntl_time64_2 -> __fcntl_time64 > diff --git a/io/fcntl64_2.c b/io/fcntl64_2.c > new file mode 100644 > index 00000000..6352f524 > --- /dev/null > +++ b/io/fcntl64_2.c > @@ -0,0 +1,33 @@ > +/* _FORTIFY_SOURCE wrapper for fcntl64. > + Copyright (C) 2013-2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +/* Make sure to get __fcntl_cmd_needs_arg from bits/fcntl2.h */ > +#undef _FORTIFY_SOURCE > +#define _FORTIFY_SOURCE 1 > + > +#include <fcntl.h> > +#include <stdio.h> > + > +int > +__fcntl64_2 (int fd, int cmd) > +{ > + if (__fcntl_cmd_needs_arg (cmd)) > + __fortify_fail ("invalid fcntl64 call: this cmd requires an argument"); > + > + return __libc_fcntl64 (fd, cmd); > +} Ok. > diff --git a/io/fcntl_2.c b/io/fcntl_2.c > new file mode 100644 > index 00000000..fc9303e4 > --- /dev/null > +++ b/io/fcntl_2.c > @@ -0,0 +1,33 @@ > +/* _FORTIFY_SOURCE wrapper for fcntl. > + Copyright (C) 2013-2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +/* Make sure to get __fcntl_cmd_needs_arg from bits/fcntl2.h */ > +#undef _FORTIFY_SOURCE > +#define _FORTIFY_SOURCE 1 > + > +#include <fcntl.h> > +#include <stdio.h> > + > +int > +__fcntl_2 (int fd, int cmd) > +{ > + if (__fcntl_cmd_needs_arg (cmd)) > + __fortify_fail ("invalid fcntl call: this cmd requires an argument"); > + > + return __libc_fcntl (fd, cmd); > +} Ok. > diff --git a/sysdeps/mach/hurd/i386/libc.abilist b/sysdeps/mach/hurd/i386/libc.abilist > index 6925222f..fa24d577 100644 > --- a/sysdeps/mach/hurd/i386/libc.abilist > +++ b/sysdeps/mach/hurd/i386/libc.abilist > @@ -2294,6 +2294,8 @@ GLIBC_2.36 arc4random_buf F > GLIBC_2.36 arc4random_uniform F > GLIBC_2.36 c8rtomb F > GLIBC_2.36 mbrtoc8 F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/mach/hurd/x86_64/libc.abilist b/sysdeps/mach/hurd/x86_64/libc.abilist > index a0be5c1a..efb83b16 100644 > --- a/sysdeps/mach/hurd/x86_64/libc.abilist > +++ b/sysdeps/mach/hurd/x86_64/libc.abilist > @@ -194,6 +194,8 @@ GLIBC_2.38 __errno_location F > GLIBC_2.38 __explicit_bzero_chk F > GLIBC_2.38 __fbufsize F > GLIBC_2.38 __fcntl F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __fdelt_chk F > GLIBC_2.38 __fdelt_warn F > GLIBC_2.38 __fentry__ F > diff --git a/sysdeps/unix/sysv/linux/aarch64/libc.abilist b/sysdeps/unix/sysv/linux/aarch64/libc.abilist > index 0e2d9c30..86144979 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist > @@ -2633,6 +2633,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/alpha/libc.abilist b/sysdeps/unix/sysv/linux/alpha/libc.abilist > index f1bec197..646c11e3 100644 > --- a/sysdeps/unix/sysv/linux/alpha/libc.abilist > +++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist > @@ -2730,6 +2730,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/arc/libc.abilist b/sysdeps/unix/sysv/linux/arc/libc.abilist > index aa874b88..37e9129f 100644 > --- a/sysdeps/unix/sysv/linux/arc/libc.abilist > +++ b/sysdeps/unix/sysv/linux/arc/libc.abilist > @@ -2394,6 +2394,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/arm/be/libc.abilist b/sysdeps/unix/sysv/linux/arm/be/libc.abilist > index afbd57da..4c9b937e 100644 > --- a/sysdeps/unix/sysv/linux/arm/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/arm/be/libc.abilist > @@ -514,6 +514,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/arm/le/libc.abilist b/sysdeps/unix/sysv/linux/arm/le/libc.abilist > index e7364cd3..8dbf72c5 100644 > --- a/sysdeps/unix/sysv/linux/arm/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/arm/le/libc.abilist > @@ -511,6 +511,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/csky/libc.abilist b/sysdeps/unix/sysv/linux/csky/libc.abilist > index 913fa592..b58b2b9c 100644 > --- a/sysdeps/unix/sysv/linux/csky/libc.abilist > +++ b/sysdeps/unix/sysv/linux/csky/libc.abilist > @@ -2670,6 +2670,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/hppa/libc.abilist b/sysdeps/unix/sysv/linux/hppa/libc.abilist > index 43af3a98..c80bb835 100644 > --- a/sysdeps/unix/sysv/linux/hppa/libc.abilist > +++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist > @@ -2619,6 +2619,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/i386/libc.abilist b/sysdeps/unix/sysv/linux/i386/libc.abilist > index af72f8fa..5a737b3d 100644 > --- a/sysdeps/unix/sysv/linux/i386/libc.abilist > +++ b/sysdeps/unix/sysv/linux/i386/libc.abilist > @@ -2803,6 +2803,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/ia64/libc.abilist b/sysdeps/unix/sysv/linux/ia64/libc.abilist > index 48cbb0fa..aab6d422 100644 > --- a/sysdeps/unix/sysv/linux/ia64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist > @@ -2568,6 +2568,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist > index c15884bb..82da98d2 100644 > --- a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist > @@ -2154,6 +2154,8 @@ GLIBC_2.36 wprintf F > GLIBC_2.36 write F > GLIBC_2.36 writev F > GLIBC_2.36 wscanf F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist > index 3738db81..f6c80b46 100644 > --- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist > +++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist > @@ -515,6 +515,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist > index ed136277..dc34ea56 100644 > --- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist > +++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist > @@ -2746,6 +2746,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist > index 83577386..466e4092 100644 > --- a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist > @@ -2719,6 +2719,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist > index 58c5da58..dc27b4e7 100644 > --- a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist > @@ -2716,6 +2716,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist > index d3741945..a8dfa647 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist > @@ -2711,6 +2711,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist > index 5319fdc2..4defd8dc 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist > @@ -2709,6 +2709,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist > index 1743ea6e..824b6b4c 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist > @@ -2717,6 +2717,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist > index 9b1f53c6..ff4341a8 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist > @@ -2619,6 +2619,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/nios2/libc.abilist b/sysdeps/unix/sysv/linux/nios2/libc.abilist > index ae1c6ca1..ede0e6eb 100644 > --- a/sysdeps/unix/sysv/linux/nios2/libc.abilist > +++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist > @@ -2758,6 +2758,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/or1k/libc.abilist b/sysdeps/unix/sysv/linux/or1k/libc.abilist > index a7c572c9..22525fe8 100644 > --- a/sysdeps/unix/sysv/linux/or1k/libc.abilist > +++ b/sysdeps/unix/sysv/linux/or1k/libc.abilist > @@ -2140,6 +2140,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist > index 074fa031..2938361e 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist > @@ -2773,6 +2773,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist > index dfcb4bd2..e1c70a25 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist > @@ -2806,6 +2806,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist > index 63bbccf3..2ce2d974 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist > @@ -2527,6 +2527,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist > index ab85fd61..b1024501 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist > @@ -2829,6 +2829,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fscanfieee128 F > GLIBC_2.38 __isoc23_fwscanf F > diff --git a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist > index b716f5c7..833fb747 100644 > --- a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist > @@ -2396,6 +2396,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist > index 774e777b..cbd85454 100644 > --- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist > @@ -2596,6 +2596,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist > index 8625135c..1442a0b9 100644 > --- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist > @@ -2771,6 +2771,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist > index d00c7eb2..1a7f4fc7 100644 > --- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist > @@ -2564,6 +2564,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sh/be/libc.abilist b/sysdeps/unix/sysv/linux/sh/be/libc.abilist > index b6303724..3146ce99 100644 > --- a/sysdeps/unix/sysv/linux/sh/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sh/be/libc.abilist > @@ -2626,6 +2626,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sh/le/libc.abilist b/sysdeps/unix/sysv/linux/sh/le/libc.abilist > index d8005561..31360691 100644 > --- a/sysdeps/unix/sysv/linux/sh/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sh/le/libc.abilist > @@ -2623,6 +2623,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist > index 5be55c11..026e9581 100644 > --- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist > @@ -2766,6 +2766,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist > index 475fdaae..6ecb0d46 100644 > --- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist > @@ -2591,6 +2591,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist > index 6cfb928b..a37f4ab4 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist > @@ -2542,6 +2542,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist > index c7350971..3b8790f4 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist > @@ -2648,6 +2648,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 19:09 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Adhemerval Zanella Netto @ 2023-05-23 19:43 ` Sergey Bugaev 2023-05-23 19:56 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-23 19:43 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha On Tue, May 23, 2023 at 10:09 PM Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote: > > +# ifndef __USE_FILE_OFFSET64 > > +/* Just fcntl (). */ > > Does this comment adds anything? Also I think we avoid to use '()' on function > definition on comments. These comments were mainly for myself to keep track of the various possible configurations -- I decided to leave them in since the reader of the code might get a little overwhelmed by the combinatorial explosion of the possible configuration variants as well. I can remove these comments (or just this one?) if you would prefer that. > > +__extern_always_inline int > > Should we use __fortify_function also for this case? I think __fortify_function is supposed to be used on the replacement functions themselves, not their helpers? if that's not the case, sure. > > +#ifdef __F_GETOWN_EX > > + case __F_GETOWN_EX: > > + case __F_SETOWN_EX: > > + case __F_SETSIG: > > +#endif > > I think it would be better to condicionalize using __USE_GNU: > > #ifdef __USE_GNU > case F_GETOWN_EX: > case F_SETOWN_EX: > case F_SETSIG: > #endif > > Since the double underscore symbols are implementation defined one and should > not be used by the application. But that would break on the Hurd, no? I could do #ifdef F_GETOWN_EX (without the double underscore), but I wanted to handle __F_GETOWN_EX etc. even if the user has not enabled them (better handle them than not). In other words, this ifdef is meant to check for Hurd vs Linux, not for __USE_GNU vs not. > Should be worried that since we don't have const expressions like newer C++ > that large switch might ended being extra code on every fcntl call? I think > the __builtin_constant_p check below gives compiler enough information to > avoid it, but I am not sure. Yes, __builtin_constant_p is, AFAIK, some compiler magic that "guarantees" (quoted since I'm not sure if it's actually guaranteed, but I've never seen it behave otherwise) that in the true branch the value is treated as a compile-time constant. In practice (see what the tst-fortify compiles to!), with -O2 this whole thing is fully optimized away, and the generated code contains the very same calls to fcntl (or fcntl64, or whatever) that it would if there never was any fortification -- except you get compile-time errors if you forget the argument when it's required. So there's zero run-time cost. If the cmd argument is a c-t const, that is -- otherwise it calls the _2 versions and does the check at runtime, yes. But that's only extra code (as in code size) once, in glibc, not at the call sites. > > + 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? > I think we need also to handle __USE_TIME_BITS64 and add another fcntl debug > symbol to handle; even though for Linux now __fcntl_time64 is just a alias > to __libc_fcntl64. > > It is because we even need to add a proper __fcntl_time64 implementation, > the redirection now does: > > fcntl -> __fcntl64_2 -> __libc_fcntl64 > > Where it should be something like: > > fcntl -> __fcntl_time64_2 -> __fcntl_time64 The idea here was that the 2-argument version of fcntl clearly does not deal with time, either 32- or 64-bit... Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 19:43 ` Sergey Bugaev @ 2023-05-23 19:56 ` Adhemerval Zanella Netto 2023-05-23 20:24 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-23 19:56 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha On 23/05/23 16:43, Sergey Bugaev wrote: > On Tue, May 23, 2023 at 10:09 PM Adhemerval Zanella Netto > <adhemerval.zanella@linaro.org> wrote: >>> +# ifndef __USE_FILE_OFFSET64 >>> +/* Just fcntl (). */ >> >> Does this comment adds anything? Also I think we avoid to use '()' on function >> definition on comments. > > These comments were mainly for myself to keep track of the various > possible configurations -- I decided to leave them in since the reader > of the code might get a little overwhelmed by the combinatorial > explosion of the possible configuration variants as well. > > I can remove these comments (or just this one?) if you would prefer that. I am chiming in because it is an installed header, but this should be ok. > >>> +__extern_always_inline int >> >> Should we use __fortify_function also for this case? > > I think __fortify_function is supposed to be used on the replacement > functions themselves, not their helpers? if that's not the case, sure. The __fortify_function mainly adds the artificial attribute, which improves the error message to make on the call site instead of the decorated function. If the error messages from wrong fnctl usage are ok, I think we can skip the __fortify_function usage here. > >>> +#ifdef __F_GETOWN_EX >>> + case __F_GETOWN_EX: >>> + case __F_SETOWN_EX: >>> + case __F_SETSIG: >>> +#endif >> >> I think it would be better to condicionalize using __USE_GNU: >> >> #ifdef __USE_GNU >> case F_GETOWN_EX: >> case F_SETOWN_EX: >> case F_SETSIG: >> #endif >> >> Since the double underscore symbols are implementation defined one and should >> not be used by the application. > > But that would break on the Hurd, no? > > I could do #ifdef F_GETOWN_EX (without the double underscore), but I > wanted to handle __F_GETOWN_EX etc. even if the user has not enabled > them (better handle them than not). In other words, this ifdef is > meant to check for Hurd vs Linux, not for __USE_GNU vs not. But user will actually use F_GETOWN_EX, not __F_GETOWN_EX; so I think check for F_GETOWN_EX define as well. > >> Should be worried that since we don't have const expressions like newer C++ >> that large switch might ended being extra code on every fcntl call? I think >> the __builtin_constant_p check below gives compiler enough information to >> avoid it, but I am not sure. > > Yes, __builtin_constant_p is, AFAIK, some compiler magic that > "guarantees" (quoted since I'm not sure if it's actually guaranteed, > but I've never seen it behave otherwise) that in the true branch the > value is treated as a compile-time constant. > > In practice (see what the tst-fortify compiles to!), with -O2 this > whole thing is fully optimized away, and the generated code contains > the very same calls to fcntl (or fcntl64, or whatever) that it would > if there never was any fortification -- except you get compile-time > errors if you forget the argument when it's required. So there's zero > run-time cost. > > If the cmd argument is a c-t const, that is -- otherwise it calls the > _2 versions and does the check at runtime, yes. But that's only extra > code (as in code size) once, in glibc, not at the call sites. Fair enough. > >>> + 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 think we need also to handle __USE_TIME_BITS64 and add another fcntl debug >> symbol to handle; even though for Linux now __fcntl_time64 is just a alias >> to __libc_fcntl64. >> >> It is because we even need to add a proper __fcntl_time64 implementation, >> the redirection now does: >> >> fcntl -> __fcntl64_2 -> __libc_fcntl64 >> >> Where it should be something like: >> >> fcntl -> __fcntl_time64_2 -> __fcntl_time64 > > 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 19:56 ` Adhemerval Zanella Netto @ 2023-05-23 20:24 ` Sergey Bugaev 2023-05-23 20:44 ` Sergey Bugaev 2023-05-24 12:04 ` Adhemerval Zanella Netto 0 siblings, 2 replies; 31+ messages in thread From: Sergey Bugaev @ 2023-05-23 20:24 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha 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 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. Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 20:24 ` Sergey Bugaev @ 2023-05-23 20:44 ` Sergey Bugaev 2023-05-24 12:04 ` Adhemerval Zanella Netto 1 sibling, 0 replies; 31+ messages in thread From: Sergey Bugaev @ 2023-05-23 20:44 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha On Tue, May 23, 2023, 23:24 Sergey Bugaev <bugaevc@gmail.com> wrote: > 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. And now that I think of it, there's also no reason to have separate __fcntl64_2 & __fcntl_2 functions either, is there? Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 20:24 ` Sergey Bugaev 2023-05-23 20:44 ` Sergey Bugaev @ 2023-05-24 12:04 ` Adhemerval Zanella Netto 1 sibling, 0 replies; 31+ messages in thread From: Adhemerval Zanella Netto @ 2023-05-24 12:04 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha 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. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev 2023-05-19 21:55 ` Joseph Myers 2023-05-23 19:09 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Adhemerval Zanella Netto @ 2023-05-23 19:15 ` Siddhesh Poyarekar 2023-05-23 20:01 ` Sergey Bugaev 2023-05-23 21:46 ` Florian Weimer 3 siblings, 1 reply; 31+ messages in thread From: Siddhesh Poyarekar @ 2023-05-23 19:15 UTC (permalink / raw) To: Sergey Bugaev, libc-alpha; +Cc: Hector Martin On 2023-05-19 17:30, Sergey Bugaev via Libc-alpha wrote: > Both open () and fcntl () are "overloaded" to accept either 2 or 3 > arguments; whether the last argument is required depends on the values > of the previous arguments. Since C provides no native support for > function overloading, this is implemented by making these functions > vararg. Unfortunately, this means the compiler is unable to check the > number of arguments and their types for correctness at compile time, and > will not diagnose any mistakes. > > To help with this, when FORTIFY_SOURCE is enabled, the special fcntl2.h > header replaces open () with a wrapper that checks the passed number of > arguments, raising a compile-time or run-time error on mismatch. This > commit adds similar handling for fcntl (). > > Recently, Hector Martin <marcan@marcan.st> has identified an issue in > libwebrtc where fcntl (fd, F_DUPFD_CLOEXEC) was invoked without a third > argument [0]. With the patch, the bug would have been detected at > compile time, assuming libwebrtc is built with FORTIFY_SOURCE. Hopefully > this will help detecting similar bugs in existing software, and prevent > more instances of similar bugs from being introduced in the future. > > [0]: https://social.treehouse.systems/@marcan/110355001391961285 > > The abilists have been modified with 'make update-abi-all'. > > Signed-off-by: Sergey Bugaev <bugaevc@gmail.com> > --- > include/fcntl.h | 3 + > io/Makefile | 2 +- > io/Versions | 4 + > io/bits/fcntl2.h | 139 ++++++++++++++++++ > io/fcntl64_2.c | 33 +++++ > io/fcntl_2.c | 33 +++++ > sysdeps/mach/hurd/i386/libc.abilist | 2 + > sysdeps/mach/hurd/x86_64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/aarch64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/alpha/libc.abilist | 2 + > sysdeps/unix/sysv/linux/arc/libc.abilist | 2 + > sysdeps/unix/sysv/linux/arm/be/libc.abilist | 2 + > sysdeps/unix/sysv/linux/arm/le/libc.abilist | 2 + > sysdeps/unix/sysv/linux/csky/libc.abilist | 2 + > sysdeps/unix/sysv/linux/hppa/libc.abilist | 2 + > sysdeps/unix/sysv/linux/i386/libc.abilist | 2 + > sysdeps/unix/sysv/linux/ia64/libc.abilist | 2 + > .../sysv/linux/loongarch/lp64/libc.abilist | 2 + > .../sysv/linux/m68k/coldfire/libc.abilist | 2 + > .../unix/sysv/linux/m68k/m680x0/libc.abilist | 2 + > .../sysv/linux/microblaze/be/libc.abilist | 2 + > .../sysv/linux/microblaze/le/libc.abilist | 2 + > .../sysv/linux/mips/mips32/fpu/libc.abilist | 2 + > .../sysv/linux/mips/mips32/nofpu/libc.abilist | 2 + > .../sysv/linux/mips/mips64/n32/libc.abilist | 2 + > .../sysv/linux/mips/mips64/n64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/nios2/libc.abilist | 2 + > sysdeps/unix/sysv/linux/or1k/libc.abilist | 2 + > .../linux/powerpc/powerpc32/fpu/libc.abilist | 2 + > .../powerpc/powerpc32/nofpu/libc.abilist | 2 + > .../linux/powerpc/powerpc64/be/libc.abilist | 2 + > .../linux/powerpc/powerpc64/le/libc.abilist | 2 + > .../unix/sysv/linux/riscv/rv32/libc.abilist | 2 + > .../unix/sysv/linux/riscv/rv64/libc.abilist | 2 + > .../unix/sysv/linux/s390/s390-32/libc.abilist | 2 + > .../unix/sysv/linux/s390/s390-64/libc.abilist | 2 + > sysdeps/unix/sysv/linux/sh/be/libc.abilist | 2 + > sysdeps/unix/sysv/linux/sh/le/libc.abilist | 2 + > .../sysv/linux/sparc/sparc32/libc.abilist | 2 + > .../sysv/linux/sparc/sparc64/libc.abilist | 2 + > .../unix/sysv/linux/x86_64/64/libc.abilist | 2 + > .../unix/sysv/linux/x86_64/x32/libc.abilist | 2 + > 42 files changed, 285 insertions(+), 1 deletion(-) > create mode 100644 io/fcntl64_2.c > create mode 100644 io/fcntl_2.c > > diff --git a/include/fcntl.h b/include/fcntl.h > index d788db2e..7742c002 100644 > --- a/include/fcntl.h > +++ b/include/fcntl.h > @@ -32,6 +32,9 @@ extern int __open64_2 (const char *__path, int __oflag); > extern int __openat_2 (int __fd, const char *__path, int __oflag); > extern int __openat64_2 (int __fd, const char *__path, int __oflag); > > +extern int __fcntl_2 (int __fd, int __cmd); > +extern int __fcntl64_2 (int __fd, int __cmd); > + With my Bikeshed Aesthetics Consultant hat on, the naming of fortified variants of file ops functions as *_2 is awkward, they should have been *_chk but I don't want to block this good work on that. The overall direction is good IMO (and I see Adhemerval is on it with a deeper review), just some notes: - manual/contrib.texi should be updated to mention fcntl - Internal users end up calling __libc_fcntl (see dup2.c or grantpt.c for example), which will essentially bypass any fortification. This is not a problem today since we don't build glibc with fortification, but Frederic Berat[1] has been experimenting with that and we're hoping to get at least a subset of glibc fortified for 2.38. It would be a shame to miss fortifying glibc itself. This is again not a problem that would block this patch, but something to be aware of. > > /* Makes open () & friends faster on the Hurd, but can only be used (without > altering user-visible behavior) when we're sure that the file we're opening > diff --git a/io/Makefile b/io/Makefile > index f72571cd..350807ee 100644 > --- a/io/Makefile > +++ b/io/Makefile > @@ -41,7 +41,7 @@ routines := \ > mkdir mkdirat \ > open open_2 open64 open64_2 openat openat_2 openat64 openat64_2 \ > read write lseek lseek64 access euidaccess faccessat \ > - fcntl fcntl64 flock lockf lockf64 \ > + fcntl fcntl_2 fcntl64 fcntl64_2 flock lockf lockf64 \ > close dup dup2 dup3 pipe pipe2 \ > creat creat64 \ > chdir fchdir \ > diff --git a/io/Versions b/io/Versions > index 4e195408..8409e105 100644 > --- a/io/Versions > +++ b/io/Versions > @@ -140,6 +140,10 @@ libc { > GLIBC_2.34 { > closefrom; > } > + GLIBC_2.38 { > + __fcntl_2; > + __fcntl64_2; > + } > GLIBC_PRIVATE { > __libc_fcntl64; > __fcntl_nocancel; > diff --git a/io/bits/fcntl2.h b/io/bits/fcntl2.h > index bdb48fa8..beb3a628 100644 > --- a/io/bits/fcntl2.h > +++ b/io/bits/fcntl2.h > @@ -170,3 +170,142 @@ openat64 (int __fd, const char *__path, int __oflag, ...) > } > # endif > #endif > + > +#ifndef __USE_TIME_BITS64 > + > +# ifndef __USE_FILE_OFFSET64 > +/* Just fcntl (). */ > +extern int __fcntl_2 (int __fd, int __cmd); > +extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl); > +# else > +/* fcntl.h redirects fcntl () to fcntl64 (). */ > +extern int __REDIRECT (__fcntl_2, (int __fd, int __cmd), __fcntl64_2); > +extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl64); > +# endif /* __USE_FILE_OFFSET64 */ > + > +# ifdef __USE_LARGEFILE64 > +/* Just fcntl64 (). */ > +extern int __fcntl64_2 (int __fd, int __cmd); > +extern int __REDIRECT (__fcntl64_alias, (int __fd, int __cmd, ...), fcntl64); > +# endif > + > +#else /* __USE_TIME_BITS64 */ > + > +/* fcntl.h redirects both fcntl () and fcntl64 () to __fcntl_time64 (). */ > +extern int __fcntl64_2 (int __fd, int __cmd); > +extern int __REDIRECT (__fcntl_2, (int __fd, int __cmd), __fcntl64_2); > +extern int __REDIRECT_NTH (__fcntl_alias, (int __fd, int __cmd, ...), > + __fcntl_time64); > +extern int __REDIRECT_NTH (__fcntl64_alias, (int __fd, int __cmd, ...), > + __fcntl_time64); > +#endif /* __USE_TIME_BITS64 */ > + > +__errordecl (__fcntl_too_many_args, > + "fcntl can be called either with 2 or 3 arguments, not more"); > +__errordecl (__fcntl_missing_arg, > + "fcntl with with this cmd needs 3 arguments"); > + > +__extern_always_inline int > +__fcntl_cmd_needs_arg (int __cmd) > +{ > + switch (__cmd) > + { > + case F_DUPFD: > + case F_DUPFD_CLOEXEC: > + case F_SETFD: > + case F_SETFL: > +#ifdef F_SETLK > + case F_SETLK: > + case F_SETLKW: > + case F_GETLK: > +#endif > +#ifdef F_OFD_SETLK > + case F_OFD_SETLK: > + case F_OFD_SETLKW: > + case F_OFD_GETLK: > +#endif > +#ifdef F_SETOWN > + case F_SETOWN: > +#endif > +#ifdef __F_GETOWN_EX > + case __F_GETOWN_EX: > + case __F_SETOWN_EX: > + case __F_SETSIG: > +#endif > +#ifdef F_SETLEASE > + case F_SETLEASE: > + case F_NOTIFY: > + case F_SETPIPE_SZ: > + case F_ADD_SEALS: > + case F_GET_RW_HINT: > + case F_SET_RW_HINT: > + case F_GET_FILE_RW_HINT: > + case F_SET_FILE_RW_HINT: > +#endif > + return 1; > + > + case F_GETFD: > + case F_GETFL: > +#ifdef F_GETOWN > + case F_GETOWN: > +#endif > +#ifdef __F_GETSIG > + case __F_GETSIG: > +#endif > +#ifdef F_GETLEASE > + case F_GETLEASE: > + case F_GETPIPE_SZ: > + case F_GET_SEALS: > +#endif > + /* For any cmd value we don't know about, > + default to not requiring an argument. */ > + default: > + return 0; > + } > +} > + > +__fortify_function int > +fcntl (int __fd, int __cmd, ...) > +{ > + if (__va_arg_pack_len () > 1) > + __fcntl_too_many_args (); > + > + if (__builtin_constant_p (__cmd)) > + { > + if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1) > + { > + __fcntl_missing_arg (); > + return __fcntl_2 (__fd, __cmd); > + } > + return __fcntl_alias (__fd, __cmd, __va_arg_pack ()); > + } > + > + if (__va_arg_pack_len () < 1) > + return __fcntl_2 (__fd, __cmd); > + > + return __fcntl_alias (__fd, __cmd, __va_arg_pack ()); > +} > + > +#ifdef __USE_LARGEFILE64 > +__fortify_function int > +fcntl64 (int __fd, int __cmd, ...) > +{ > + if (__va_arg_pack_len () > 1) > + __fcntl_too_many_args (); > + > + if (__builtin_constant_p (__cmd)) > + { > + if (__fcntl_cmd_needs_arg (__cmd) && __va_arg_pack_len () < 1) > + { > + __fcntl_missing_arg (); Given that this will error out at compile time, do we even need this __fcntl64_2 call? > + return __fcntl64_2 (__fd, __cmd); > + } > + return __fcntl64_alias (__fd, __cmd, __va_arg_pack ()); > + } > + > + if (__va_arg_pack_len () < 1) > + return __fcntl64_2 (__fd, __cmd); > + > + return __fcntl64_alias (__fd, __cmd, __va_arg_pack ()); > +} > +#endif > diff --git a/io/fcntl64_2.c b/io/fcntl64_2.c > new file mode 100644 > index 00000000..6352f524 > --- /dev/null > +++ b/io/fcntl64_2.c > @@ -0,0 +1,33 @@ > +/* _FORTIFY_SOURCE wrapper for fcntl64. > + Copyright (C) 2013-2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +/* Make sure to get __fcntl_cmd_needs_arg from bits/fcntl2.h */ > +#undef _FORTIFY_SOURCE > +#define _FORTIFY_SOURCE 1 > + > +#include <fcntl.h> > +#include <stdio.h> > + > +int > +__fcntl64_2 (int fd, int cmd) > +{ > + if (__fcntl_cmd_needs_arg (cmd)) > + __fortify_fail ("invalid fcntl64 call: this cmd requires an argument"); > + > + return __libc_fcntl64 (fd, cmd); > +} > diff --git a/io/fcntl_2.c b/io/fcntl_2.c > new file mode 100644 > index 00000000..fc9303e4 > --- /dev/null > +++ b/io/fcntl_2.c > @@ -0,0 +1,33 @@ > +/* _FORTIFY_SOURCE wrapper for fcntl. > + Copyright (C) 2013-2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > +/* Make sure to get __fcntl_cmd_needs_arg from bits/fcntl2.h */ > +#undef _FORTIFY_SOURCE > +#define _FORTIFY_SOURCE 1 > + > +#include <fcntl.h> > +#include <stdio.h> > + > +int > +__fcntl_2 (int fd, int cmd) > +{ > + if (__fcntl_cmd_needs_arg (cmd)) > + __fortify_fail ("invalid fcntl call: this cmd requires an argument"); > + > + return __libc_fcntl (fd, cmd); > +} > diff --git a/sysdeps/mach/hurd/i386/libc.abilist b/sysdeps/mach/hurd/i386/libc.abilist > index 6925222f..fa24d577 100644 > --- a/sysdeps/mach/hurd/i386/libc.abilist > +++ b/sysdeps/mach/hurd/i386/libc.abilist > @@ -2294,6 +2294,8 @@ GLIBC_2.36 arc4random_buf F > GLIBC_2.36 arc4random_uniform F > GLIBC_2.36 c8rtomb F > GLIBC_2.36 mbrtoc8 F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/mach/hurd/x86_64/libc.abilist b/sysdeps/mach/hurd/x86_64/libc.abilist > index a0be5c1a..efb83b16 100644 > --- a/sysdeps/mach/hurd/x86_64/libc.abilist > +++ b/sysdeps/mach/hurd/x86_64/libc.abilist > @@ -194,6 +194,8 @@ GLIBC_2.38 __errno_location F > GLIBC_2.38 __explicit_bzero_chk F > GLIBC_2.38 __fbufsize F > GLIBC_2.38 __fcntl F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __fdelt_chk F > GLIBC_2.38 __fdelt_warn F > GLIBC_2.38 __fentry__ F > diff --git a/sysdeps/unix/sysv/linux/aarch64/libc.abilist b/sysdeps/unix/sysv/linux/aarch64/libc.abilist > index 0e2d9c30..86144979 100644 > --- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist > @@ -2633,6 +2633,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/alpha/libc.abilist b/sysdeps/unix/sysv/linux/alpha/libc.abilist > index f1bec197..646c11e3 100644 > --- a/sysdeps/unix/sysv/linux/alpha/libc.abilist > +++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist > @@ -2730,6 +2730,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/arc/libc.abilist b/sysdeps/unix/sysv/linux/arc/libc.abilist > index aa874b88..37e9129f 100644 > --- a/sysdeps/unix/sysv/linux/arc/libc.abilist > +++ b/sysdeps/unix/sysv/linux/arc/libc.abilist > @@ -2394,6 +2394,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/arm/be/libc.abilist b/sysdeps/unix/sysv/linux/arm/be/libc.abilist > index afbd57da..4c9b937e 100644 > --- a/sysdeps/unix/sysv/linux/arm/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/arm/be/libc.abilist > @@ -514,6 +514,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/arm/le/libc.abilist b/sysdeps/unix/sysv/linux/arm/le/libc.abilist > index e7364cd3..8dbf72c5 100644 > --- a/sysdeps/unix/sysv/linux/arm/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/arm/le/libc.abilist > @@ -511,6 +511,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/csky/libc.abilist b/sysdeps/unix/sysv/linux/csky/libc.abilist > index 913fa592..b58b2b9c 100644 > --- a/sysdeps/unix/sysv/linux/csky/libc.abilist > +++ b/sysdeps/unix/sysv/linux/csky/libc.abilist > @@ -2670,6 +2670,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/hppa/libc.abilist b/sysdeps/unix/sysv/linux/hppa/libc.abilist > index 43af3a98..c80bb835 100644 > --- a/sysdeps/unix/sysv/linux/hppa/libc.abilist > +++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist > @@ -2619,6 +2619,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/i386/libc.abilist b/sysdeps/unix/sysv/linux/i386/libc.abilist > index af72f8fa..5a737b3d 100644 > --- a/sysdeps/unix/sysv/linux/i386/libc.abilist > +++ b/sysdeps/unix/sysv/linux/i386/libc.abilist > @@ -2803,6 +2803,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/ia64/libc.abilist b/sysdeps/unix/sysv/linux/ia64/libc.abilist > index 48cbb0fa..aab6d422 100644 > --- a/sysdeps/unix/sysv/linux/ia64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist > @@ -2568,6 +2568,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist > index c15884bb..82da98d2 100644 > --- a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist > @@ -2154,6 +2154,8 @@ GLIBC_2.36 wprintf F > GLIBC_2.36 write F > GLIBC_2.36 writev F > GLIBC_2.36 wscanf F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist > index 3738db81..f6c80b46 100644 > --- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist > +++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist > @@ -515,6 +515,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist > index ed136277..dc34ea56 100644 > --- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist > +++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist > @@ -2746,6 +2746,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist > index 83577386..466e4092 100644 > --- a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist > @@ -2719,6 +2719,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist > index 58c5da58..dc27b4e7 100644 > --- a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist > @@ -2716,6 +2716,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist > index d3741945..a8dfa647 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist > @@ -2711,6 +2711,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist > index 5319fdc2..4defd8dc 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist > @@ -2709,6 +2709,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist > index 1743ea6e..824b6b4c 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist > @@ -2717,6 +2717,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist > index 9b1f53c6..ff4341a8 100644 > --- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist > @@ -2619,6 +2619,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/nios2/libc.abilist b/sysdeps/unix/sysv/linux/nios2/libc.abilist > index ae1c6ca1..ede0e6eb 100644 > --- a/sysdeps/unix/sysv/linux/nios2/libc.abilist > +++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist > @@ -2758,6 +2758,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/or1k/libc.abilist b/sysdeps/unix/sysv/linux/or1k/libc.abilist > index a7c572c9..22525fe8 100644 > --- a/sysdeps/unix/sysv/linux/or1k/libc.abilist > +++ b/sysdeps/unix/sysv/linux/or1k/libc.abilist > @@ -2140,6 +2140,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist > index 074fa031..2938361e 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist > @@ -2773,6 +2773,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist > index dfcb4bd2..e1c70a25 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist > @@ -2806,6 +2806,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist > index 63bbccf3..2ce2d974 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist > @@ -2527,6 +2527,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist > index ab85fd61..b1024501 100644 > --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist > @@ -2829,6 +2829,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fscanfieee128 F > GLIBC_2.38 __isoc23_fwscanf F > diff --git a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist > index b716f5c7..833fb747 100644 > --- a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist > @@ -2396,6 +2396,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist > index 774e777b..cbd85454 100644 > --- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist > @@ -2596,6 +2596,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist > index 8625135c..1442a0b9 100644 > --- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist > @@ -2771,6 +2771,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist > index d00c7eb2..1a7f4fc7 100644 > --- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist > @@ -2564,6 +2564,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sh/be/libc.abilist b/sysdeps/unix/sysv/linux/sh/be/libc.abilist > index b6303724..3146ce99 100644 > --- a/sysdeps/unix/sysv/linux/sh/be/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sh/be/libc.abilist > @@ -2626,6 +2626,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sh/le/libc.abilist b/sysdeps/unix/sysv/linux/sh/le/libc.abilist > index d8005561..31360691 100644 > --- a/sysdeps/unix/sysv/linux/sh/le/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sh/le/libc.abilist > @@ -2623,6 +2623,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist > index 5be55c11..026e9581 100644 > --- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist > @@ -2766,6 +2766,8 @@ GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > GLIBC_2.37 __ppoll64_chk F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist > index 475fdaae..6ecb0d46 100644 > --- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist > @@ -2591,6 +2591,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist > index 6cfb928b..a37f4ab4 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist > +++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist > @@ -2542,6 +2542,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F > diff --git a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist > index c7350971..3b8790f4 100644 > --- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist > +++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist > @@ -2648,6 +2648,8 @@ GLIBC_2.36 pidfd_open F > GLIBC_2.36 pidfd_send_signal F > GLIBC_2.36 process_madvise F > GLIBC_2.36 process_mrelease F > +GLIBC_2.38 __fcntl64_2 F > +GLIBC_2.38 __fcntl_2 F > GLIBC_2.38 __isoc23_fscanf F > GLIBC_2.38 __isoc23_fwscanf F > GLIBC_2.38 __isoc23_scanf F [1] https://patchwork.sourceware.org/project/glibc/list/?series=19418 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 19:15 ` Siddhesh Poyarekar @ 2023-05-23 20:01 ` Sergey Bugaev 2023-05-23 20:06 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-23 20:01 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: libc-alpha Hello, On Tue, May 23, 2023 at 10:15 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote: > *_chk but I don't want to block this good work on that. The overall > direction is good IMO Yay, thanks! > - manual/contrib.texi should be updated to mention fcntl Do you mean manual/maint.texi? manual/contrib.texi seems to contain a list of contributors. > - Internal users end up calling __libc_fcntl (see dup2.c or grantpt.c > for example), which will essentially bypass any fortification. This is > not a problem today since we don't build glibc with fortification, but > Frederic Berat[1] has been experimenting with that and we're hoping to > get at least a subset of glibc fortified for 2.38. It would be a shame > to miss fortifying glibc itself. This is again not a problem that would > block this patch, but something to be aware of. Yes, I've thought of this, but I don't know what I could change in this patch to make it friendlier to in-glibc fortification. I've generally done things the same way as the open* fortification does; so I don't think this would be adding any new complications compared to what's already in there. > Given that this will error out at compile time, do we even need this > __fcntl64_2 call? Same as for the open* fortification: this call will only fail at compile time if the cmd is a compile-time constant. If it's not (which is a rare, but valid case), we need to do the check at runtime. Note that if cmd _is_ a compile-time constant and does not require the 3rd arg, this will call the regular fcntl, not the _2 version; the _2 version is only for non-compile-time-const cmd-s. Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 20:01 ` Sergey Bugaev @ 2023-05-23 20:06 ` Sergey Bugaev 0 siblings, 0 replies; 31+ messages in thread From: Sergey Bugaev @ 2023-05-23 20:06 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: libc-alpha On Tue, May 23, 2023 at 11:01 PM Sergey Bugaev <bugaevc@gmail.com> wrote: > Same as for the open* fortification: this call will only fail at > compile time if the cmd is a compile-time constant. If it's not (which > is a rare, but valid case), we need to do the check at runtime. Note > that if cmd _is_ a compile-time constant and does not require the 3rd > arg, this will call the regular fcntl, not the _2 version; the _2 > version is only for non-compile-time-const cmd-s. 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. Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev ` (2 preceding siblings ...) 2023-05-23 19:15 ` Siddhesh Poyarekar @ 2023-05-23 21:46 ` Florian Weimer 2023-05-24 7:31 ` Sergey Bugaev 3 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2023-05-23 21:46 UTC (permalink / raw) To: Sergey Bugaev via Libc-alpha; +Cc: Sergey Bugaev, Hector Martin * Sergey Bugaev via Libc-alpha: > Both open () and fcntl () are "overloaded" to accept either 2 or 3 > arguments; whether the last argument is required depends on the values > of the previous arguments. Since C provides no native support for > function overloading, this is implemented by making these functions > vararg. Unfortunately, this means the compiler is unable to check the > number of arguments and their types for correctness at compile time, and > will not diagnose any mistakes. > > To help with this, when FORTIFY_SOURCE is enabled, the special fcntl2.h > header replaces open () with a wrapper that checks the passed number of > arguments, raising a compile-time or run-time error on mismatch. This > commit adds similar handling for fcntl (). > > Recently, Hector Martin <marcan@marcan.st> has identified an issue in > libwebrtc where fcntl (fd, F_DUPFD_CLOEXEC) was invoked without a third > argument [0]. With the patch, the bug would have been detected at > compile time, assuming libwebrtc is built with FORTIFY_SOURCE. Hopefully > this will help detecting similar bugs in existing software, and prevent > more instances of similar bugs from being introduced in the future. A while ago, I looked into implementing this with __builtin_types_compatible_p and __builtin_choose_expr. It seemed feasible (although I didn't complete it), but C++ would require a completely different implementation. I'm not sure if this goes in the right direction. Maybe we should add specialized functions for the common fcntl requests. This way, we'd get compile-time type checking in a far more maintainable manner. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-23 21:46 ` Florian Weimer @ 2023-05-24 7:31 ` Sergey Bugaev 2023-05-24 8:29 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-24 7:31 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha Hello, On Wed, May 24, 2023 at 12:46 AM Florian Weimer <fw@deneb.enyo.de> wrote: > A while ago, I looked into implementing this with > __builtin_types_compatible_p and __builtin_choose_expr. It seemed > feasible (although I didn't complete it), but C++ would require a > completely different implementation. > > I'm not sure if this goes in the right direction. Maybe we should add > specialized functions for the common fcntl requests. This way, we'd > get compile-time type checking in a far more maintainable manner. Interesting -- I have thought of doing type checking, but haven't found a way to make it work. There doesn't seem to be a way to extract an argument value / type out of a __builtin_va_arg_pack. Even if you know that __builtin_va_arg_pack_len () == 1, you cannot do: int arg = __builtin_va_arg_pack (); or int __fcntl3_int (int fd, int cmd, int arg); __fcntl3_int (fd, cmd, __builtin_va_arg_pack ()); In other words: __builtin_va_arg_pack () does not behave like a macro that gets textually expanded to the anonymous argument(s), but really as a value of the "..." "argument". You can do typeof (__builtin_va_arg_pack ()), but it just always ends up being an int, no matter what you actually pass. You could use the regular va_arg (), but that of course requires you to specify the type to cast to yourself, so no point in doing that. Maybe it's possible to achieve with some preprocessor trickery? Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 7:31 ` Sergey Bugaev @ 2023-05-24 8:29 ` Florian Weimer 2023-05-24 10:51 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2023-05-24 8:29 UTC (permalink / raw) To: Sergey Bugaev via Libc-alpha; +Cc: Sergey Bugaev [-- Attachment #1: Type: text/plain, Size: 5078 bytes --] * Sergey Bugaev via Libc-alpha: > Hello, > > On Wed, May 24, 2023 at 12:46 AM Florian Weimer <fw@deneb.enyo.de> wrote: >> A while ago, I looked into implementing this with >> __builtin_types_compatible_p and __builtin_choose_expr. It seemed >> feasible (although I didn't complete it), but C++ would require a >> completely different implementation. >> >> I'm not sure if this goes in the right direction. Maybe we should add >> specialized functions for the common fcntl requests. This way, we'd >> get compile-time type checking in a far more maintainable manner. > > Interesting -- I have thought of doing type checking, but haven't > found a way to make it work. > > There doesn't seem to be a way to extract an argument value / type out > of a __builtin_va_arg_pack. Even if you know that > __builtin_va_arg_pack_len () == 1, you cannot do: > > int arg = __builtin_va_arg_pack (); > > or > > int __fcntl3_int (int fd, int cmd, int arg); > __fcntl3_int (fd, cmd, __builtin_va_arg_pack ()); > > In other words: __builtin_va_arg_pack () does not behave like a macro > that gets textually expanded to the anonymous argument(s), but really > as a value of the "..." "argument". In my attempt, the top level looks like this: +#define fcntl(fd, cmd, ...) \ + (__builtin_constant_p (cmd) \ + ? __builtin_choose_expression \ + (__fcntl_is_void (cmd), __fcntl_void (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_int (cmd), __fcntl_int (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock_const (cmd), __fcntl_flock_const (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock (cmd), __fcntl_flock (fd, cmd, __VA_ARGS__), \ + __builtin_chose_expression \ + (__fcntl_is_flock64_const (cmd), __fcntl_flock64_const (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock64 (cmd), __fcntl_flock64 (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock32_const (cmd), __fcntl_flock32_const (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock32 (cmd), __fcntl_flock32 (fd, cmd, __VA_ARGS__), \ + __fcntl_unchecked (fd, cmd, __VA_ARGS__))))))))) \ + : __fcntl_unchecked (fd, cmd, __VA_ARGS__)) 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. The type predicts look like this: +#ifdef F_SETLK64 +# define __fcntl_is_flock64_const(cmd) \ + ((cmd) == F_SETLK64 || (cmd) == F_SETLKW64) +# define __fcntl_is_flock64(cmd) ((cmd) == F_GETLK64) +#else +# define __fcntl_is_flock64_const(cmd) 0 +# define __fcntl_is_flock64(cmd) 0 +#endif The helper wrappers are simple redirects to the existing exported function, with transparent unions to handle the aliases. +#if defined (__USE_LARGEFILE64) && defined (__OFF_T_MATCHES_OFF64_T) +typedef union __attribute__ ((__transparent_union__)) +{ + struct flock *__flock; + struct flock64 *__flock64; +} __flock_pointer; +typedef union __attribute__ ((__transparent_union__)) +{ + const struct flock *__flock; + const struct flock64 *__flock64; +} __flock_const_pointer; +#else /* flock and flock64 are distinct */ +typedef struct flock *__flock_pointer; +typedef const struct flock *__flock_const_pointer; +#endif + +int __REDIRECT (__fcntl_flock, (int, int, __flock_pointer), __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock_const, (int, int, __flock_const_pointer), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock32, (int, int, struct flock *), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock32_const, (int, int, const struct flock *), + __fcntl_chk) __wur; +#ifdef __USE_LARGEFILE64 +int __REDIRECT (__fcntl_flock64, (int, int, struct flock64 *), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock64_const, (int, int, const struct flock64 *), + __fcntl_chk) __wur; +#endif The compiler will then warn about the type mismatches (-Wincompatible-pointer-types is not an error by default, for backwards compatibility). But as I said, I don't think this approach worked because __builtin_choose_expression does not suppress those warnings. I'm attaching my broken patch. It's based on commit ef4f97648dc9584 (from 2016). Thanks, Florian [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: fcntl.patch --] [-- Type: text/x-patch, Size: 8786 bytes --] commit badf4c4724dcd838d27b40f92eb7d0039a968793 Author: Florian Weimer <fweimer@redhat.com> Date: Thu Sep 1 15:49:45 2016 +0200 WIP fcntl hardening diff --git a/include/bits/fcntl3.h b/include/bits/fcntl3.h new file mode 100644 index 0000000000..d31154f635 --- /dev/null +++ b/include/bits/fcntl3.h @@ -0,0 +1 @@ +#include "../../io/bits/fcntl3.h" diff --git a/io/Makefile b/io/Makefile index deb6100156..98603e012c 100644 --- a/io/Makefile +++ b/io/Makefile @@ -25,7 +25,7 @@ include ../Makeconfig headers := sys/stat.h bits/stat.h sys/statfs.h bits/statfs.h sys/vfs.h \ sys/statvfs.h bits/statvfs.h fcntl.h sys/fcntl.h bits/fcntl.h \ poll.h sys/poll.h bits/poll.h bits/fcntl2.h bits/poll2.h \ - utime.h ftw.h fts.h sys/sendfile.h + utime.h ftw.h fts.h sys/sendfile.h bits/fcntl3.h routines := \ utime \ diff --git a/io/bits/fcntl3.h b/io/bits/fcntl3.h new file mode 100644 index 0000000000..f9778ecec3 --- /dev/null +++ b/io/bits/fcntl3.h @@ -0,0 +1,180 @@ +/* Checking macros for fcntl functions. + Copyright (C) 2016 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <http://www.gnu.org/licenses/>. */ + +#ifndef _FCNTL_H +# error "Never include <bits/fcntl3.h> directly; use <fcntl.h> instead." +#endif + +/* The type-safe aliases call the __fcntl_chk function. Only calls + which are not known to be type-safe use fcntl. */ +int __fcntl_chk (int, int, ...); + +__errordecl (__fcntl_too_many_args, + "fcntl can only be called with 2 or 3 arguments"); + +/* Void arguments are ignored if present, and the return value must be + used. */ +__fortify_function __wur int +__fcntl_void (int __fd, int __cmd, ...) +{ + if (__va_arg_pack_len () > 1) + __fcntl_too_many_args (); + return __fcntl_chk (__fd, __cmd, __va_arg_pack ()); +} + +#ifdef F_GET_SEALS +# define __fcntl_is_F_GET_SEALS(cmd) ((cmd) == F_GET_SEALS) +#else +# define __fcntl_is_F_GET_SEALS(cmd) 0 +#endif +#define __fcntl_is_void(cmd) \ + ((cmd) == F_GET_FD \ + || (cmd) == F_GETFL \ + || (cmd) == F_GETOWN \ + || (cmd) == F_GETSIG \ + || (cmd) == __fcntl_is_F_GET_SEALS(cmd)) + +int __REDIRECT (__fcntl_int, (int, int, int __arg), __fcntl_chk); +#ifdef F_ADD_SEALS +# define __fcntl_is_F_ADD_SEALS(cmd) (cmd) == F_ADD_SEALS +#else +# define __fcntl_is_F_ADD_SEALS(cmd) 0 +#endif +#define __fcntl_is_int(cmd) \ + ((cmd) == F_DUPFD \ + || (cmd) == F_DUPFD_CLOEXEC \ + || (cmd) == F_SETFD \ + || (cmd) == F_SETFL \ + || (cmd) == F_SETOWN \ + || (cmd) == F_SETSIG \ + || (cmd) == F_SETLEASE \ + || (cmd) == F_NOTIFY \ + || (cmd) == F_SETPIPE_SZ \ + || __fcntl_is_F_ADD_SEALS (cmd)) + +int __REDIRECT (__fcntl_f_owner_ex, (int, int, struct f_owner_ex *), + __fcntl_chk); +int __REDIRECT (__fcntl_f_owner_ex_const, + (int, int, const struct f_owner_ex *), + __fcntl_chk); + +/* struct flock *, struct flock64 * arguments. */ +#define __fcntl_is_flock_const(cmd) ((cmd) == F_SETLK || (cmd) == F_SETLKW) +#define __fcntl_is_flock(cmd) ((cmd) == F_GETLK) + +#ifdef F_SETLK64 +# define __fcntl_is_flock64_const(cmd) \ + ((cmd) == F_SETLK64 || (cmd) == F_SETLKW64) +# define __fcntl_is_flock64(cmd) ((cmd) == F_GETLK64) +#else +# define __fcntl_is_flock64_const(cmd) 0 +# define __fcntl_is_flock64(cmd) 0 +#endif +#ifdef F_SETLK32 +# define __fcntl_is_flock32_const(cmd) \ + ((cmd) == F_SETLK32 || (cmd) == F_SETLKW32) +# define __fcntl_is_flock32(cmd) ((cmd) == F_GETLK32) +#else +# define __fcntl_is_flock32_const(cmd) 0 +# define __fcntl_is_flock32(cmd) 0 +#endif + +#if defined (__USE_LARGEFILE64) && defined (__OFF_T_MATCHES_OFF64_T) +typedef union __attribute__ ((__transparent_union__)) +{ + struct flock *__flock; + struct flock64 *__flock64; +} __flock_pointer; +typedef union __attribute__ ((__transparent_union__)) +{ + const struct flock *__flock; + const struct flock64 *__flock64; +} __flock_const_pointer; +#else /* flock and flock64 are distinct */ +typedef struct flock *__flock_pointer; +typedef const struct flock *__flock_const_pointer; +#endif + +int __REDIRECT (__fcntl_flock, (int, int, __flock_pointer), __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock_const, (int, int, __flock_const_pointer), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock32, (int, int, struct flock *), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock32_const, (int, int, const struct flock *), + __fcntl_chk) __wur; +#ifdef __USE_LARGEFILE64 +int __REDIRECT (__fcntl_flock64, (int, int, struct flock64 *), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_flock64_const, (int, int, const struct flock64 *), + __fcntl_chk) __wur; +#endif + +#ifdef F_OFD_SETLK +# define __fcntl_is_ofd_flock_const(cmd) \ + ((cmd) == F_OFD_SETLK || (cmd) == F_OFD_SETLKW) +# define __fcntl_is_ofd_flock(cmd) ((cmd) == F_OFD_GETLK) + +/* OFD locks are only available with 64-bit struct flock. */ +# if defined (__OFF_T_MATCHES_OFF64_T) +int __REDIRECT (__fcntl_ofd_flock, (int, int, __flock_pointer), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_ofd_flock_const, (int, int, __flock_const_pointer), + __fcntl_chk) __wur; +# else +int __REDIRECT (__fcntl_ofd_flock, (int, int, struct flock64 *), + __fcntl_chk) __wur; +int __REDIRECT (__fcntl_ofd_flock_const, (int, const struct flock64 *), + __fcntl_chk) __wur; +# endif + +#else /* !defined (F_OFD_SETLK) */ +# define __fcntl_is_ofd_flock_const (cmd) 0 +# define __fcntl_is_ofd_flock (cmd) 0 +#endif + +extern int __REDIRECT (__fcntl_warn, (int, int, ...), fcntl) + __warnattr ("fcntl called with an unknown command argument"); + +__fortify_function int +__fcntl_unchecked (int __fd, int __cmd, ...) +{ + if (__va_arg_pack_len () > 1) + __fcntl_too_many_args (); + return __fcntl_warn (__fd, __cmd, __va_arg_pack ()); +} + +#define fcntl(fd, cmd, ...) \ + (__builtin_constant_p (cmd) \ + ? __builtin_choose_expression \ + (__fcntl_is_void (cmd), __fcntl_void (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_int (cmd), __fcntl_int (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock_const (cmd), __fcntl_flock_const (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock (cmd), __fcntl_flock (fd, cmd, __VA_ARGS__), \ + __builtin_chose_expression \ + (__fcntl_is_flock64_const (cmd), __fcntl_flock64_const (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock64 (cmd), __fcntl_flock64 (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock32_const (cmd), __fcntl_flock32_const (fd, cmd, __VA_ARGS__), \ + __builtin_choose_expression \ + (__fcntl_is_flock32 (cmd), __fcntl_flock32 (fd, cmd, __VA_ARGS__), \ + __fcntl_unchecked (fd, cmd, __VA_ARGS__))))))))) \ + : __fcntl_unchecked (fd, cmd, __VA_ARGS__)) diff --git a/io/fcntl.h b/io/fcntl.h index cb706b4f0f..79887db6f8 100644 --- a/io/fcntl.h +++ b/io/fcntl.h @@ -314,6 +314,11 @@ extern int posix_fallocate64 (int __fd, off64_t __offset, off64_t __len); # include <bits/fcntl2.h> #endif +# if !defined (__cplusplus) && __USE_FORTIFY_LEVEL > 0 \ + && defined __fortify_function && defined __va_arg_pack_len +# include <bits/fcntl3.h> +#endif + __END_DECLS #endif /* fcntl.h */ ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 8:29 ` Florian Weimer @ 2023-05-24 10:51 ` Sergey Bugaev 2023-05-24 11:18 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-24 10:51 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha 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. 2. It doesn't do the runtime check if called with 2 args and non-c-t-const cmd. > 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++). 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? Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 10:51 ` Sergey Bugaev @ 2023-05-24 11:18 ` Florian Weimer 2023-05-24 11:46 ` Siddhesh Poyarekar 0 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2023-05-24 11:18 UTC (permalink / raw) To: Sergey Bugaev; +Cc: libc-alpha * 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 11:18 ` Florian Weimer @ 2023-05-24 11:46 ` Siddhesh Poyarekar 2023-05-24 12:12 ` Andreas Schwab 0 siblings, 1 reply; 31+ messages in thread From: Siddhesh Poyarekar @ 2023-05-24 11:46 UTC (permalink / raw) To: Florian Weimer, Sergey Bugaev; +Cc: libc-alpha On 2023-05-24 07:18, Florian Weimer via Libc-alpha wrote: >>> 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. Please be sure to make the _Generic bits conditional on __USE_ISOC11. Thanks, Sid ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 11:46 ` Siddhesh Poyarekar @ 2023-05-24 12:12 ` Andreas Schwab 2023-05-24 12:18 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Andreas Schwab @ 2023-05-24 12:12 UTC (permalink / raw) To: Siddhesh Poyarekar; +Cc: Florian Weimer, Sergey Bugaev, libc-alpha On Mai 24 2023, Siddhesh Poyarekar wrote: > Please be sure to make the _Generic bits conditional on __USE_ISOC11. That's not enough. GCC 4.7 supports C11, but _Generic has only been added in GCC 4.9. -- Andreas Schwab, SUSE Labs, schwab@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different." ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 12:12 ` Andreas Schwab @ 2023-05-24 12:18 ` Florian Weimer 2023-05-24 12:37 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2023-05-24 12:18 UTC (permalink / raw) To: Andreas Schwab; +Cc: Siddhesh Poyarekar, Sergey Bugaev, libc-alpha * Andreas Schwab: > On Mai 24 2023, Siddhesh Poyarekar wrote: > >> Please be sure to make the _Generic bits conditional on __USE_ISOC11. > > That's not enough. GCC 4.7 supports C11, but _Generic has only been > added in GCC 4.9. We also generally do not prevent the use of compiler extensions in strict feature modes. I think this should use a compiler version check and maybe a fallback check against C17 compiler support if we do not need any extensions (assuming that all C17 compilers implement _Generic, so that an exception like the one you mentioned for GCC does not apply vis-a-vis C17). Thanks, Florian ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 12:18 ` Florian Weimer @ 2023-05-24 12:37 ` Sergey Bugaev 2023-05-24 12:45 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-24 12:37 UTC (permalink / raw) To: Florian Weimer; +Cc: Andreas Schwab, Siddhesh Poyarekar, libc-alpha On Wed, May 24, 2023 at 3:18 PM Florian Weimer <fweimer@redhat.com> wrote: > We also generally do not prevent the use of compiler extensions in > strict feature modes. I think this should use a compiler version check > and maybe a fallback check against C17 compiler support if we do not > need any extensions (assuming that all C17 compilers implement _Generic, > so that an exception like the one you mentioned for GCC does not apply > vis-a-vis C17). Do I understand this correctly? -- you want the check to be #if (__STDC_VERSION__ >= 201710L) || (__GNUC_PREREQ (4, 9) && __STDC_VERSION__ >= 201112L) right? Why even care for compilers other than modern GCC (and Clang pretending to be GCC), considering the fortifications use very GCC-specific intrinsics (__builtin_*, attributes)? Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 12:37 ` Sergey Bugaev @ 2023-05-24 12:45 ` Florian Weimer 2023-05-24 13:02 ` Sergey Bugaev 0 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2023-05-24 12:45 UTC (permalink / raw) To: Sergey Bugaev; +Cc: Andreas Schwab, Siddhesh Poyarekar, libc-alpha * Sergey Bugaev: > On Wed, May 24, 2023 at 3:18 PM Florian Weimer <fweimer@redhat.com> wrote: >> We also generally do not prevent the use of compiler extensions in >> strict feature modes. I think this should use a compiler version check >> and maybe a fallback check against C17 compiler support if we do not >> need any extensions (assuming that all C17 compilers implement _Generic, >> so that an exception like the one you mentioned for GCC does not apply >> vis-a-vis C17). > > Do I understand this correctly? -- you want the check to be > > #if (__STDC_VERSION__ >= 201710L) || (__GNUC_PREREQ (4, 9) && > __STDC_VERSION__ >= 201112L) > > right? More like this: #if __STDC_VERSION__ >= 201710L) || __GNUC_PREREQ (4, 9) If you have GCC 4.9, __STDC_VERSION__ does not matter if you use __extension__ in front of _Generic (untested). Clang would benefit from another version conditional. But we may not need _Generic after all. > Why even care for compilers other than modern GCC (and Clang > pretending to be GCC), considering the fortifications use very > GCC-specific intrinsics (__builtin_*, attributes)? Some of us want to reproduce things with older compilers, for bisecting regressions for example. For that, the headers better remain compatible. And GCC 4.8 is still very widely used (although not with current GCC headers, admittedly). Thanks, Florian ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 12:45 ` Florian Weimer @ 2023-05-24 13:02 ` Sergey Bugaev 2023-05-24 13:18 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Sergey Bugaev @ 2023-05-24 13:02 UTC (permalink / raw) To: Florian Weimer; +Cc: Andreas Schwab, Siddhesh Poyarekar, libc-alpha On Wed, May 24, 2023 at 3:45 PM Florian Weimer <fweimer@redhat.com> wrote: > If you have GCC 4.9, __STDC_VERSION__ does not matter if you use > __extension__ in front of _Generic (untested). Clang would benefit from > another version conditional. So *that's* what __extension__ is for! Makes sense, thanks. > But we may not need _Generic after all. Indeed, this seems to work just as well: #define __fcntl_type_check(cmd, arg) \ (__fcntl_is_int (cmd) ? __builtin_types_compatible_p (typeof (arg), int) : \ (__fcntl_is_flock (cmd) ? __builtin_types_compatible_p (typeof (arg), struct flock *) : 1)) And __builtin_types_compatible_p appears to have been introduced in GCC 3.1. Sergey ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments 2023-05-24 13:02 ` Sergey Bugaev @ 2023-05-24 13:18 ` Florian Weimer 0 siblings, 0 replies; 31+ messages in thread From: Florian Weimer @ 2023-05-24 13:18 UTC (permalink / raw) To: Sergey Bugaev; +Cc: Andreas Schwab, Siddhesh Poyarekar, libc-alpha * Sergey Bugaev: > On Wed, May 24, 2023 at 3:45 PM Florian Weimer <fweimer@redhat.com> wrote: >> If you have GCC 4.9, __STDC_VERSION__ does not matter if you use >> __extension__ in front of _Generic (untested). Clang would benefit from >> another version conditional. > > So *that's* what __extension__ is for! Makes sense, thanks. I'm not 100% sure if we need it because _Generic is reserved in any standards version. >> But we may not need _Generic after all. > > Indeed, this seems to work just as well: > > #define __fcntl_type_check(cmd, arg) > \ > (__fcntl_is_int (cmd) ? __builtin_types_compatible_p (typeof (arg), > int) : \ > (__fcntl_is_flock (cmd) ? __builtin_types_compatible_p (typeof > (arg), struct flock *) : 1)) Fair enough. This needs __typeof (or maybe __extension__ __typeof), by the way. Thanks, Florian ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2023-05-24 13:18 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-05-19 21:30 [RFC PATCH 0/1] Attempt to detect missing fcntl argument at compile time Sergey Bugaev 2023-05-19 21:30 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev 2023-05-19 21:55 ` Joseph Myers 2023-05-20 11:46 ` Sergey Bugaev 2023-05-20 18:21 ` [RFC PATCH] debug: Add tests for fortified fcntl () Sergey Bugaev 2023-05-23 18:40 ` Adhemerval Zanella Netto 2023-05-23 19:19 ` Sergey Bugaev 2023-05-23 19:48 ` Adhemerval Zanella Netto 2023-05-24 7:15 ` Sergey Bugaev 2023-05-24 12:15 ` Adhemerval Zanella Netto 2023-05-23 19:09 ` [RFC PATCH 1/1] io: Add FORTIFY_SOURCE check for fcntl arguments Adhemerval Zanella Netto 2023-05-23 19:43 ` Sergey Bugaev 2023-05-23 19:56 ` Adhemerval Zanella Netto 2023-05-23 20:24 ` Sergey Bugaev 2023-05-23 20:44 ` Sergey Bugaev 2023-05-24 12:04 ` Adhemerval Zanella Netto 2023-05-23 19:15 ` Siddhesh Poyarekar 2023-05-23 20:01 ` Sergey Bugaev 2023-05-23 20:06 ` Sergey Bugaev 2023-05-23 21:46 ` Florian Weimer 2023-05-24 7:31 ` Sergey Bugaev 2023-05-24 8:29 ` Florian Weimer 2023-05-24 10:51 ` Sergey Bugaev 2023-05-24 11:18 ` Florian Weimer 2023-05-24 11:46 ` Siddhesh Poyarekar 2023-05-24 12:12 ` Andreas Schwab 2023-05-24 12:18 ` Florian Weimer 2023-05-24 12:37 ` Sergey Bugaev 2023-05-24 12:45 ` Florian Weimer 2023-05-24 13:02 ` Sergey Bugaev 2023-05-24 13:18 ` 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).