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

Hello,

this is v3 of the fcntl fortification work. v1 was at [0], v2 at [1].

[0]: https://sourceware.org/pipermail/libc-alpha/2023-May/148332.html
[1]: https://sourceware.org/pipermail/libc-alpha/2023-May/148569.html

Changes since v2:

- This is now in its own separate header, bits/fcntl3.h
  (bits/fcntl2.h is used by the open fortification)

- Clang is now supported in addition to GCC!
  - Clang does not support nor need the "-Wsystem-headers" pragma
  - Clang does support error/warning attributes since recently
  - There seems to be a bug in Clang which prevents the type mismatch
    warning from actually firing. Specifically, it appears that Clang
    gets confused about C functions names vs symbol names when it comes
    to attribute ((warning)), and does not emit the warning if the
    function declared with __warnattr has a symbol name matching that of
    another function that has not been declared with __warnattr.

    While this could be worked around in glibc (such as by adding
    __fcntl_warn as a real wrapper function when built with Clang), I
    think this just needs to be fixed in Clang. Any LLVM developers
    here? :D

- Changed hide_constant utility to use an empty inline asm statement
  instead of volatile and noinline, as per the discussion. I did not
  make this into a general-purpose glibc-wide utility because I don't
  know what a fitting name and place (header) for it would be. If you'd
  like to see it glibc-wide, please suggest me where to put it and how
  to name it!

- Fixed the C++ template linkage thing

- Addressed misc review comments

- Looked into applying __builtin_constant_p to the result of the cmd
  check and not the cmd value itself, as suggested by Florian.

  Unfortunately this does not work at all :( __builtin_constant_p starts
  returning 0 given anything remotely complex like even a trivial inline
  function call (so technically hide_constant would still work if it was
  just 'return value;'), even if the function is (later?) fully inlined
  and const-folded.

  *Maybe* this could be made to work if I used an obscene amount of
  macros instead of inline functions (to handle all the various commands
  being conditionally defined), but I don't want to go there.

  So since this didn't work out, I left the runtime __fcntl_2 function,
  but split it into a separate patch, so you can apply it or drop it
  depending on what you prefer in the end.

Clang / C++ demo:

------------------------------------------------------------------
$ clang++ test-fcntl.cpp -D _FORTIFY_SOURCE=2 -O2
In file included from test-fcntl.cpp:1:
In file included from /usr/include/fcntl.h:348:
/usr/include/bits/fcntl3.h:394:5: error: call to '__fcntl_missing_arg' declared with 'error' attribute: fcntl with with this command needs 3 arguments
    __fcntl_missing_arg ();
    ^
1 error generated.
------------------------------------------------------------------

Sergey

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 1/5] support: Add support_fcntl_support_ofd_locks ()
  2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
@ 2023-06-17 22:22 ` Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 2/5] cdefs.h: Define __glibc_warn_system_headers_{begin,end} Sergey Bugaev
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-17 22:22 UTC (permalink / raw)
  To: libc-alpha; +Cc: Adhemerval Zanella

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 support/Makefile                          |  1 +
 support/support.h                         |  3 ++
 support/support_fcntl_support_ofd_locks.c | 44 +++++++++++++++++++++++
 3 files changed, 48 insertions(+)
 create mode 100644 support/support_fcntl_support_ofd_locks.c

diff --git a/support/Makefile b/support/Makefile
index 8ebe68aa..4dd9335b 100644
--- a/support/Makefile
+++ b/support/Makefile
@@ -58,6 +58,7 @@ libsupport-routines = \
   support_descriptors \
   support_enter_mount_namespace \
   support_enter_network_namespace \
+  support_fcntl_support_ofd_locks \
   support_format_address_family \
   support_format_addrinfo \
   support_format_dns_packet \
diff --git a/support/support.h b/support/support.h
index b7f76bf0..e20d2ce7 100644
--- a/support/support.h
+++ b/support/support.h
@@ -178,6 +178,9 @@ static __inline bool support_itimer_support_time64 (void)
 #endif
 }
 
+/* Return true if the kernel/file supports open file description locks.  */
+extern bool support_fcntl_support_ofd_locks (int fd);
+
 /* Return true if stat supports nanoseconds resolution.  PATH is used
    for tests and its ctime may change.  */
 extern bool support_stat_nanoseconds (const char *path);
diff --git a/support/support_fcntl_support_ofd_locks.c b/support/support_fcntl_support_ofd_locks.c
new file mode 100644
index 00000000..fb197a70
--- /dev/null
+++ b/support/support_fcntl_support_ofd_locks.c
@@ -0,0 +1,44 @@
+/* Return whether the kernel/file supports OFD locks.
+   Copyright (C) 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/>.  */
+
+#include <support/support.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <string.h>
+
+bool
+support_fcntl_support_ofd_locks (int fd)
+{
+#ifdef F_OFD_GETLK
+  int res;
+  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 (fd, F_OFD_GETLK, &flock);
+  return res != -1 || errno != EINVAL;
+#else
+  (void) fd;
+  return false;
+#endif
+}
-- 
2.41.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 2/5] cdefs.h: Define __glibc_warn_system_headers_{begin,end}
  2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 1/5] support: Add support_fcntl_support_ofd_locks () Sergey Bugaev
@ 2023-06-17 22:22 ` Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 3/5] cdefs.h: Enable __errordecl & __warnattr for Clang 14+ Sergey Bugaev
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-17 22:22 UTC (permalink / raw)
  To: libc-alpha; +Cc: Adhemerval Zanella

By default, GCC supresses warnings inside "system headers". Moreover, it
also supresses warnings resulting from expansion of macros defined in
system headers, even in the expansion itself happens in user code. This
may be desirable most of the time because the user cannot do anything
about the mistakes of the system headers, but sometimes causing a
warning is an intentional effect that a macro has, in which case this
GCC feature gets in a way.

GCC allows turning off the warning suspension feature by passing
-Wsystem-headers; however, that turns it off globally. But by using
"#pragma GCC diagnostic" it can be made to only apply to the relevant
macro definition, in which case GCC only does not supress warnings
resulting from expansion of that one macro.

To that end, introduce __glibc_warn_system_headers_begin and
__glibc_warn_system_headers_end macros that can be used to surround a
macro definition and ensure that warnings inside the macro are not
supressed:

    __glibc_warn_system_headers_begin
    #define foo(x) bar_warn (x)
    __glibc_warn_system_headers_end

This will be used in a following patch which turns fcntl and fcntl64
into macros that cause warnings on argument type mismatch.

Note that "#pragma GCC diagnostic warning" normally overrides any
diagnostic options specified on the command line, and so can even
downgrade severity of a diagnostic from an error to a warning (if, for
instance, -Werror is in effect). But this is not a concern here, since
the actual warning that gets emitted is not "-Wsystem-headers", but some
other specific warning; "-Wsystem-headers" is only used to disable its
supression. So passing -Werror still causes the specific warning to be
treated as an error, and to fail the compilation.

Clang does not seem to support turning -Wsystem-headers on and off with
a pragma, but does not seem to suppress warnings from system-defined
macros either. Hence, the macros added by this patch expand to nothing
under Clang.

Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---

