* [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 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-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] 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 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] 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 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: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-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-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] 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 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-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-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] 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-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).