[PATCH v2 2/2] debug: Synchronize feature guards in fortified functions [BZ #28746]
Adhemerval Zanella
adhemerval.zanella@linaro.org
Thu Jan 6 21:15:44 GMT 2022
On 05/01/2022 01:45, Siddhesh Poyarekar via Libc-alpha wrote:
> Some functions (e.g. stpcpy, pread64, etc.) had moved to POSIX in the
> main headers as they got incorporated into the standard, but their
> fortified variants remained under __USE_GNU. As a result, these
> functions did not get fortified when _GNU_SOURCE was not defined.
>
> Add test wrappers that check all functions tested in tst-chk0 at all
> levels with _GNU_SOURCE undefined and then use the failures to (1)
> exclude checks for _GNU_SOURCE functions in these tests and (2) Fix
> feature macro guards in the fortified function headers so that they're
> the same as the ones in the main headers.
>
> This fixes BZ #28746.
>
> Signed-off-by: Siddhesh Poyarekar <siddhesh@sourceware.org>
Patch look ok in general, some comments below.
> ---
> debug/Makefile | 17 +++++++-----
> debug/tst-chk-0-cc-nongnu.cc | 3 +++
> debug/tst-chk-0-nongnu.c | 3 +++
> debug/tst-chk-0.c | 49 ++++++++++++++++++++++++++++------
> debug/tst-chk-1-cc-nongnu.cc | 3 +++
> debug/tst-chk-1-nongnu.c | 3 +++
> debug/tst-chk-2-cc-nongnu.cc | 3 +++
> debug/tst-chk-2-nongnu.c | 3 +++
> debug/tst-chk-3-cc-nongnu.cc | 3 +++
> debug/tst-chk-3-nongnu.c | 3 +++
> posix/bits/unistd.h | 2 +-
> string/bits/string_fortified.h | 8 +++---
> support/xsignal.h | 2 ++
> wcsmbs/bits/wchar2.h | 2 +-
> 14 files changed, 85 insertions(+), 19 deletions(-)
> create mode 100644 debug/tst-chk-0-cc-nongnu.cc
> create mode 100644 debug/tst-chk-0-nongnu.c
> create mode 100644 debug/tst-chk-1-cc-nongnu.cc
> create mode 100644 debug/tst-chk-1-nongnu.c
> create mode 100644 debug/tst-chk-2-cc-nongnu.cc
> create mode 100644 debug/tst-chk-2-nongnu.c
> create mode 100644 debug/tst-chk-3-cc-nongnu.cc
> create mode 100644 debug/tst-chk-3-nongnu.c
>
> diff --git a/debug/Makefile b/debug/Makefile
> index 9aa27eb00c..876d10e521 100644
> --- a/debug/Makefile
> +++ b/debug/Makefile
> @@ -116,6 +116,8 @@ tests-chk = $(addprefix tst-chk-, 0 1 2 3)
> tests-chk-cc = $(addsuffix -cc, $(tests-chk))
> tests-chk-lfs = $(addsuffix -lfs, $(tests-chk))
> tests-chk-cc-lfs = $(addsuffix -lfs, $(tests-chk-cc))
> +tests-chk-nongnu = $(addsuffix -nongnu, $(tests-chk))
> +tests-chk-cc-nongnu = $(addsuffix -nongnu, $(tests-chk-cc))
>
> # We know these tests have problems with format strings, this is what
> # we are testing. Disable that warning. They are also testing
> @@ -126,16 +128,17 @@ define disable-warnings
> CFLAGS-$(1).$(2) += -Wno-format -Wno-deprecated-declarations -Wno-error
> endef
>
> -$(foreach t,$(tests-chk) $(tests-chk-lfs), \
> +$(foreach t,$(tests-chk) $(tests-chk-lfs) $(tests-chk-nongnu), \
> $(eval $(call disable-warnings,$(t),c)))
>
> -$(foreach t,$(tests-chk-cc) $(tests-chk-cc-lfs), \
> +$(foreach t,$(tests-chk-cc) $(tests-chk-cc-lfs) $(tests-chk-cc-nongnu), \
> $(eval $(call disable-warnings,$(t),cc)))
>
> define link-cc
> LDLIBS-$(1) = -lstdc++
> endef
> -$(foreach t,$(tests-chk-cc) $(tests-chk-cc-lfs), $(eval $(call link-cc,$(t))))
> +$(foreach t,$(tests-chk-cc) $(tests-chk-cc-lfs) $(tests-chk-cc-nongnu), \
> + $(eval $(call link-cc,$(t))))
>
> # backtrace_symbols only works if we link with -rdynamic. backtrace
> # requires unwind tables on most architectures.
Ok.
> @@ -156,14 +159,15 @@ tests = backtrace-tst tst-longjmp_chk \
> test-strcpy_chk test-stpcpy_chk \
> tst-longjmp_chk2 tst-backtrace2 tst-backtrace3 \
> tst-backtrace4 tst-backtrace5 tst-backtrace6 tst-realpath-chk \
> - $(tests-chk) $(tests-chk-cc) $(tests-chk-lfs) $(tests-chk-cc-lfs)
> + $(tests-chk) $(tests-chk-cc) $(tests-chk-lfs) $(tests-chk-cc-lfs) \
> + $(tests-chk-nongnu) $(tests-chk-cc-nongnu)
>
> ifeq ($(have-ssp),yes)
> tests += tst-ssp-1
> endif
>
> ifeq (,$(CXX))
> -tests-unsupported = $(tests-chk-cc) $(tests-chk-cc-lfs)
> +tests-unsupported = $(tests-chk-cc) $(tests-chk-cc-lfs) $(tests-chk-cc-nongnu)
> endif
>
> extra-libs = libSegFault libpcprofile
> @@ -194,7 +198,8 @@ define chk-gen-locales
> $(objpfx)$(1).out: $(gen-locales)
> endef
> $(foreach t, \
> - $(tests-chk) $(tests-chk-cc) $(tests-chk-lfs) $(tests-chk-cc-lfs), \
> + $(tests-chk) $(tests-chk-cc) $(tests-chk-lfs) $(tests-chk-cc-lfs) \
> + $(tests-chk-nongnu) $(tests-chk-cc-nongnu), \
> $(eval $(call link-cc,$(t))))
> endif
>
Similar to LFS suggestion I think you can simplify it further by using:
define enable-largefile
CFLAGS-$(1).$(2) += -U_GNU_SOURCE -D_LARGEFILE64_SOURCE=1
endef
$(foreach t,$(tests-chk-nongnu), $(eval $(call enable-largefile,$(t),c)))
$(foreach t,$(tests-chk-cc-nongnu), $(eval $(call enable-largefile,$(t),cc)))
And remove _LARGEFILE64_SOURCE and _GNU_SOURCE on each new test.
> diff --git a/debug/tst-chk-0-cc-nongnu.cc b/debug/tst-chk-0-cc-nongnu.cc
> new file mode 100644
> index 0000000000..24b1306e16
> --- /dev/null
> +++ b/debug/tst-chk-0-cc-nongnu.cc
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-0.c"
> diff --git a/debug/tst-chk-0-nongnu.c b/debug/tst-chk-0-nongnu.c
> new file mode 100644
> index 0000000000..24b1306e16
> --- /dev/null
> +++ b/debug/tst-chk-0-nongnu.c
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-0.c"
> diff --git a/debug/tst-chk-0.c b/debug/tst-chk-0.c
> index 9288610fe6..d65a2fe6e1 100644
> --- a/debug/tst-chk-0.c
> +++ b/debug/tst-chk-0.c
> @@ -1,4 +1,5 @@
> /* Copyright (C) 2004-2022 Free Software Foundation, Inc.
> + Copyright The GNU Toolchain Authors.
> This file is part of the GNU C Library.
>
> The GNU C Library is free software; you can redistribute it and/or
> @@ -36,6 +37,17 @@
> #include <sys/socket.h>
> #include <sys/un.h>
>
> +#ifndef _GNU_SOURCE
> +# define MEMPCPY memcpy
> +# define WMEMPCPY wmemcpy
> +# define MEMPCPY_RET(x) 0
> +# define WMEMPCPY_RET(x) 0
> +#else
> +# define MEMPCPY mempcpy
> +# define WMEMPCPY wmempcpy
> +# define MEMPCPY_RET(x) __builtin_strlen (x)
> +# define WMEMPCPY_RET(x) wcslen (x)
> +#endif
>
> #define obstack_chunk_alloc malloc
> #define obstack_chunk_free free
> @@ -162,7 +174,7 @@ do_test (void)
> if (memcmp (buf, "aabcdefghi", 10))
> FAIL ();
>
> - if (mempcpy (buf + 5, "abcde", 5) != buf + 10
> + if (MEMPCPY (buf + 5, "abcde", 5) != buf + 5 + MEMPCPY_RET ("abcde")
> || memcmp (buf, "aabcdabcde", 10))
> FAIL ();
>
> @@ -207,7 +219,7 @@ do_test (void)
> if (memcmp (buf, "aabcdefghi", 10))
> FAIL ();
>
> - if (mempcpy (buf + 5, "abcde", l0 + 5) != buf + 10
> + if (MEMPCPY (buf + 5, "abcde", l0 + 5) != buf + 5 + MEMPCPY_RET ("abcde")
> || memcmp (buf, "aabcdabcde", 10))
> FAIL ();
>
> @@ -266,7 +278,8 @@ do_test (void)
> if (memcmp (a.buf1, "aabcdefghi", 10))
> FAIL ();
>
> - if (mempcpy (a.buf1 + 5, "abcde", l0 + 5) != a.buf1 + 10
> + if (MEMPCPY (a.buf1 + 5, "abcde", l0 + 5)
> + != a.buf1 + 5 + MEMPCPY_RET ("abcde")
> || memcmp (a.buf1, "aabcdabcde", 10))
> FAIL ();
>
> @@ -347,6 +360,7 @@ do_test (void)
> bcopy (buf + 1, buf + 2, l0 + 9);
> CHK_FAIL_END
>
> +#ifdef _GNU_SOURCE
> CHK_FAIL_START
> p = (char *) mempcpy (buf + 6, "abcde", 5);
> CHK_FAIL_END
> @@ -354,6 +368,7 @@ do_test (void)
> CHK_FAIL_START
> p = (char *) mempcpy (buf + 6, "abcde", l0 + 5);
> CHK_FAIL_END
> +#endif
>
> CHK_FAIL_START
> memset (buf + 9, 'j', 2);
> @@ -464,6 +479,7 @@ do_test (void)
> bcopy (a.buf1 + 1, a.buf1 + 2, l0 + 9);
> CHK_FAIL_END
>
> +#ifdef _GNU_SOURCE
> CHK_FAIL_START
> p = (char *) mempcpy (a.buf1 + 6, "abcde", 5);
> CHK_FAIL_END
> @@ -471,6 +487,7 @@ do_test (void)
> CHK_FAIL_START
> p = (char *) mempcpy (a.buf1 + 6, "abcde", l0 + 5);
> CHK_FAIL_END
> +#endif
>
> CHK_FAIL_START
> memset (a.buf1 + 9, 'j', 2);
> @@ -550,7 +567,7 @@ do_test (void)
> if (wmemcmp (wbuf, L"aabcdefghi", 10))
> FAIL ();
>
> - if (wmempcpy (wbuf + 5, L"abcde", 5) != wbuf + 10
> + if (WMEMPCPY (wbuf + 5, L"abcde", 5) != wbuf + 5 + WMEMPCPY_RET (L"abcde")
> || wmemcmp (wbuf, L"aabcdabcde", 10))
> FAIL ();
>
> @@ -583,7 +600,8 @@ do_test (void)
> if (wmemcmp (wbuf, L"aabcdefghi", 10))
> FAIL ();
>
> - if (wmempcpy (wbuf + 5, L"abcde", l0 + 5) != wbuf + 10
> + if (WMEMPCPY (wbuf + 5, L"abcde", l0 + 5)
> + != wbuf + 5 + WMEMPCPY_RET (L"abcde")
> || wmemcmp (wbuf, L"aabcdabcde", 10))
> FAIL ();
>
> @@ -625,7 +643,8 @@ do_test (void)
> if (wmemcmp (wa.buf1, L"aabcdefghi", 10))
> FAIL ();
>
> - if (wmempcpy (wa.buf1 + 5, L"abcde", l0 + 5) != wa.buf1 + 10
> + if (WMEMPCPY (wa.buf1 + 5, L"abcde", l0 + 5)
> + != wa.buf1 + 5 + WMEMPCPY_RET (L"abcde")
> || wmemcmp (wa.buf1, L"aabcdabcde", 10))
> FAIL ();
>
> @@ -694,6 +713,7 @@ do_test (void)
> wmemmove (wbuf + 2, wbuf + 1, l0 + 9);
> CHK_FAIL_END
>
> +#ifdef _GNU_SOURCE
> CHK_FAIL_START
> wp = wmempcpy (wbuf + 6, L"abcde", 5);
> CHK_FAIL_END
> @@ -701,6 +721,7 @@ do_test (void)
> CHK_FAIL_START
> wp = wmempcpy (wbuf + 6, L"abcde", l0 + 5);
> CHK_FAIL_END
> +#endif
>
> CHK_FAIL_START
> wmemset (wbuf + 9, L'j', 2);
> @@ -768,6 +789,7 @@ do_test (void)
> wmemmove (wa.buf1 + 2, wa.buf1 + 1, l0 + 9);
> CHK_FAIL_END
>
> +#ifdef _GNU_SOURCE
> CHK_FAIL_START
> wp = wmempcpy (wa.buf1 + 6, L"abcde", 5);
> CHK_FAIL_END
> @@ -775,6 +797,7 @@ do_test (void)
> CHK_FAIL_START
> wp = wmempcpy (wa.buf1 + 6, L"abcde", l0 + 5);
> CHK_FAIL_END
> +#endif
>
> CHK_FAIL_START
> wmemset (wa.buf1 + 9, L'j', 2);
> @@ -906,6 +929,7 @@ do_test (void)
> if (fprintf (fp, buf2 + 4, str5) != 7)
> FAIL ();
>
> +#ifdef _GNU_SOURCE
> char *my_ptr = NULL;
> strcpy (buf2 + 2, "%n%s%n");
> /* When the format string is writable and contains %n,
> @@ -935,6 +959,7 @@ do_test (void)
> if (obstack_printf (&obs, "%s%n%s%n", str4, &n1, str5, &n1) != 14)
> FAIL ();
> obstack_free (&obs, NULL);
> +#endif
>
> if (freopen (temp_filename, "r", stdin) == NULL)
> {
> @@ -982,6 +1007,7 @@ do_test (void)
>
> rewind (stdin);
>
> +#ifdef _GNU_SOURCE
> if (fgets_unlocked (buf, buf_size, stdin) != buf
> || memcmp (buf, "abcdefgh\n", 10))
> FAIL ();
> @@ -1008,6 +1034,7 @@ do_test (void)
> #endif
>
> rewind (stdin);
> +#endif
>
> if (fread (buf, 1, buf_size, stdin) != buf_size
> || memcmp (buf, "abcdefgh\nA", 10))
> @@ -1578,7 +1605,10 @@ do_test (void)
> ret = 1;
> }
>
> - int fd = posix_openpt (O_RDWR);
> + int fd;
> +
> +#ifdef _GNU_SOURCE
> + fd = posix_openpt (O_RDWR);
> if (fd != -1)
> {
> char enough[1000];
> @@ -1594,6 +1624,7 @@ do_test (void)
> #endif
> close (fd);
> }
> +#endif
>
> #if PATH_MAX > 0
> confstr (_CS_GNU_LIBC_VERSION, largebuf, sizeof (largebuf));
> @@ -1711,8 +1742,9 @@ do_test (void)
> poll (fds, l0 + 2, 0);
> CHK_FAIL_END
> #endif
> +#ifdef _GNU_SOURCE
> ppoll (fds, 1, NULL, NULL);
> -#if __USE_FORTIFY_LEVEL >= 1
> +# if __USE_FORTIFY_LEVEL >= 1
> CHK_FAIL_START
> ppoll (fds, 2, NULL, NULL);
> CHK_FAIL_END
> @@ -1720,6 +1752,7 @@ do_test (void)
> CHK_FAIL_START
> ppoll (fds, l0 + 2, NULL, NULL);
> CHK_FAIL_END
> +# endif
> #endif
>
> return ret;
> diff --git a/debug/tst-chk-1-cc-nongnu.cc b/debug/tst-chk-1-cc-nongnu.cc
> new file mode 100644
> index 0000000000..cce36ff617
> --- /dev/null
> +++ b/debug/tst-chk-1-cc-nongnu.cc
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-1.c"
> diff --git a/debug/tst-chk-1-nongnu.c b/debug/tst-chk-1-nongnu.c
> new file mode 100644
> index 0000000000..cce36ff617
> --- /dev/null
> +++ b/debug/tst-chk-1-nongnu.c
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-1.c"
> diff --git a/debug/tst-chk-2-cc-nongnu.cc b/debug/tst-chk-2-cc-nongnu.cc
> new file mode 100644
> index 0000000000..149af283c4
> --- /dev/null
> +++ b/debug/tst-chk-2-cc-nongnu.cc
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-2.c"
> diff --git a/debug/tst-chk-2-nongnu.c b/debug/tst-chk-2-nongnu.c
> new file mode 100644
> index 0000000000..149af283c4
> --- /dev/null
> +++ b/debug/tst-chk-2-nongnu.c
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-2.c"
> diff --git a/debug/tst-chk-3-cc-nongnu.cc b/debug/tst-chk-3-cc-nongnu.cc
> new file mode 100644
> index 0000000000..a306b3290c
> --- /dev/null
> +++ b/debug/tst-chk-3-cc-nongnu.cc
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-3.c"
> diff --git a/debug/tst-chk-3-nongnu.c b/debug/tst-chk-3-nongnu.c
> new file mode 100644
> index 0000000000..a306b3290c
> --- /dev/null
> +++ b/debug/tst-chk-3-nongnu.c
> @@ -0,0 +1,3 @@
> +#undef _GNU_SOURCE
> +#define _LARGEFILE64_SOURCE 1
> +#include "tst-chk-3.c"
> diff --git a/posix/bits/unistd.h b/posix/bits/unistd.h
> index 01df84b5f9..f53a7a06eb 100644
> --- a/posix/bits/unistd.h
> +++ b/posix/bits/unistd.h
> @@ -40,7 +40,7 @@ read (int __fd, void *__buf, size_t __nbytes)
> __fd, __buf, __nbytes);
> }
>
> -#ifdef __USE_UNIX98
> +#if defined __USE_UNIX98 || defined __USE_XOPEN2K8
> extern ssize_t __pread_chk (int __fd, void *__buf, size_t __nbytes,
> __off_t __offset, size_t __bufsize)
> __wur __attr_access ((__write_only__, 2, 3));
Ok.
> diff --git a/string/bits/string_fortified.h b/string/bits/string_fortified.h
> index 51ad480401..d0188b5ba8 100644
> --- a/string/bits/string_fortified.h
> +++ b/string/bits/string_fortified.h
> @@ -79,7 +79,7 @@ __NTH (strcpy (char *__restrict __dest, const char *__restrict __src))
> return __builtin___strcpy_chk (__dest, __src, __glibc_objsize (__dest));
> }
>
> -#ifdef __USE_GNU
> +#ifdef __USE_XOPEN2K8
> __fortify_function char *
> __NTH (stpcpy (char *__restrict __dest, const char *__restrict __src))
> {
> @@ -96,14 +96,15 @@ __NTH (strncpy (char *__restrict __dest, const char *__restrict __src,
> __glibc_objsize (__dest));
> }
>
Ok.
> -#if __GNUC_PREREQ (4, 7) || __glibc_clang_prereq (2, 6)
> +#if __USE_XOPEN2K8
Should it be #ifdef here?
> +# if __GNUC_PREREQ (4, 7) || __glibc_clang_prereq (2, 6)
> __fortify_function char *
> __NTH (stpncpy (char *__dest, const char *__src, size_t __n))
> {
> return __builtin___stpncpy_chk (__dest, __src, __n,
> __glibc_objsize (__dest));
> }
> -#else
> +# else
> extern char *__stpncpy_chk (char *__dest, const char *__src, size_t __n,
> size_t __destlen) __THROW
> __fortified_attr_access ((__write_only__, 1, 3))
> @@ -119,6 +120,7 @@ __NTH (stpncpy (char *__dest, const char *__src, size_t __n))
> return __stpncpy_chk (__dest, __src, __n, __bos (__dest));
> return __stpncpy_alias (__dest, __src, __n);
> }
> +# endif
> #endif
>
>
Ok.
> diff --git a/support/xsignal.h b/support/xsignal.h
> index 973f532495..d868bb336c 100644
> --- a/support/xsignal.h
> +++ b/support/xsignal.h
> @@ -28,7 +28,9 @@ __BEGIN_DECLS
> terminate the process on error. */
>
> void xraise (int sig);
> +#ifdef _GNU_SOURCE
> sighandler_t xsignal (int sig, sighandler_t handler);
> +#endif
> void xsigaction (int sig, const struct sigaction *newact,
> struct sigaction *oldact);
>
> diff --git a/wcsmbs/bits/wchar2.h b/wcsmbs/bits/wchar2.h
> index 25d06d433f..0e017f458b 100644
> --- a/wcsmbs/bits/wchar2.h
> +++ b/wcsmbs/bits/wchar2.h
> @@ -457,7 +457,7 @@ __NTH (wcsrtombs (char *__restrict __dst, const wchar_t **__restrict __src,
> }
>
>
> -#ifdef __USE_GNU
> +#ifdef __USE_XOPEN2K8
> extern size_t __mbsnrtowcs_chk (wchar_t *__restrict __dst,
> const char **__restrict __src, size_t __nmc,
> size_t __len, mbstate_t *__restrict __ps,
More information about the Libc-alpha
mailing list