diff to v2: no longer expands to anything on Clang

 misc/sys/cdefs.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 393d9091..6ca8ca31 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -649,6 +649,21 @@ _Static_assert (0, "IEEE 128-bits long double requires redirection on this platf
 # define __glibc_macro_warning(msg)
 #endif
 
+/* __glibc_warn_system_headers_begin starts a block of code where warnings
+   produced by expanding macros defined in system headers will *not* be
+   supressed.  __glibc_warn_system_headers_end ends such a block.  */
+#if __GNUC_PREREQ (4,8)
+# define __glibc_warn_system_headers1(message) _Pragma (#message)
+# define __glibc_warn_system_headers_begin \
+  __glibc_warn_system_headers1 (GCC diagnostic push) \
+  __glibc_warn_system_headers1 (GCC diagnostic warning "-Wsystem-headers")
+# define __glibc_warn_system_headers_end \
+  __glibc_warn_system_headers1 (GCC diagnostic pop)
+#else
+# define __glibc_warn_system_headers_begin
+# define __glibc_warn_system_headers_end
+#endif
+
 /* Generic selection (ISO C11) is a C-only feature, available in GCC
    since version 4.9.  Previous versions do not provide generic
    selection, even though they might set __STDC_VERSION__ to 201112L,
-- 
2.41.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 3/5] cdefs.h: Enable __errordecl & __warnattr for Clang 14+
  2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 1/5] support: Add support_fcntl_support_ofd_locks () Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 2/5] cdefs.h: Define __glibc_warn_system_headers_{begin,end} Sergey Bugaev
@ 2023-06-17 22:22 ` Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 4/5] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-17 22:22 UTC (permalink / raw)
  To: libc-alpha; +Cc: Nick Desaulniers

Clang 14 (released in March 2022) has added support for
attribute ((error)) and attribute ((warning)). Enable their usage when
Clang 14 or later is detected, to get nicer diagnostics.

https://reviews.llvm.org/D106030
https://github.com/llvm/llvm-project/commit/846e562dcc6a9a611d844dc0d123da95629a0567

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 misc/sys/cdefs.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/misc/sys/cdefs.h b/misc/sys/cdefs.h
index 6ca8ca31..9b84043f 100644
--- a/misc/sys/cdefs.h
+++ b/misc/sys/cdefs.h
@@ -208,7 +208,7 @@
       : __ ## f ## _chk (__VA_ARGS__, (__osz) / (__s))))
 #endif
 
-#if __GNUC_PREREQ (4,3)
+#if __GNUC_PREREQ (4,3) || __glibc_clang_prereq (14,0)
 # define __warnattr(msg) __attribute__((__warning__ (msg)))
 # define __errordecl(name, msg) \
   extern void name (void) __attribute__((__error__ (msg)))
-- 
2.41.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 4/5] io: Add FORTIFY_SOURCE check for fcntl arguments
  2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
                   ` (2 preceding siblings ...)
  2023-06-17 22:22 ` [PATCH v3 3/5] cdefs.h: Enable __errordecl & __warnattr for Clang 14+ Sergey Bugaev
@ 2023-06-17 22:22 ` Sergey Bugaev
  2023-06-17 22:22 ` [PATCH v3 5/5] io: Also verify 2-arg fctnl calls at runtime Sergey Bugaev
  2023-06-19 12:58 ` [PATCH v3 0/5] fcntl fortification Carlos O'Donell
  5 siblings, 0 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-17 22:22 UTC (permalink / raw)
  To: libc-alpha

Both open () and fcntl () are "overloaded" to accept either 2 or 3
arguments; whether the last argument is required (and, in case of fcntl,
the type of the third argument) 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

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---

In manual/maint.texi, how would I properly update the command for
generating the list of fortified functions to include fcntl / fcntl64 in
absence of __fcntl_2? (The next patch does make it work with __fcntl_2.)

 debug/tst-fortify.c   | 140 ++++++++++++++
 include/bits/fcntl3.h |   1 +
 io/Makefile           |   1 +
 io/bits/fcntl3.h      | 420 ++++++++++++++++++++++++++++++++++++++++++
 io/fcntl.h            |   6 +
 manual/maint.texi     |   9 +-
 6 files changed, 575 insertions(+), 2 deletions(-)
 create mode 100644 include/bits/fcntl3.h
 create mode 100644 io/bits/fcntl3.h

diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
index 3744aada..674ada74 100644
--- a/debug/tst-fortify.c
+++ b/debug/tst-fortify.c
@@ -36,6 +36,8 @@
 #include <sys/select.h>
 #include <sys/socket.h>
 #include <sys/un.h>
+#include <support/support.h>
+#include <support/xunistd.h>
 
 #ifndef _GNU_SOURCE
 # define MEMPCPY memcpy
@@ -78,6 +80,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 inline int
+hide_constant (int value)
+{
+  asm ("" : "+rm" (value));
+  return value;
+}
+
 volatile int chk_fail_ok;
 volatile int ret;
 jmp_buf chk_fail_buf;
@@ -1811,6 +1822,135 @@ 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.  */
+  int lockfd1 = xopen (temp_filename, O_RDWR, 0);
+  int lockfd2 = xopen (temp_filename, O_RDWR, 0);
+
+  int ofd_locks_supported = support_fcntl_support_ofd_locks (lockfd1);
+
+  if (ofd_locks_supported)
+    {
+      struct flock flock;
+
+      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)
+    {
+      struct flock flock;
+
+      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 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
+
 #endif
 
   return ret;
diff --git a/include/bits/fcntl3.h b/include/bits/fcntl3.h
new file mode 100644
index 00000000..d31154f6
--- /dev/null
+++ b/include/bits/fcntl3.h
@@ -0,0 +1 @@
+#include "../../io/bits/fcntl3.h"
diff --git a/io/Makefile b/io/Makefile
index d573064e..21e1914f 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -25,6 +25,7 @@ include ../Makeconfig
 headers := \
   bits/fcntl.h \
   bits/fcntl2.h \
+  bits/fcntl3.h \
   bits/poll.h \
   bits/poll2.h \
   bits/stat.h \
diff --git a/io/bits/fcntl3.h b/io/bits/fcntl3.h
new file mode 100644
index 00000000..52a8a0d7
--- /dev/null
+++ b/io/bits/fcntl3.h
@@ -0,0 +1,420 @@
+/* Checking macros for fcntl functions.
+   Copyright (C) 2007-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/>.  */
+
+#ifndef	_FCNTL_H
+# error "Never include <bits/fcntl3.h> directly; use <fcntl.h> instead."
+#endif
+
+#ifndef __USE_TIME_BITS64
+
+# ifndef __USE_FILE_OFFSET64
+extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl);
+extern int __REDIRECT (__fcntl_warn, (int __fd, int __cmd, ...), fcntl)
+  __warnattr ("fcntl argument has wrong type for this command");
+# else
+extern int __REDIRECT (__fcntl_alias, (int __fd, int __cmd, ...), fcntl64);
+extern int __REDIRECT (__fcntl_warn, (int __fd, int __cmd, ...), fcntl64)
+  __warnattr ("fcntl argument has wrong type for this command");
+# endif /* __USE_FILE_OFFSET64 */
+
+# ifdef __USE_LARGEFILE64
+extern int __REDIRECT (__fcntl64_alias, (int __fd, int __cmd, ...), fcntl64);
+extern int __REDIRECT (__fcntl64_warn, (int __fd, int __cmd, ...), fcntl64)
+  __warnattr ("fcntl64 argument has wrong type for this command");
+# endif
+
+#else /* __USE_TIME_BITS64 */
+
+extern int __REDIRECT_NTH (__fcntl_alias, (int __fd, int __cmd, ...),
+			   __fcntl_time64);
+extern int __REDIRECT_NTH (__fcntl64_alias, (int __fd, int __cmd, ...),
+			   __fcntl_time64);
+extern int __REDIRECT (__fcntl_warn, (int __fd, int __cmd, ...),
+                        __fcntl_time64)
+  __warnattr ("fcntl argument has wrong type for this command");
+extern int __REDIRECT (__fcntl64_warn, (int __fd, int __cmd, ...),
+                        __fcntl_time64)
+  __warnattr ("fcntl64 argument has wrong type for this command");
+
+#endif /* __USE_TIME_BITS64 */
+
+
+/* Whether the fcntl CMD is known to require an argument.  */
+__extern_always_inline int
+__fcntl_requires_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;
+
+    default:
+      return 0;
+  }
+}
+
+/* Whether the fcntl CMD requires an int argument.  */
+__extern_always_inline int
+__fcntl_is_int (int __cmd)
+{
+  switch (__cmd)
+    {
+    case F_DUPFD:
+    case F_DUPFD_CLOEXEC:
+    case F_SETFD:
+    case F_SETFL:
+#ifdef F_SETOWN
+    case F_SETOWN:
+#endif
+#ifdef F_SETSIG
+    case F_SETSIG:
+#endif
+#ifdef F_SETLEASE
+    case F_SETLEASE:
+    case F_NOTIFY:
+    case F_SETPIPE_SZ:
+    case F_ADD_SEALS:
+#endif
+      return 1;
+
+    default:
+      return 0;
+    }
+}
+
+/* Whether the fcntl CMD requires a (const uint64_t *) argument.  */
+__extern_always_inline int
+__fcntl_is_const_uint64_t_ptr (int __cmd)
+{
+  switch (__cmd)
+    {
+#ifdef F_SET_RW_HINT
+    case F_SET_RW_HINT:
+    case F_SET_FILE_RW_HINT:
+      return 1;
+#endif
+
+    default:
+      return 0;
+    }
+}
+
+
+/* Whether the fcntl CMD requires an (uint64_t *) argument.  */
+__extern_always_inline int
+__fcntl_is_uint64_t_ptr (int __cmd)
+{
+  switch (__cmd)
+    {
+#ifdef F_GET_RW_HINT
+    case F_GET_RW_HINT:
+    case F_GET_FILE_RW_HINT:
+      return 1;
+#endif
+
+    default:
+      return 0;
+    }
+}
+
+/* Whether the fcntl CMD requires a (const struct f_owner_ex *) argument.  */
+__extern_always_inline int
+__fcntl_is_const_fowner_ex (int __cmd)
+{
+  switch (__cmd)
+    {
+#ifdef F_SETOWN_EX
+    case F_SETOWN_EX:
+      return 1;
+#endif
+
+    default:
+      return 0;
+  }
+}
+
+/* Whether the fcntl CMD requires a (struct f_owner_ex *) argument.  */
+__extern_always_inline int
+__fcntl_is_fowner_ex (int __cmd)
+{
+  switch (__cmd)
+    {
+#ifdef F_GETOWN_EX
+    case F_GETOWN_EX:
+      return 1;
+#endif
+
+    default:
+      return 0;
+  }
+}
+
+/* Whether the fcntl CMD requires a (const struct flock *) argument.  */
+__extern_always_inline int
+__fcntl_is_const_flock (int __cmd, int __is_fcntl64)
+{
+  (void) __is_fcntl64;
+  switch (__cmd)
+    {
+#ifdef F_SETLK
+    case F_SETLK:
+    case F_SETLKW:
+      return 1;
+#endif
+
+#ifdef F_OFD_SETLK
+    case F_OFD_SETLK:
+    case F_OFD_SETLKW:
+      return !__is_fcntl64;
+#endif
+
+    default:
+      return 0;
+    }
+}
+
+/* Whether the fcntl CMD requires a (struct flock *) argument.  */
+__extern_always_inline int
+__fcntl_is_flock (int __cmd, int __is_fcntl64)
+{
+  (void) __is_fcntl64;
+  switch (__cmd)
+    {
+#ifdef F_GETLK
+    case F_GETLK:
+      return 1;
+#endif
+
+#ifdef F_OFD_GETLK
+    case F_OFD_GETLK:
+      return !__is_fcntl64;
+#endif
+
+    default:
+      return 0;
+    }
+}
+
+/* Whether the fcntl CMD requires a (const struct flock64 *) argument.  */
+__extern_always_inline int
+__fcntl_is_const_flock64 (int __cmd, int __is_fcntl64)
+{
+  (void) __is_fcntl64;
+  switch (__cmd)
+    {
+#ifdef F_SETLK64
+    case F_SETLK64:
+    case F_SETLKW64:
+      return 1;
+#endif
+
+#ifdef F_OFD_SETLK
+    case F_OFD_SETLK:
+    case F_OFD_SETLKW:
+      return __is_fcntl64;
+#endif
+
+    default:
+      return 0;
+    }
+}
+
+/* Whether the fcntl CMD requires a (struct flock64 *) argument.  */
+__extern_always_inline int
+__fcntl_is_flock64 (int __cmd, int __is_fcntl64)
+{
+  (void) __is_fcntl64;
+  switch (__cmd)
+    {
+#ifdef F_GETLK64
+    case F_GETLK64:
+      return 1;
+#endif
+
+#ifdef F_OFD_GETLK
+    case F_OFD_GETLK:
+      return __is_fcntl64;
+#endif
+
+    default:
+      return 0;
+    }
+}
+
+#ifndef __cplusplus
+
+# define __fcntl_types_compatible(arg, type)				      \
+  __builtin_types_compatible_p (__typeof (arg), type)
+
+#else
+
+extern "C++" {
+
+template<typename, typename>
+struct __fcntl_types_compatible_helper
+{
+  __always_inline static int
+  __compatible ()
+  {
+    return 0;
+  }
+};
+
+template<typename __T>
+struct __fcntl_types_compatible_helper<__T, __T>
+{
+  __always_inline static int
+  __compatible ()
+  {
+    return 1;
+  }
+};
+
+# define __fcntl_types_compatible(arg, type)				      \
+  __fcntl_types_compatible_helper<__typeof (arg), type>::__compatible ()
+
+} /* extern C++ */
+
+#endif /* __cplusplus */
+
+#define __fcntl_type_check_int(arg) __fcntl_types_compatible (arg, int)
+
+#define __fcntl_type_check_const_uint64_t_ptr(arg)			      \
+ (__fcntl_types_compatible (arg, const __uint64_t *)			      \
+  || __fcntl_types_compatible (arg, __uint64_t *))
+
+#define __fcntl_type_check_uint64_t_ptr(arg)				      \
+  __fcntl_types_compatible (arg, __uint64_t *)
+
+#define __fcntl_type_check_const_fowner_ex(arg)				      \
+  (__fcntl_types_compatible (arg, const struct f_owner_ex *)		      \
+   || __fcntl_types_compatible (arg, struct f_owner_ex *))
+
+#define __fcntl_type_check_fowner_ex(arg)				      \
+  __fcntl_types_compatible (arg, struct f_owner_ex *)
+
+#define __fcntl_type_check_const_flock(arg)				      \
+  (__fcntl_types_compatible (arg, const struct flock *)			      \
+   || __fcntl_types_compatible (arg, struct flock *))
+
+#define __fcntl_type_check_flock(arg)					      \
+  __fcntl_types_compatible (arg, struct flock *)
+
+#ifdef __USE_LARGEFILE64
+
+# define __fcntl_type_check_const_flock64(arg)				      \
+  (__fcntl_types_compatible (arg, const struct flock64 *)		      \
+   || __fcntl_types_compatible (arg, struct flock64 *))
+
+# define __fcntl_type_check_flock64(arg)				      \
+  __fcntl_types_compatible (arg, struct flock64 *)
+
+#else
+
+# define __fcntl_type_check_const_flock64(arg) 0
+# define __fcntl_type_check_flock64(arg) 0
+
+#endif /* __USE_LARGEFILE64 */
+
+#define __fcntl_type_check(cmd, arg, is_fcntl64)			      \
+  (__fcntl_is_int (cmd) ? __fcntl_type_check_int (arg) :		      \
+   __fcntl_is_const_uint64_t_ptr (cmd)					      \
+     ? __fcntl_type_check_const_uint64_t_ptr (arg) :			      \
+   __fcntl_is_uint64_t_ptr (cmd) ? __fcntl_type_check_uint64_t_ptr (arg) :    \
+   __fcntl_is_const_fowner_ex (cmd)					      \
+     ? __fcntl_type_check_const_fowner_ex (arg) :			      \
+   __fcntl_is_fowner_ex (cmd) ? __fcntl_type_check_fowner_ex (arg) :	      \
+   __fcntl_is_const_flock (cmd, is_fcntl64)				      \
+     ? __fcntl_type_check_const_flock (arg) :				      \
+   __fcntl_is_flock (cmd, is_fcntl64) ? __fcntl_type_check_flock (arg) :      \
+   __fcntl_is_const_flock64 (cmd, is_fcntl64)				      \
+     ? __fcntl_type_check_const_flock64 (arg) :				      \
+   __fcntl_is_flock64 (cmd, is_fcntl64) ? __fcntl_type_check_flock64 (arg) :  \
+   1)
+
+
+__errordecl (__fcntl_missing_arg,
+             "fcntl with with this command needs 3 arguments");
+
+__fortify_function int
+__fcntl_2_inline (int __fd, int __cmd)
+{
+  if (!__builtin_constant_p (__cmd))
+    return __fcntl_alias (__fd, __cmd);
+
+  if (__fcntl_requires_arg (__cmd))
+    __fcntl_missing_arg ();
+
+  return __fcntl_alias (__fd, __cmd);
+}
+
+__glibc_warn_system_headers_begin
+
+#define fcntl(fd, cmd, ...)						      \
+  (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)				      \
+   __VA_OPT__ (:							      \
+     !__builtin_constant_p (cmd) ? __fcntl_alias (fd, cmd, __VA_ARGS__)	      \
+        : __fcntl_type_check (cmd, __VA_ARGS__, 0)			      \
+             ? __fcntl_alias (fd, cmd, __VA_ARGS__)			      \
+             : __fcntl_warn (fd, cmd, __VA_ARGS__)))
+
+#ifdef __USE_LARGEFILE64
+
+#define fcntl64(fd, cmd, ...)						      \
+  (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)				      \
+   __VA_OPT__ (:							      \
+     !__builtin_constant_p (cmd) ? __fcntl64_alias (fd, cmd, __VA_ARGS__)     \
+        : __fcntl_type_check (cmd, __VA_ARGS__, 1)			      \
+             ? __fcntl64_alias (fd, cmd, __VA_ARGS__)			      \
+             : __fcntl64_warn (fd, cmd, __VA_ARGS__)))
+
+
+#endif
+
+__glibc_warn_system_headers_end
diff --git a/io/fcntl.h b/io/fcntl.h
index dd620c08..61738f42 100644
--- a/io/fcntl.h
+++ b/io/fcntl.h
@@ -337,11 +337,17 @@ extern int posix_fallocate64 (int __fd, off64_t __offset, off64_t __len);
 
 
 /* Define some inlines helping to catch common problems.  */
+
 #if __USE_FORTIFY_LEVEL > 0 && defined __fortify_function \
     && defined __va_arg_pack_len
 # include <bits/fcntl2.h>
 #endif
 
+#if __USE_FORTIFY_LEVEL > 0 && defined (__fortify_function) \
+    && (__GNUC_PREREQ (8, 1) || __glibc_clang_prereq (12, 0))
+# include <bits/fcntl3.h>
+#endif
+
 __END_DECLS
 
 #endif /* fcntl.h  */
diff --git a/manual/maint.texi b/manual/maint.texi
index 89da704f..11509def 100644
--- a/manual/maint.texi
+++ b/manual/maint.texi
@@ -200,7 +200,7 @@ functions but may also include checks for validity of other inputs to
 the functions.
 
 When the @code{_FORTIFY_SOURCE} macro is defined, it enables code that
-validates inputs passed to some functions in @theglibc to determine if
+validates inputs passed to some functions in @theglibc{} to determine if
 they are safe.  If the compiler is unable to determine that the inputs
 to the function call are safe, the call may be replaced by a call to its
 hardened variant that does additional safety checks at runtime.  Some
@@ -221,7 +221,8 @@ returned by the @code{__builtin_object_size} compiler builtin function.
 If the function returns @code{(size_t) -1}, the function call is left
 untouched.  Additionally, this level also enables validation of flags to
 the @code{open}, @code{open64}, @code{openat} and @code{openat64}
-functions.
+functions, as well as validation of the presence and the type of the
+third argument to the @code{fcntl} and @code{fcntl64} functions.
 
 @item @math{2}: This behaves like @math{1}, with the addition of some
 checks that may trap code that is conforming but unsafe, e.g. accepting
@@ -267,6 +268,10 @@ The following functions and macros are fortified in @theglibc{}:
 
 @item @code{explicit_bzero}
 
+@item @code{fcntl}
+
+@item @code{fcntl64}
+
 @item @code{FD_SET}
 
 @item @code{FD_CLR}
-- 
2.41.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCH v3 5/5] io: Also verify 2-arg fctnl calls at runtime
  2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
                   ` (3 preceding siblings ...)
  2023-06-17 22:22 ` [PATCH v3 4/5] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev
@ 2023-06-17 22:22 ` Sergey Bugaev
  2023-06-19 12:58 ` [PATCH v3 0/5] fcntl fortification Carlos O'Donell
  5 siblings, 0 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-17 22:22 UTC (permalink / raw)
  To: libc-alpha

This adds a runtime fortification function, __fcntl_2, similar to
__open_2, that checks at runtime whether the actual passed-in command in
fact requires a third argument, and aborts if so.

The abilists have been modified with 'make update-abi-all'.

Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 debug/tst-fortify.c                           | 11 +++++++
 include/fcntl.h                               |  1 +
 io/Makefile                                   |  1 +
 io/Versions                                   |  3 ++
 io/bits/fcntl3.h                              |  4 ++-
 io/fcntl_2.c                                  | 33 +++++++++++++++++++
 manual/maint.texi                             | 10 +++---
 sysdeps/mach/hurd/i386/libc.abilist           |  1 +
 sysdeps/mach/hurd/x86_64/libc.abilist         |  1 +
 sysdeps/unix/sysv/linux/aarch64/libc.abilist  |  1 +
 sysdeps/unix/sysv/linux/alpha/libc.abilist    |  1 +
 sysdeps/unix/sysv/linux/arc/libc.abilist      |  1 +
 sysdeps/unix/sysv/linux/arm/be/libc.abilist   |  1 +
 sysdeps/unix/sysv/linux/arm/le/libc.abilist   |  1 +
 sysdeps/unix/sysv/linux/csky/libc.abilist     |  1 +
 sysdeps/unix/sysv/linux/hppa/libc.abilist     |  1 +
 sysdeps/unix/sysv/linux/i386/libc.abilist     |  1 +
 sysdeps/unix/sysv/linux/ia64/libc.abilist     |  1 +
 .../sysv/linux/loongarch/lp64/libc.abilist    |  1 +
 .../sysv/linux/m68k/coldfire/libc.abilist     |  1 +
 .../unix/sysv/linux/m68k/m680x0/libc.abilist  |  1 +
 .../sysv/linux/microblaze/be/libc.abilist     |  1 +
 .../sysv/linux/microblaze/le/libc.abilist     |  1 +
 .../sysv/linux/mips/mips32/fpu/libc.abilist   |  1 +
 .../sysv/linux/mips/mips32/nofpu/libc.abilist |  1 +
 .../sysv/linux/mips/mips64/n32/libc.abilist   |  1 +
 .../sysv/linux/mips/mips64/n64/libc.abilist   |  1 +
 sysdeps/unix/sysv/linux/nios2/libc.abilist    |  1 +
 sysdeps/unix/sysv/linux/or1k/libc.abilist     |  1 +
 .../linux/powerpc/powerpc32/fpu/libc.abilist  |  1 +
 .../powerpc/powerpc32/nofpu/libc.abilist      |  1 +
 .../linux/powerpc/powerpc64/be/libc.abilist   |  1 +
 .../linux/powerpc/powerpc64/le/libc.abilist   |  1 +
 .../unix/sysv/linux/riscv/rv32/libc.abilist   |  1 +
 .../unix/sysv/linux/riscv/rv64/libc.abilist   |  1 +
 .../unix/sysv/linux/s390/s390-32/libc.abilist |  1 +
 .../unix/sysv/linux/s390/s390-64/libc.abilist |  1 +
 sysdeps/unix/sysv/linux/sh/be/libc.abilist    |  1 +
 sysdeps/unix/sysv/linux/sh/le/libc.abilist    |  1 +
 .../sysv/linux/sparc/sparc32/libc.abilist     |  1 +
 .../sysv/linux/sparc/sparc64/libc.abilist     |  1 +
 .../unix/sysv/linux/x86_64/64/libc.abilist    |  1 +
 .../unix/sysv/linux/x86_64/x32/libc.abilist   |  1 +
 43 files changed, 94 insertions(+), 5 deletions(-)
 create mode 100644 io/fcntl_2.c

diff --git a/debug/tst-fortify.c b/debug/tst-fortify.c
index 674ada74..dc8b94fe 100644
--- a/debug/tst-fortify.c
+++ b/debug/tst-fortify.c
@@ -1901,6 +1901,12 @@ do_test (void)
     }
 #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);
@@ -1951,6 +1957,11 @@ do_test (void)
     }
 #endif
 
+# if __USE_FORTIFY_LEVEL >= 1
+  CHK_FAIL_START
+  fcntl64 (STDIN_FILENO, hide_constant (F_SETFD));
+  CHK_FAIL_END
+# endif
 #endif
 
   return ret;
diff --git a/include/fcntl.h b/include/fcntl.h
index be435047..cb86c5e7 100644
--- a/include/fcntl.h
+++ b/include/fcntl.h
@@ -32,6 +32,7 @@ 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);
 
 #if IS_IN (rtld)
 #  include <dl-fcntl.h>
diff --git a/io/Makefile b/io/Makefile
index 21e1914f..3cf96ae1 100644
--- a/io/Makefile
+++ b/io/Makefile
@@ -72,6 +72,7 @@ routines := \
   fchownat \
   fcntl \
   fcntl64 \
+  fcntl_2 \
   file_change_detection \
   flock \
   fstat \
diff --git a/io/Versions b/io/Versions
index 4e195408..0e77a287 100644
--- a/io/Versions
+++ b/io/Versions
@@ -140,6 +140,9 @@ libc {
   GLIBC_2.34 {
     closefrom;
   }
+  GLIBC_2.38 {
+    __fcntl_2;
+  }
   GLIBC_PRIVATE {
     __libc_fcntl64;
     __fcntl_nocancel;
diff --git a/io/bits/fcntl3.h b/io/bits/fcntl3.h
index 52a8a0d7..b6c390d0 100644
--- a/io/bits/fcntl3.h
+++ b/io/bits/fcntl3.h
@@ -20,6 +20,8 @@
 # error "Never include <bits/fcntl3.h> directly; use <fcntl.h> instead."
 #endif
 
+extern int __fcntl_2 (int __fd, int __cmd);
+
 #ifndef __USE_TIME_BITS64
 
 # ifndef __USE_FILE_OFFSET64
@@ -386,7 +388,7 @@ __fortify_function int
 __fcntl_2_inline (int __fd, int __cmd)
 {
   if (!__builtin_constant_p (__cmd))
-    return __fcntl_alias (__fd, __cmd);
+    return __fcntl_2 (__fd, __cmd);
 
   if (__fcntl_requires_arg (__cmd))
     __fcntl_missing_arg ();
diff --git a/io/fcntl_2.c b/io/fcntl_2.c
new file mode 100644
index 00000000..d9fd9c04
--- /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_requires_arg from bits/fcntl3.h */
+#undef _FORTIFY_SOURCE
+#define _FORTIFY_SOURCE 1
+
+#include <fcntl.h>
+#include <stdio.h>
+
+int
+__fcntl_2 (int fd, int cmd)
+{
+  if (__fcntl_requires_arg (cmd))
+    __fortify_fail ("invalid fcntl call: this cmd requires an argument");
+
+  return __libc_fcntl64 (fd, cmd);
+}
diff --git a/manual/maint.texi b/manual/maint.texi
index 11509def..9b24e5a0 100644
--- a/manual/maint.texi
+++ b/manual/maint.texi
@@ -244,10 +244,11 @@ depending on the architecture, one may also see fortified variants have
 the @code{_chkieee128} suffix or the @code{__nldbl___} prefix to their
 names.
 
-Another exception is the @code{open} family of functions, where their
-fortified replacements have the @code{__} prefix and a @code{_2} suffix.
-The @code{FD_SET}, @code{FD_CLR} and @code{FD_ISSET} macros use the
-@code{__fdelt_chk} function on fortification.
+Another exception is the @code{open} and @code{fcntl} families of
+functions, where their fortified 2-argument version replacements have the
+@code{__} prefix and a @code{_2} suffix. The @code{FD_SET}, @code{FD_CLR}
+and @code{FD_ISSET} macros use the @code{__fdelt_chk} function on
+fortification.
 
 The following functions and macros are fortified in @theglibc{}:
 @c Generated using the following command:
@@ -256,6 +257,7 @@ The following functions and macros are fortified in @theglibc{}:
 @c   sort -u | grep ^__ |
 @c   grep -v -e ieee128 -e __nldbl -e align_cpy -e "fdelt_warn" |
 @c   sed 's/__fdelt_chk/@item @code{FD_SET}\n\n@item @code{FD_CLR}\n\n@item @code{FD_ISSET}\n/' |
+@c   sed 's/__fcntl_2/@item @code{fcntl}\n\n@item @code{fcntl64}\n/' |
 @c   sed 's/__\(.*\)_\(chk\|2\)/@item @code{\1}\n/'
 
 @itemize @bullet
diff --git a/sysdeps/mach/hurd/i386/libc.abilist b/sysdeps/mach/hurd/i386/libc.abilist
index 74a9f427..c6d14e28 100644
--- a/sysdeps/mach/hurd/i386/libc.abilist
+++ b/sysdeps/mach/hurd/i386/libc.abilist
@@ -2294,6 +2294,7 @@ 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 __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 24db98d7..79b52cb8 100644
--- a/sysdeps/mach/hurd/x86_64/libc.abilist
+++ b/sysdeps/mach/hurd/x86_64/libc.abilist
@@ -194,6 +194,7 @@ 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 __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 c49363e7..581e4af0 100644
--- a/sysdeps/unix/sysv/linux/aarch64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/aarch64/libc.abilist
@@ -2633,6 +2633,7 @@ 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 __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 d6b1dcaa..76d52d79 100644
--- a/sysdeps/unix/sysv/linux/alpha/libc.abilist
+++ b/sysdeps/unix/sysv/linux/alpha/libc.abilist
@@ -2730,6 +2730,7 @@ 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 __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 dfe0c3f7..02285b53 100644
--- a/sysdeps/unix/sysv/linux/arc/libc.abilist
+++ b/sysdeps/unix/sysv/linux/arc/libc.abilist
@@ -2394,6 +2394,7 @@ 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 __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 6c75e5aa..a8fb5c54 100644
--- a/sysdeps/unix/sysv/linux/arm/be/libc.abilist
+++ b/sysdeps/unix/sysv/linux/arm/be/libc.abilist
@@ -514,6 +514,7 @@ 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 __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 03d6f7ae..b410dee2 100644
--- a/sysdeps/unix/sysv/linux/arm/le/libc.abilist
+++ b/sysdeps/unix/sysv/linux/arm/le/libc.abilist
@@ -511,6 +511,7 @@ 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 __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 d858c108..031c2259 100644
--- a/sysdeps/unix/sysv/linux/csky/libc.abilist
+++ b/sysdeps/unix/sysv/linux/csky/libc.abilist
@@ -2670,6 +2670,7 @@ 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 __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 82a14f8a..e32962c9 100644
--- a/sysdeps/unix/sysv/linux/hppa/libc.abilist
+++ b/sysdeps/unix/sysv/linux/hppa/libc.abilist
@@ -2619,6 +2619,7 @@ 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 __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 1950b15d..af6de37a 100644
--- a/sysdeps/unix/sysv/linux/i386/libc.abilist
+++ b/sysdeps/unix/sysv/linux/i386/libc.abilist
@@ -2803,6 +2803,7 @@ 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 __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 d0b9cb27..cb27a0e6 100644
--- a/sysdeps/unix/sysv/linux/ia64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/ia64/libc.abilist
@@ -2568,6 +2568,7 @@ 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 __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 e760a631..41c1f222 100644
--- a/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/loongarch/lp64/libc.abilist
@@ -2154,6 +2154,7 @@ GLIBC_2.36 wprintf F
 GLIBC_2.36 write F
 GLIBC_2.36 writev F
 GLIBC_2.36 wscanf 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 35785a3d..eabad188 100644
--- a/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist
+++ b/sysdeps/unix/sysv/linux/m68k/coldfire/libc.abilist
@@ -515,6 +515,7 @@ 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 __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 4ab2426e..dd4058ef 100644
--- a/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist
+++ b/sysdeps/unix/sysv/linux/m68k/m680x0/libc.abilist
@@ -2746,6 +2746,7 @@ 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 __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 38faa162..7c00eb03 100644
--- a/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist
+++ b/sysdeps/unix/sysv/linux/microblaze/be/libc.abilist
@@ -2719,6 +2719,7 @@ 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 __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 374d6589..d9de2cff 100644
--- a/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist
+++ b/sysdeps/unix/sysv/linux/microblaze/le/libc.abilist
@@ -2716,6 +2716,7 @@ 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 __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 fcc5e88e..6e67ce03 100644
--- a/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips32/fpu/libc.abilist
@@ -2711,6 +2711,7 @@ 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 __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 01eb96cd..4e9ac1a0 100644
--- a/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips32/nofpu/libc.abilist
@@ -2709,6 +2709,7 @@ 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 __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 a2748b7b..4d8717e3 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips64/n32/libc.abilist
@@ -2717,6 +2717,7 @@ 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 __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 0ae7ba49..c2c4afbc 100644
--- a/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/mips/mips64/n64/libc.abilist
@@ -2619,6 +2619,7 @@ 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 __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 947495a0..8ed46cd4 100644
--- a/sysdeps/unix/sysv/linux/nios2/libc.abilist
+++ b/sysdeps/unix/sysv/linux/nios2/libc.abilist
@@ -2758,6 +2758,7 @@ 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 __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 115f1039..35f17236 100644
--- a/sysdeps/unix/sysv/linux/or1k/libc.abilist
+++ b/sysdeps/unix/sysv/linux/or1k/libc.abilist
@@ -2140,6 +2140,7 @@ 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 __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 19c4c325..6cbc124e 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/fpu/libc.abilist
@@ -2773,6 +2773,7 @@ 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 __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 3e043c40..184f2126 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc32/nofpu/libc.abilist
@@ -2806,6 +2806,7 @@ 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 __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 e4f3a766..c572b596 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/be/libc.abilist
@@ -2527,6 +2527,7 @@ 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 __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 dafe1c4a..2715881f 100644
--- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist
+++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/le/libc.abilist
@@ -2829,6 +2829,7 @@ 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 __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 b9740a1a..65c1f6fd 100644
--- a/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/riscv/rv32/libc.abilist
@@ -2396,6 +2396,7 @@ 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 __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 e3b4656a..29d33412 100644
--- a/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/riscv/rv64/libc.abilist
@@ -2596,6 +2596,7 @@ 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 __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 84cb7a50..265e1294 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/s390/s390-32/libc.abilist
@@ -2771,6 +2771,7 @@ 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 __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 33df3b16..c9f522c4 100644
--- a/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/s390/s390-64/libc.abilist
@@ -2564,6 +2564,7 @@ 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 __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 94cbccd7..fa413456 100644
--- a/sysdeps/unix/sysv/linux/sh/be/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sh/be/libc.abilist
@@ -2626,6 +2626,7 @@ 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 __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 3bb316a7..6c02c3e2 100644
--- a/sysdeps/unix/sysv/linux/sh/le/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sh/le/libc.abilist
@@ -2623,6 +2623,7 @@ 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 __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 6341b491..5cc5d128 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sparc/sparc32/libc.abilist
@@ -2766,6 +2766,7 @@ 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 __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 8ed1ea29..2279a88e 100644
--- a/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/sparc/sparc64/libc.abilist
@@ -2591,6 +2591,7 @@ 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 __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 57cfcc20..dd8af45d 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/libc.abilist
@@ -2542,6 +2542,7 @@ 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 __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 3f0a9f6d..8e4ad424 100644
--- a/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/x32/libc.abilist
@@ -2648,6 +2648,7 @@ 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 __fcntl_2 F
 GLIBC_2.38 __isoc23_fscanf F
 GLIBC_2.38 __isoc23_fwscanf F
 GLIBC_2.38 __isoc23_scanf F
-- 
2.41.0


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
                   ` (4 preceding siblings ...)
  2023-06-17 22:22 ` [PATCH v3 5/5] io: Also verify 2-arg fctnl calls at runtime Sergey Bugaev
@ 2023-06-19 12:58 ` Carlos O'Donell
  2023-06-19 14:23   ` Sergey Bugaev
  5 siblings, 1 reply; 22+ messages in thread
From: Carlos O'Donell @ 2023-06-19 12:58 UTC (permalink / raw)
  To: Sergey Bugaev, libc-alpha

On 6/17/23 18:22, Sergey Bugaev via Libc-alpha wrote:
> Hello,
> 
> this is v3 of the fcntl fortification work. v1 was at [0], v2 at [1].
> 
> [0]: https://sourceware.org/pipermail/libc-alpha/2023-May/148332.html
> [1]: https://sourceware.org/pipermail/libc-alpha/2023-May/148569.html

Pre-commit CI regression caused by this series on both aarch64 and aarch32.
https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/

# Running glibc:io ...
# FAIL: io/check-installed-headers-c 
# FAIL: io/check-installed-headers-cxx 
# 
# Running glibc:misc ...
# FAIL: misc/check-installed-headers-c 
# FAIL: misc/check-installed-headers-cxx 
# 
# Running glibc:rt ...
# FAIL: rt/check-installed-headers-c 
# FAIL: rt/check-installed-headers-cxx 


> Changes since v2:
> 
> - This is now in its own separate header, bits/fcntl3.h
>   (bits/fcntl2.h is used by the open fortification)
> 
> - Clang is now supported in addition to GCC!
>   - Clang does not support nor need the "-Wsystem-headers" pragma
>   - Clang does support error/warning attributes since recently
>   - There seems to be a bug in Clang which prevents the type mismatch
>     warning from actually firing. Specifically, it appears that Clang
>     gets confused about C functions names vs symbol names when it comes
>     to attribute ((warning)), and does not emit the warning if the
>     function declared with __warnattr has a symbol name matching that of
>     another function that has not been declared with __warnattr.
> 
>     While this could be worked around in glibc (such as by adding
>     __fcntl_warn as a real wrapper function when built with Clang), I
>     think this just needs to be fixed in Clang. Any LLVM developers
>     here? :D
> 
> - Changed hide_constant utility to use an empty inline asm statement
>   instead of volatile and noinline, as per the discussion. I did not
>   make this into a general-purpose glibc-wide utility because I don't
>   know what a fitting name and place (header) for it would be. If you'd
>   like to see it glibc-wide, please suggest me where to put it and how
>   to name it!
> 
> - Fixed the C++ template linkage thing
> 
> - Addressed misc review comments
> 
> - Looked into applying __builtin_constant_p to the result of the cmd
>   check and not the cmd value itself, as suggested by Florian.
> 
>   Unfortunately this does not work at all :( __builtin_constant_p starts
>   returning 0 given anything remotely complex like even a trivial inline
>   function call (so technically hide_constant would still work if it was
>   just 'return value;'), even if the function is (later?) fully inlined
>   and const-folded.
> 
>   *Maybe* this could be made to work if I used an obscene amount of
>   macros instead of inline functions (to handle all the various commands
>   being conditionally defined), but I don't want to go there.
> 
>   So since this didn't work out, I left the runtime __fcntl_2 function,
>   but split it into a separate patch, so you can apply it or drop it
>   depending on what you prefer in the end.
> 
> Clang / C++ demo:
> 
> ------------------------------------------------------------------
> $ clang++ test-fcntl.cpp -D _FORTIFY_SOURCE=2 -O2
> In file included from test-fcntl.cpp:1:
> In file included from /usr/include/fcntl.h:348:
> /usr/include/bits/fcntl3.h:394:5: error: call to '__fcntl_missing_arg' declared with 'error' attribute: fcntl with with this command needs 3 arguments
>     __fcntl_missing_arg ();
>     ^
> 1 error generated.
> ------------------------------------------------------------------
> 
> Sergey
> 

-- 
Cheers,
Carlos.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-19 12:58 ` [PATCH v3 0/5] fcntl fortification Carlos O'Donell
@ 2023-06-19 14:23   ` Sergey Bugaev
  2023-06-19 18:36     ` Adhemerval Zanella Netto
  0 siblings, 1 reply; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-19 14:23 UTC (permalink / raw)
  To: Carlos O'Donell; +Cc: libc-alpha

On Mon, Jun 19, 2023 at 3:58 PM Carlos O'Donell <carlos@redhat.com> wrote:
> Pre-commit CI regression caused by this series on both aarch64 and aarch32.
> https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/
>
> # Running glibc:io ...
> # FAIL: io/check-installed-headers-c
> # FAIL: io/check-installed-headers-cxx
> #
> # Running glibc:misc ...
> # FAIL: misc/check-installed-headers-c
> # FAIL: misc/check-installed-headers-cxx
> #
> # Running glibc:rt ...
> # FAIL: rt/check-installed-headers-c
> # FAIL: rt/check-installed-headers-cxx

Thank you :(

I'm a little bit lost in the Jenkins web UI -- is there anywhere I can
view the output of the failing tests to understand what exactly went
wrong? I see the sum files here [0], but not the .out files.

[0]: https://ci.linaro.org/job/tcwg_glibc_check--master-arm-build/386/artifact/artifacts/artifacts.precommit/sumfiles/

Sergey

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-19 14:23   ` Sergey Bugaev
@ 2023-06-19 18:36     ` Adhemerval Zanella Netto
  2023-06-20  5:59       ` Maxim Kuvyrkov
  2023-07-21 13:59       ` Adhemerval Zanella Netto
  0 siblings, 2 replies; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-06-19 18:36 UTC (permalink / raw)
  To: Sergey Bugaev, Carlos O'Donell, Maxim Kuvyrkov; +Cc: libc-alpha



On 19/06/23 11:23, Sergey Bugaev via Libc-alpha wrote:
> On Mon, Jun 19, 2023 at 3:58 PM Carlos O'Donell <carlos@redhat.com> wrote:
>> Pre-commit CI regression caused by this series on both aarch64 and aarch32.
>> https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/
>>
>> # Running glibc:io ...
>> # FAIL: io/check-installed-headers-c
>> # FAIL: io/check-installed-headers-cxx
>> #
>> # Running glibc:misc ...
>> # FAIL: misc/check-installed-headers-c
>> # FAIL: misc/check-installed-headers-cxx
>> #
>> # Running glibc:rt ...
>> # FAIL: rt/check-installed-headers-c
>> # FAIL: rt/check-installed-headers-cxx
> 
> Thank you :(
> 
> I'm a little bit lost in the Jenkins web UI -- is there anywhere I can
> view the output of the failing tests to understand what exactly went
> wrong? I see the sum files here [0], but not the .out files.
> 
> [0]: https://ci.linaro.org/job/tcwg_glibc_check--master-arm-build/386/artifact/artifacts/artifacts.precommit/sumfiles/

I could not reproduce it locally, so I think it is a limitation of our 
(Linaro) CI framework, where not all tests artifacts are being removed 
with make test-clean. Maxim already sent a fix for this [1].

[1] https://patchwork.sourceware.org/project/glibc/patch/20230615152547.2924770-1-maxim.kuvyrkov@linaro.org/

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-19 18:36     ` Adhemerval Zanella Netto
@ 2023-06-20  5:59       ` Maxim Kuvyrkov
  2023-06-20  7:33         ` Sergey Bugaev
  2023-06-20 12:50         ` Adhemerval Zanella Netto
  2023-07-21 13:59       ` Adhemerval Zanella Netto
  1 sibling, 2 replies; 22+ messages in thread
From: Maxim Kuvyrkov @ 2023-06-20  5:59 UTC (permalink / raw)
  To: Sergey Bugaev; +Cc: Carlos O'Donell, Adhemerval Zanella Netto, Libc-alpha

[-- Attachment #1: Type: text/plain, Size: 2560 bytes --]

> On Jun 19, 2023, at 22:36, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
> 
> 
> 
> On 19/06/23 11:23, Sergey Bugaev via Libc-alpha wrote:
>> On Mon, Jun 19, 2023 at 3:58 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>> Pre-commit CI regression caused by this series on both aarch64 and aarch32.
>>> https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/
>>> 
>>> # Running glibc:io ...
>>> # FAIL: io/check-installed-headers-c
>>> # FAIL: io/check-installed-headers-cxx
>>> #
>>> # Running glibc:misc ...
>>> # FAIL: misc/check-installed-headers-c
>>> # FAIL: misc/check-installed-headers-cxx
>>> #
>>> # Running glibc:rt ...
>>> # FAIL: rt/check-installed-headers-c
>>> # FAIL: rt/check-installed-headers-cxx
>> 
>> Thank you :(
>> 
>> I'm a little bit lost in the Jenkins web UI -- is there anywhere I can
>> view the output of the failing tests to understand what exactly went
>> wrong? I see the sum files here [0], but not the .out files.
>> 
>> [0]: https://ci.linaro.org/job/tcwg_glibc_check--master-arm-build/386/artifact/artifacts/artifacts.precommit/sumfiles/

Hi Sergey,

See the attached tarball with output from failed tests:
===
Running glibc:io ...
FAIL: io/check-installed-headers-c 
FAIL: io/check-installed-headers-cxx 

Running glibc:misc ...
FAIL: misc/check-installed-headers-c 
FAIL: misc/check-installed-headers-cxx 

Running glibc:rt ...
FAIL: rt/check-installed-headers-c 
FAIL: rt/check-installed-headers-cxx 
===

Let me know if you need any help in reproducing these.  The results are for Ubuntu 22.04 armhf target, but results for Ubuntu 22.04 aarch64 show the same regressions.

Interestingly the top-level check-installed-headers-* tests PASS:
===
PASS: check-installed-headers-c
PASS: check-installed-headers-cxx
===

> 
> I could not reproduce it locally, so I think it is a limitation of our 
> (Linaro) CI framework, where not all tests artifacts are being removed 
> with make test-clean. Maxim already sent a fix for this [1].
> 
> [1] https://patchwork.sourceware.org/project/glibc/patch/20230615152547.2924770-1-maxim.kuvyrkov@linaro.org/

Hi Adhemerval,

We (Linaro) have fixed or added workarounds for all known testsuite stability problems in our CI before enabling reporting for failed patches.  I am surprised that you could not reproduce the above failures in your setup; let's troubleshoot your local setup offline.

Kind regards,

--
Maxim Kuvyrkov
https://www.linaro.org




[-- Attachment #2: series-21482.tar.xz --]
[-- Type: application/x-xz, Size: 2520 bytes --]

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20  5:59       ` Maxim Kuvyrkov
@ 2023-06-20  7:33         ` Sergey Bugaev
  2023-06-20  9:41           ` Maxim Kuvyrkov
  2023-06-20 12:50         ` Adhemerval Zanella Netto
  1 sibling, 1 reply; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-20  7:33 UTC (permalink / raw)
  To: Maxim Kuvyrkov; +Cc: Carlos O'Donell, Adhemerval Zanella Netto, Libc-alpha

On Tue, Jun 20, 2023 at 8:59 AM Maxim Kuvyrkov
<maxim.kuvyrkov@linaro.org> wrote:
> Hi Sergey,
>
> See the attached tarball with output from failed tests

Hi, and thank you -- so it's just that F_DUPFD_CLOEXEC is defined
conditionally (ifdef __USE_XOPEN2K8), that'd be easy to fix. I don't see
why it would vary depending on CPU architecture though.

Sergey

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20  7:33         ` Sergey Bugaev
@ 2023-06-20  9:41           ` Maxim Kuvyrkov
  2023-06-20 11:28             ` Sergey Bugaev
  0 siblings, 1 reply; 22+ messages in thread
From: Maxim Kuvyrkov @ 2023-06-20  9:41 UTC (permalink / raw)
  To: Sergey Bugaev; +Cc: Carlos O'Donell, Adhemerval Zanella Netto, Libc-alpha

> On Jun 20, 2023, at 11:33, Sergey Bugaev <bugaevc@gmail.com> wrote:
> 
> On Tue, Jun 20, 2023 at 8:59 AM Maxim Kuvyrkov
> <maxim.kuvyrkov@linaro.org> wrote:
>> Hi Sergey,
>> 
>> See the attached tarball with output from failed tests
> 
> Hi, and thank you -- so it's just that F_DUPFD_CLOEXEC is defined
> conditionally (ifdef __USE_XOPEN2K8), that'd be easy to fix. I don't see
> why it would vary depending on CPU architecture though.

I don't think CPU architecture plays a role here either.  My guess (didn't verify) is that the difference comes from GCC version and its default C standard.

In our case we are running CI in latest Ubuntu LTS (22.04 at the moment), which has GCC 11.

What environment did you test this in?

Kind regards,

--
Maxim Kuvyrkov
https://www.linaro.org


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20  9:41           ` Maxim Kuvyrkov
@ 2023-06-20 11:28             ` Sergey Bugaev
  2023-06-20 12:38               ` Maxim Kuvyrkov
  0 siblings, 1 reply; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-20 11:28 UTC (permalink / raw)
  To: Maxim Kuvyrkov; +Cc: Carlos O'Donell, Adhemerval Zanella Netto, Libc-alpha

On Tue, Jun 20, 2023 at 12:41 PM Maxim Kuvyrkov
<maxim.kuvyrkov@linaro.org> wrote:
> I don't think CPU architecture plays a role here either.  My guess (didn't verify) is that the difference comes from GCC version and its default C standard.

But the C standard is being explicitly set in these tests (those
-std=c89 flags), no?

> In our case we are running CI in latest Ubuntu LTS (22.04 at the moment), which has GCC 11.
>
> What environment did you test this in?

I have GCC 13.1 and Clang 16.0 targeting {x86_64,i686}-linux-gnu, GCC
12.2 targeting i686-gnu, and GCC master targeting x86_64-gnu. I have
only run the full testsuite on x86_64-linux-gnu (and it does pass,
including all the check-installed-headers tests); for the other
variants I've only checked debug/tst-fortify (which also tries to
compile the header in various configurations) and tried to build
sample code against the installed headers manually (but have not
checked different C standards and _XXXX_SOURCE definitions while doing
that). I have also done some checks on Compiler Explorer, and indeed I
can reproduce F_DUPFD_CLOEXEC being unavailable with -std=c89 (or any
non-GNU C, e.g. -std=c11) there -- but that happens when targeting
x86_64 too.

...Having written the above, I went and re-ran the test suite, and
traced which files GCC opens, and it does not even look at fcntl3.h
when running check-installed-headers! That explains it, the CI must
have _FORTIFY_SOURCE set in the CFLAGS for glibc itself, but I don't,
and Adhemerval probably doesn't either. With _FORTIFY_SOURCE manually
added, I can reproduce the exact test failure on my setup too.

Thanks!

Sergey

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20 11:28             ` Sergey Bugaev
@ 2023-06-20 12:38               ` Maxim Kuvyrkov
  2023-06-20 12:53                 ` Sergey Bugaev
  0 siblings, 1 reply; 22+ messages in thread
From: Maxim Kuvyrkov @ 2023-06-20 12:38 UTC (permalink / raw)
  To: Sergey Bugaev; +Cc: Carlos O'Donell, Adhemerval Zanella Netto, Libc-alpha

> On Jun 20, 2023, at 15:28, Sergey Bugaev <bugaevc@gmail.com> wrote:
> 
> On Tue, Jun 20, 2023 at 12:41 PM Maxim Kuvyrkov
> <maxim.kuvyrkov@linaro.org> wrote:
>> I don't think CPU architecture plays a role here either.  My guess (didn't verify) is that the difference comes from GCC version and its default C standard.
> 
> But the C standard is being explicitly set in these tests (those
> -std=c89 flags), no?
> 
>> In our case we are running CI in latest Ubuntu LTS (22.04 at the moment), which has GCC 11.
>> 
>> What environment did you test this in?
> 
> I have GCC 13.1 and Clang 16.0 targeting {x86_64,i686}-linux-gnu, GCC
> 12.2 targeting i686-gnu, and GCC master targeting x86_64-gnu. I have
> only run the full testsuite on x86_64-linux-gnu (and it does pass,
> including all the check-installed-headers tests); for the other
> variants I've only checked debug/tst-fortify (which also tries to
> compile the header in various configurations) and tried to build
> sample code against the installed headers manually (but have not
> checked different C standards and _XXXX_SOURCE definitions while doing
> that). I have also done some checks on Compiler Explorer, and indeed I
> can reproduce F_DUPFD_CLOEXEC being unavailable with -std=c89 (or any
> non-GNU C, e.g. -std=c11) there -- but that happens when targeting
> x86_64 too.
> 
> ...Having written the above, I went and re-ran the test suite, and
> traced which files GCC opens, and it does not even look at fcntl3.h
> when running check-installed-headers! That explains it, the CI must
> have _FORTIFY_SOURCE set in the CFLAGS for glibc itself, but I don't,
> and Adhemerval probably doesn't either. With _FORTIFY_SOURCE manually
> added, I can reproduce the exact test failure on my setup too.

We don't set _FORTIFY_SOURCE in our CI's glibc build, but, I think, it comes from Ubuntu's GCC, where it may be enabled by default.  Or are you using Ubuntu and not seeing this with default Ubuntu toolchain?

--
Maxim Kuvyrkov
https://www.linaro.org


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20  5:59       ` Maxim Kuvyrkov
  2023-06-20  7:33         ` Sergey Bugaev
@ 2023-06-20 12:50         ` Adhemerval Zanella Netto
  2023-06-20 14:21           ` Frederic Berat
  1 sibling, 1 reply; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-06-20 12:50 UTC (permalink / raw)
  To: Maxim Kuvyrkov, Sergey Bugaev
  Cc: Carlos O'Donell, Libc-alpha, Frédéric Bérat



On 20/06/23 02:59, Maxim Kuvyrkov wrote:
>> On Jun 19, 2023, at 22:36, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 19/06/23 11:23, Sergey Bugaev via Libc-alpha wrote:
>>> On Mon, Jun 19, 2023 at 3:58 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>>> Pre-commit CI regression caused by this series on both aarch64 and aarch32.
>>>> https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/
>>>>
>>>> # Running glibc:io ...
>>>> # FAIL: io/check-installed-headers-c
>>>> # FAIL: io/check-installed-headers-cxx
>>>> #
>>>> # Running glibc:misc ...
>>>> # FAIL: misc/check-installed-headers-c
>>>> # FAIL: misc/check-installed-headers-cxx
>>>> #
>>>> # Running glibc:rt ...
>>>> # FAIL: rt/check-installed-headers-c
>>>> # FAIL: rt/check-installed-headers-cxx
>>>
>>> Thank you :(
>>>
>>> I'm a little bit lost in the Jenkins web UI -- is there anywhere I can
>>> view the output of the failing tests to understand what exactly went
>>> wrong? I see the sum files here [0], but not the .out files.
>>>
>>> [0]: https://ci.linaro.org/job/tcwg_glibc_check--master-arm-build/386/artifact/artifacts/artifacts.precommit/sumfiles/
> 
> Hi Sergey,
> 
> See the attached tarball with output from failed tests:
> ===
> Running glibc:io ...
> FAIL: io/check-installed-headers-c 
> FAIL: io/check-installed-headers-cxx 
> 
> Running glibc:misc ...
> FAIL: misc/check-installed-headers-c 
> FAIL: misc/check-installed-headers-cxx 
> 
> Running glibc:rt ...
> FAIL: rt/check-installed-headers-c 
> FAIL: rt/check-installed-headers-cxx 
> ===
> 
> Let me know if you need any help in reproducing these.  The results are for Ubuntu 22.04 armhf target, but results for Ubuntu 22.04 aarch64 show the same regressions.
> 
> Interestingly the top-level check-installed-headers-* tests PASS:
> ===
> PASS: check-installed-headers-c
> PASS: check-installed-headers-cxx
> ===
> 
>>
>> I could not reproduce it locally, so I think it is a limitation of our 
>> (Linaro) CI framework, where not all tests artifacts are being removed 
>> with make test-clean. Maxim already sent a fix for this [1].
>>
>> [1] https://patchwork.sourceware.org/project/glibc/patch/20230615152547.2924770-1-maxim.kuvyrkov@linaro.org/
> 
> Hi Adhemerval,
> 
> We (Linaro) have fixed or added workarounds for all known testsuite stability problems in our CI before enabling reporting for failed patches.  I am surprised that you could not reproduce the above failures in your setup; let's troubleshoot your local setup offline.

I think I figure out what is happening here. I am using toolchains built with
scripts/build-many-glibcs.py which does not set fortify as default, different
than distro that are now setting the it as default.

Since in our CI we use the system compiler to check for header, fortify will
be always enabled and thus breaking the testing.

Frédéric is fixing a lot of tests to assume fortify is enabled as default, so
maybe it is something that he is already tracking.  In any case, I think we
should fix scripts/check-installed-headers.sh to check for different levels
of _FORTIFY_SOURCE along with already in places flags.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20 12:38               ` Maxim Kuvyrkov
@ 2023-06-20 12:53                 ` Sergey Bugaev
  2023-06-20 13:40                   ` Adhemerval Zanella Netto
  2023-06-20 13:46                   ` Zack Weinberg
  0 siblings, 2 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-06-20 12:53 UTC (permalink / raw)
  To: Maxim Kuvyrkov; +Cc: Carlos O'Donell, Adhemerval Zanella Netto, Libc-alpha

On Tue, Jun 20, 2023 at 3:38 PM Maxim Kuvyrkov
<maxim.kuvyrkov@linaro.org> wrote:
> We don't set _FORTIFY_SOURCE in our CI's glibc build, but, I think, it comes from Ubuntu's GCC, where it may be enabled by default.  Or are you using Ubuntu and not seeing this with default Ubuntu toolchain?

I'm not using Ubuntu.

Do they just set _FORTIFY_SOURCE by default -- i.e. not only when
building OS packages, but for all compilations? That's... unusual :|

It's also concerning that check-installed-headers doesn't check this
configuration (_FORTIFY_SOURCE with non-GNU C standard), and we're
only finding out about this by accident. There's a comment in
check-installed-headers.sh that says:

# An exhaustive test of feature selection macros would take far too long.
# These are probably the most commonly used three.

That makes sense when running the testsuite locally since you want it
to finish in minutes, not days, but wouldn't checking all combinations
(or at least _a lot_ more of them) make more sense for CI?

Sergey

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20 12:53                 ` Sergey Bugaev
@ 2023-06-20 13:40                   ` Adhemerval Zanella Netto
  2023-06-20 13:47                     ` Zack Weinberg
  2023-06-20 13:46                   ` Zack Weinberg
  1 sibling, 1 reply; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-06-20 13:40 UTC (permalink / raw)
  To: Sergey Bugaev, Maxim Kuvyrkov; +Cc: Carlos O'Donell, Libc-alpha



On 20/06/23 09:53, Sergey Bugaev wrote:
> On Tue, Jun 20, 2023 at 3:38 PM Maxim Kuvyrkov
> <maxim.kuvyrkov@linaro.org> wrote:
>> We don't set _FORTIFY_SOURCE in our CI's glibc build, but, I think, it comes from Ubuntu's GCC, where it may be enabled by default.  Or are you using Ubuntu and not seeing this with default Ubuntu toolchain?
> 
> I'm not using Ubuntu.
> 
> Do they just set _FORTIFY_SOURCE by default -- i.e. not only when
> building OS packages, but for all compilations? That's... unusual :|

It does for any optimized build:

$ gcc -v 2>&1 | grep 'gcc version'
gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04.1)
$ gcc -dM -E - < /dev/null | grep -w _FORTIFY_SOURCE
$ gcc -O2 -dM -E - < /dev/null | grep -w _FORTIFY_SOURCE
#define _FORTIFY_SOURCE 2

And I think this is a common configuration for recent distros.

> 
> It's also concerning that check-installed-headers doesn't check this
> configuration (_FORTIFY_SOURCE with non-GNU C standard), and we're
> only finding out about this by accident. There's a comment in
> check-installed-headers.sh that says:
> 
> # An exhaustive test of feature selection macros would take far too long.
> # These are probably the most commonly used three.
> 
> That makes sense when running the testsuite locally since you want it
> to finish in minutes, not days, but wouldn't checking all combinations
> (or at least _a lot_ more of them) make more sense for CI?
> 

I think it is worth to extend check-installed-headers.sh to include 
fortify as well, specially because it is now being enabled as default
on multiple configurations.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20 12:53                 ` Sergey Bugaev
  2023-06-20 13:40                   ` Adhemerval Zanella Netto
@ 2023-06-20 13:46                   ` Zack Weinberg
  1 sibling, 0 replies; 22+ messages in thread
From: Zack Weinberg @ 2023-06-20 13:46 UTC (permalink / raw)
  To: Sergey Bugaev, Maxim Kuvyrkov
  Cc: Carlos O'Donell, Adhemerval Zanella, GNU libc development

On Tue, Jun 20, 2023, at 8:53 AM, Sergey Bugaev via Libc-alpha wrote:
> I'm not using Ubuntu.
> Do they just set _FORTIFY_SOURCE by default -- i.e. not only when
> building OS packages, but for all compilations? That's... unusual :|

To me it looks like they only set it when building packages:

$ lsb_release -a
No LSB modules are available.
Distributor ID:	Ubuntu
Description:	Ubuntu 22.04.1 LTS
Release:	22.04
Codename:	jammy

$ which gcc
/usr/bin/gcc

$ echo '#include <string.h>' | gcc -E -dD -xc - | grep FORTIFY
#undef __USE_FORTIFY_LEVEL
#define __USE_FORTIFY_LEVEL 0
#define __bos(ptr) __builtin_object_size (ptr, __USE_FORTIFY_LEVEL > 1)

$ dpkg-buildflags | grep FORTIFY
CPPFLAGS=-Wdate-time -D_FORTIFY_SOURCE=2

> It's also concerning that check-installed-headers doesn't check this
> configuration (_FORTIFY_SOURCE with non-GNU C standard), and we're
> only finding out about this by accident. There's a comment in
> check-installed-headers.sh that says:
>
> # An exhaustive test of feature selection macros would take far too long.
> # These are probably the most commonly used three.
>
> That makes sense when running the testsuite locally since you want it
> to finish in minutes, not days, but wouldn't checking all combinations
> (or at least _a lot_ more of them) make more sense for CI?

When I wrote check-installed-headers, IIRC, the only CI we had was
Joseph's build-many-glibcs bot, which was already struggling to keep
up just with the load from its own built in list of configurations.

It may well make sense to change this now, and I agree that permutations
involving _FORTIFY_SOURCE should be a priority since fortification makes
so many changes to important headers.

zw

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20 13:40                   ` Adhemerval Zanella Netto
@ 2023-06-20 13:47                     ` Zack Weinberg
  0 siblings, 0 replies; 22+ messages in thread
From: Zack Weinberg @ 2023-06-20 13:47 UTC (permalink / raw)
  To: Adhemerval Zanella, Sergey Bugaev, Maxim Kuvyrkov
  Cc: Carlos O'Donell, GNU libc development

On Tue, Jun 20, 2023, at 9:40 AM, Adhemerval Zanella Netto via Libc-alpha wrote:
> On 20/06/23 09:53, Sergey Bugaev wrote:
>> On Tue, Jun 20, 2023 at 3:38 PM Maxim Kuvyrkov
>> <maxim.kuvyrkov@linaro.org> wrote:
>>> We don't set _FORTIFY_SOURCE in our CI's glibc build, but, I think, it comes from Ubuntu's GCC, where it may be enabled by default.  Or are you using Ubuntu and not seeing this with default Ubuntu toolchain?
>> 
>> I'm not using Ubuntu.
>> 
>> Do they just set _FORTIFY_SOURCE by default -- i.e. not only when
>> building OS packages, but for all compilations? That's... unusual :|
>
> It does for any optimized build:
>
> $ gcc -v 2>&1 | grep 'gcc version'
> gcc version 11.3.0 (Ubuntu 11.3.0-1ubuntu1~22.04.1)
> $ gcc -dM -E - < /dev/null | grep -w _FORTIFY_SOURCE
> $ gcc -O2 -dM -E - < /dev/null | grep -w _FORTIFY_SOURCE
> #define _FORTIFY_SOURCE 2
>
> And I think this is a common configuration for recent distros.

Argh, I should have thought to try turning on optimization :-/

zw

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-20 12:50         ` Adhemerval Zanella Netto
@ 2023-06-20 14:21           ` Frederic Berat
  0 siblings, 0 replies; 22+ messages in thread
From: Frederic Berat @ 2023-06-20 14:21 UTC (permalink / raw)
  To: Adhemerval Zanella Netto
  Cc: Maxim Kuvyrkov, Sergey Bugaev, Carlos O'Donell, Libc-alpha

On Tue, Jun 20, 2023 at 2:50 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 20/06/23 02:59, Maxim Kuvyrkov wrote:
> >> On Jun 19, 2023, at 22:36, Adhemerval Zanella Netto <adhemerval.zanella@linaro.org> wrote:
> >>
> >>
> >>
> >> On 19/06/23 11:23, Sergey Bugaev via Libc-alpha wrote:
> >>> On Mon, Jun 19, 2023 at 3:58 PM Carlos O'Donell <carlos@redhat.com> wrote:
> >>>> Pre-commit CI regression caused by this series on both aarch64 and aarch32.
> >>>> https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/
> >>>>
> >>>> # Running glibc:io ...
> >>>> # FAIL: io/check-installed-headers-c
> >>>> # FAIL: io/check-installed-headers-cxx
> >>>> #
> >>>> # Running glibc:misc ...
> >>>> # FAIL: misc/check-installed-headers-c
> >>>> # FAIL: misc/check-installed-headers-cxx
> >>>> #
> >>>> # Running glibc:rt ...
> >>>> # FAIL: rt/check-installed-headers-c
> >>>> # FAIL: rt/check-installed-headers-cxx
> >>>
> >>> Thank you :(
> >>>
> >>> I'm a little bit lost in the Jenkins web UI -- is there anywhere I can
> >>> view the output of the failing tests to understand what exactly went
> >>> wrong? I see the sum files here [0], but not the .out files.
> >>>
> >>> [0]: https://ci.linaro.org/job/tcwg_glibc_check--master-arm-build/386/artifact/artifacts/artifacts.precommit/sumfiles/
> >
> > Hi Sergey,
> >
> > See the attached tarball with output from failed tests:
> > ===
> > Running glibc:io ...
> > FAIL: io/check-installed-headers-c
> > FAIL: io/check-installed-headers-cxx
> >
> > Running glibc:misc ...
> > FAIL: misc/check-installed-headers-c
> > FAIL: misc/check-installed-headers-cxx
> >
> > Running glibc:rt ...
> > FAIL: rt/check-installed-headers-c
> > FAIL: rt/check-installed-headers-cxx
> > ===
> >
> > Let me know if you need any help in reproducing these.  The results are for Ubuntu 22.04 armhf target, but results for Ubuntu 22.04 aarch64 show the same regressions.
> >
> > Interestingly the top-level check-installed-headers-* tests PASS:
> > ===
> > PASS: check-installed-headers-c
> > PASS: check-installed-headers-cxx
> > ===
> >
> >>
> >> I could not reproduce it locally, so I think it is a limitation of our
> >> (Linaro) CI framework, where not all tests artifacts are being removed
> >> with make test-clean. Maxim already sent a fix for this [1].
> >>
> >> [1] https://patchwork.sourceware.org/project/glibc/patch/20230615152547.2924770-1-maxim.kuvyrkov@linaro.org/
> >
> > Hi Adhemerval,
> >
> > We (Linaro) have fixed or added workarounds for all known testsuite stability problems in our CI before enabling reporting for failed patches.  I am surprised that you could not reproduce the above failures in your setup; let's troubleshoot your local setup offline.
>
> I think I figure out what is happening here. I am using toolchains built with
> scripts/build-many-glibcs.py which does not set fortify as default, different
> than distro that are now setting the it as default.
>
> Since in our CI we use the system compiler to check for header, fortify will
> be always enabled and thus breaking the testing.
>
> Frédéric is fixing a lot of tests to assume fortify is enabled as default, so
> maybe it is something that he is already tracking.  In any case, I think we
> should fix scripts/check-installed-headers.sh to check for different levels
> of _FORTIFY_SOURCE along with already in places flags.
>

I'm indeed working on a patch series to enable _FORTIFY_SOURCE while
building Glibc, which may improve your situation.
I expect the patchset will be out in the next few days (or even hours
if everything goes right), so you might be able to check soon.


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-06-19 18:36     ` Adhemerval Zanella Netto
  2023-06-20  5:59       ` Maxim Kuvyrkov
@ 2023-07-21 13:59       ` Adhemerval Zanella Netto
  2023-07-21 15:50         ` Sergey Bugaev
  1 sibling, 1 reply; 22+ messages in thread
From: Adhemerval Zanella Netto @ 2023-07-21 13:59 UTC (permalink / raw)
  To: Sergey Bugaev, Carlos O'Donell, Maxim Kuvyrkov; +Cc: libc-alpha



On 19/06/23 15:36, Adhemerval Zanella Netto wrote:
> 
> 
> On 19/06/23 11:23, Sergey Bugaev via Libc-alpha wrote:
>> On Mon, Jun 19, 2023 at 3:58 PM Carlos O'Donell <carlos@redhat.com> wrote:
>>> Pre-commit CI regression caused by this series on both aarch64 and aarch32.
>>> https://patchwork.sourceware.org/project/glibc/patch/20230617222218.642125-6-bugaevc@gmail.com/
>>>
>>> # Running glibc:io ...
>>> # FAIL: io/check-installed-headers-c
>>> # FAIL: io/check-installed-headers-cxx
>>> #
>>> # Running glibc:misc ...
>>> # FAIL: misc/check-installed-headers-c
>>> # FAIL: misc/check-installed-headers-cxx
>>> #
>>> # Running glibc:rt ...
>>> # FAIL: rt/check-installed-headers-c
>>> # FAIL: rt/check-installed-headers-cxx
>>
>> Thank you :(
>>
>> I'm a little bit lost in the Jenkins web UI -- is there anywhere I can
>> view the output of the failing tests to understand what exactly went
>> wrong? I see the sum files here [0], but not the .out files.
>>
>> [0]: https://ci.linaro.org/job/tcwg_glibc_check--master-arm-build/386/artifact/artifacts/artifacts.precommit/sumfiles/
> 
> I could not reproduce it locally, so I think it is a limitation of our 
> (Linaro) CI framework, where not all tests artifacts are being removed 
> with make test-clean. Maxim already sent a fix for this [1].
> 
> [1] https://patchwork.sourceware.org/project/glibc/patch/20230615152547.2924770-1-maxim.kuvyrkov@linaro.org/

Hi Surgey,

Now that we have proper tests (30379efad117b and a3090c2c98facb) for 
check-installed-headers with fortify usage, it easy to trigger the regressions.
I think it should be easy to fix and I hope we get this fortify extension
on next development cycle.

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH v3 0/5] fcntl fortification
  2023-07-21 13:59       ` Adhemerval Zanella Netto
@ 2023-07-21 15:50         ` Sergey Bugaev
  0 siblings, 0 replies; 22+ messages in thread
From: Sergey Bugaev @ 2023-07-21 15:50 UTC (permalink / raw)
  To: Adhemerval Zanella Netto
  Cc: Carlos O'Donell, Maxim Kuvyrkov, libc-alpha, Frederic Berat

On Fri, Jul 21, 2023 at 4:59 PM Adhemerval Zanella Netto
<adhemerval.zanella@linaro.org> wrote:
> Hi Surgey,

Hi,

> Now that we have proper tests (30379efad117b and a3090c2c98facb) for
> check-installed-headers with fortify usage, it easy to trigger the regressions.
> I think it should be easy to fix and I hope we get this fortify extension
> on next development cycle.

That's great! Yes, the regression should be very easy to fix (need to
add an ifdef check around F_DUPFD_CLOEXEC). I've been putting it off
because I saw (some of?) Frederic's work landing, so I'll have to
rebase and see if there's any new breakage; and also because of the
upcoming release (this is clearly not 2.38 material at this point).
I'll look into rebasing & retesting this weekend then.

Sergey (not Surgey :D)

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2023-07-21 15:50 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-17 22:22 [PATCH v3 0/5] fcntl fortification Sergey Bugaev
2023-06-17 22:22 ` [PATCH v3 1/5] support: Add support_fcntl_support_ofd_locks () Sergey Bugaev
2023-06-17 22:22 ` [PATCH v3 2/5] cdefs.h: Define __glibc_warn_system_headers_{begin,end} Sergey Bugaev
2023-06-17 22:22 ` [PATCH v3 3/5] cdefs.h: Enable __errordecl & __warnattr for Clang 14+ Sergey Bugaev
2023-06-17 22:22 ` [PATCH v3 4/5] io: Add FORTIFY_SOURCE check for fcntl arguments Sergey Bugaev
2023-06-17 22:22 ` [PATCH v3 5/5] io: Also verify 2-arg fctnl calls at runtime Sergey Bugaev
2023-06-19 12:58 ` [PATCH v3 0/5] fcntl fortification Carlos O'Donell
2023-06-19 14:23   ` Sergey Bugaev
2023-06-19 18:36     ` Adhemerval Zanella Netto
2023-06-20  5:59       ` Maxim Kuvyrkov
2023-06-20  7:33         ` Sergey Bugaev
2023-06-20  9:41           ` Maxim Kuvyrkov
2023-06-20 11:28             ` Sergey Bugaev
2023-06-20 12:38               ` Maxim Kuvyrkov
2023-06-20 12:53                 ` Sergey Bugaev
2023-06-20 13:40                   ` Adhemerval Zanella Netto
2023-06-20 13:47                     ` Zack Weinberg
2023-06-20 13:46                   ` Zack Weinberg
2023-06-20 12:50         ` Adhemerval Zanella Netto
2023-06-20 14:21           ` Frederic Berat
2023-07-21 13:59       ` Adhemerval Zanella Netto
2023-07-21 15:50         ` Sergey Bugaev

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).