From: Carlos O'Donell <carlos@redhat.com>
To: Adhemerval Zanella <adhemerval.zanella@linaro.org>,
libc-alpha@sourceware.org
Cc: Siddhesh Poyarekar <siddhesh@gotplt.org>
Subject: Re: [PATCH v3 02/10] libio: Improve fortify with clang
Date: Tue, 20 Feb 2024 17:06:12 -0500 [thread overview]
Message-ID: <9c3980fd-692f-4a06-95b5-3c5650e29497@redhat.com> (raw)
In-Reply-To: <20240208184622.332678-3-adhemerval.zanella@linaro.org>
On 2/8/24 13:46, Adhemerval Zanella wrote:
> It improve fortify checks for sprintf, vsprintf, vsnsprintf, fprintf,
> dprintf, asprintf, __asprintf, obstack_printf, gets, fgets,
> fgets_unlocked, fread, and fread_unlocked. The runtime checks have
> similar support coverage as with GCC.
LGTM.
Tested on x86_64 and i686 with gcc.
Reviewed-by: Carlos O'Donell <carlos@redhat.com>
Tested-by: Carlos O'Donell <carlos@redhat.com>
> For function with variadic argument (sprintf, snprintf, fprintf, printf,
> dprintf, asprintf, __asprintf, obstack_printf) the fortify wrapper calls
> the va_arg version since clang does not support __va_arg_pack.
>
> Checked on aarch64, armhf, x86_64, and i686.
> ---
> libio/bits/stdio2.h | 173 +++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 153 insertions(+), 20 deletions(-)
>
> diff --git a/libio/bits/stdio2.h b/libio/bits/stdio2.h
> index f9e8d37610..91a80dd7c6 100644
> --- a/libio/bits/stdio2.h
> +++ b/libio/bits/stdio2.h
> @@ -31,15 +31,29 @@ __NTH (sprintf (char *__restrict __s, const char *__restrict __fmt, ...))
> __glibc_objsize (__s), __fmt,
> __va_arg_pack ());
> }
> +#elif __fortify_use_clang
> +/* clang does not have __va_arg_pack, so defer to va_arg version. */
> +__fortify_function_error_function __attribute_overloadable__ int
> +__NTH (sprintf (__fortify_clang_overload_arg (char *, __restrict, __s),
> + const char *__restrict __fmt, ...))
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __builtin___vsprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
> + __glibc_objsize (__s), __fmt,
> + __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> #elif !defined __cplusplus
> # define sprintf(str, ...) \
> __builtin___sprintf_chk (str, __USE_FORTIFY_LEVEL - 1, \
> __glibc_objsize (str), __VA_ARGS__)
> #endif
>
> -__fortify_function int
> -__NTH (vsprintf (char *__restrict __s, const char *__restrict __fmt,
> - __gnuc_va_list __ap))
> +__fortify_function __attribute_overloadable__ int
> +__NTH (vsprintf (__fortify_clang_overload_arg (char *, __restrict, __s),
> + const char *__restrict __fmt, __gnuc_va_list __ap))
> {
> return __builtin___vsprintf_chk (__s, __USE_FORTIFY_LEVEL - 1,
> __glibc_objsize (__s), __fmt, __ap);
> @@ -55,15 +69,33 @@ __NTH (snprintf (char *__restrict __s, size_t __n,
> __glibc_objsize (__s), __fmt,
> __va_arg_pack ());
> }
> +# elif __fortify_use_clang
> +/* clang does not have __va_arg_pack, so defer to va_arg version. */
> +__fortify_function_error_function __attribute_overloadable__ int
> +__NTH (snprintf (__fortify_clang_overload_arg (char *, __restrict, __s),
> + size_t __n, const char *__restrict __fmt, ...))
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __builtin___vsnprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
> + __glibc_objsize (__s), __fmt,
> + __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> # elif !defined __cplusplus
> # define snprintf(str, len, ...) \
> __builtin___snprintf_chk (str, len, __USE_FORTIFY_LEVEL - 1, \
> __glibc_objsize (str), __VA_ARGS__)
> # endif
>
> -__fortify_function int
> -__NTH (vsnprintf (char *__restrict __s, size_t __n,
> - const char *__restrict __fmt, __gnuc_va_list __ap))
> +__fortify_function __attribute_overloadable__ int
> +__NTH (vsnprintf (__fortify_clang_overload_arg (char *, __restrict, __s),
> + size_t __n, const char *__restrict __fmt,
> + __gnuc_va_list __ap))
> + __fortify_clang_warning (__fortify_clang_bos_static_lt (__n, __s),
> + "call to vsnprintf may overflow the destination "
> + "buffer")
OK.
> {
> return __builtin___vsnprintf_chk (__s, __n, __USE_FORTIFY_LEVEL - 1,
> __glibc_objsize (__s), __fmt, __ap);
> @@ -85,6 +117,30 @@ printf (const char *__restrict __fmt, ...)
> {
> return __printf_chk (__USE_FORTIFY_LEVEL - 1, __fmt, __va_arg_pack ());
> }
> +# elif __fortify_use_clang
> +/* clang does not have __va_arg_pack, so defer to va_arg version. */
> +__fortify_function_error_function __attribute_overloadable__ __nonnull ((1)) int
> +fprintf (__fortify_clang_overload_arg (FILE *, __restrict, __stream),
> + const char *__restrict __fmt, ...)
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __builtin___vfprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1,
> + __fmt, __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> +
> +__fortify_function_error_function __attribute_overloadable__ int
> +printf (__fortify_clang_overload_arg (const char *, __restrict, __fmt), ...)
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __builtin___vprintf_chk (__USE_FORTIFY_LEVEL - 1, __fmt,
> + __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> # elif !defined __cplusplus
> # define printf(...) \
> __printf_chk (__USE_FORTIFY_LEVEL - 1, __VA_ARGS__)
> @@ -92,8 +148,9 @@ printf (const char *__restrict __fmt, ...)
> __fprintf_chk (stream, __USE_FORTIFY_LEVEL - 1, __VA_ARGS__)
> # endif
>
> -__fortify_function int
> -vprintf (const char *__restrict __fmt, __gnuc_va_list __ap)
> +__fortify_function __attribute_overloadable__ int
> +vprintf (__fortify_clang_overload_arg (const char *, __restrict, __fmt),
> + __gnuc_va_list __ap)
> {
> #ifdef __USE_EXTERN_INLINES
> return __vfprintf_chk (stdout, __USE_FORTIFY_LEVEL - 1, __fmt, __ap);
> @@ -117,6 +174,18 @@ dprintf (int __fd, const char *__restrict __fmt, ...)
> return __dprintf_chk (__fd, __USE_FORTIFY_LEVEL - 1, __fmt,
> __va_arg_pack ());
> }
> +# elif __fortify_use_clang
> +__fortify_function_error_function __attribute_overloadable__ int
> +dprintf (int __fd, __fortify_clang_overload_arg (const char *, __restrict,
> + __fmt), ...)
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __vdprintf_chk (__fd, __USE_FORTIFY_LEVEL - 1, __fmt,
> + __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> # elif !defined __cplusplus
> # define dprintf(fd, ...) \
> __dprintf_chk (fd, __USE_FORTIFY_LEVEL - 1, __VA_ARGS__)
> @@ -153,6 +222,43 @@ __NTH (obstack_printf (struct obstack *__restrict __obstack,
> return __obstack_printf_chk (__obstack, __USE_FORTIFY_LEVEL - 1, __fmt,
> __va_arg_pack ());
> }
> +# elif __fortify_use_clang
> +__fortify_function_error_function __attribute_overloadable__ int
> +__NTH (asprintf (__fortify_clang_overload_arg (char **, __restrict, __ptr),
> + const char *__restrict __fmt, ...))
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __vasprintf_chk (__ptr, __USE_FORTIFY_LEVEL - 1, __fmt,
> + __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> +
> +__fortify_function_error_function __attribute_overloadable__ int
> +__NTH (__asprintf (__fortify_clang_overload_arg (char **, __restrict, __ptr),
> + const char *__restrict __fmt, ...))
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __vasprintf_chk (__ptr, __USE_FORTIFY_LEVEL - 1, __fmt,
> + __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> +
> +__fortify_function_error_function __attribute_overloadable__ int
> +__NTH (obstack_printf (__fortify_clang_overload_arg (struct obstack *,
> + __restrict, __obstack),
> + const char *__restrict __fmt, ...))
> +{
> + __gnuc_va_list __fortify_ap;
> + __builtin_va_start (__fortify_ap, __fmt);
> + int __r = __obstack_vprintf_chk (__obstack, __USE_FORTIFY_LEVEL - 1,
> + __fmt, __fortify_ap);
> + __builtin_va_end (__fortify_ap);
> + return __r;
> +}
> # elif !defined __cplusplus
> # define asprintf(ptr, ...) \
> __asprintf_chk (ptr, __USE_FORTIFY_LEVEL - 1, __VA_ARGS__)
> @@ -182,8 +288,11 @@ __NTH (obstack_vprintf (struct obstack *__restrict __obstack,
> #endif
>
> #if __GLIBC_USE (DEPRECATED_GETS)
> -__fortify_function __wur char *
> -gets (char *__str)
> +__fortify_function __wur __attribute_overloadable__ char *
> +gets (__fortify_clang_overload_arg (char *, , __str))
> + __fortify_clang_warning (__glibc_objsize (__str) == (size_t) -1,
> + "please use fgets or getline instead, gets "
> + "can not specify buffer size")
> {
> if (__glibc_objsize (__str) != (size_t) -1)
> return __gets_chk (__str, __glibc_objsize (__str));
> @@ -192,48 +301,70 @@ gets (char *__str)
> #endif
>
> __fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
> -__nonnull ((3)) char *
> -fgets (char *__restrict __s, int __n, FILE *__restrict __stream)
> +__nonnull ((3)) __attribute_overloadable__ char *
> +fgets (__fortify_clang_overload_arg (char *, __restrict, __s), int __n,
> + FILE *__restrict __stream)
> + __fortify_clang_warning (__fortify_clang_bos_static_lt (__n, __s) && __n > 0,
> + "fgets called with bigger size than length of "
> + "destination buffer")
> {
> size_t sz = __glibc_objsize (__s);
> if (__glibc_safe_or_unknown_len (__n, sizeof (char), sz))
> return __fgets_alias (__s, __n, __stream);
> +#if !__fortify_use_clang
> if (__glibc_unsafe_len (__n, sizeof (char), sz))
> return __fgets_chk_warn (__s, sz, __n, __stream);
> +#endif
> return __fgets_chk (__s, sz, __n, __stream);
> }
>
> -__fortify_function __wur __nonnull ((4)) size_t
> -fread (void *__restrict __ptr, size_t __size, size_t __n,
> - FILE *__restrict __stream)
> +__fortify_function __wur __nonnull ((4)) __attribute_overloadable__ size_t
> +fread (__fortify_clang_overload_arg (void *, __restrict, __ptr),
> + size_t __size, size_t __n, FILE *__restrict __stream)
> + __fortify_clang_warning (__fortify_clang_bos0_static_lt (__size * __n, __ptr)
> + && !__fortify_clang_mul_may_overflow (__size, __n),
> + "fread called with bigger size * n than length "
> + "of destination buffer")
> {
> size_t sz = __glibc_objsize0 (__ptr);
> if (__glibc_safe_or_unknown_len (__n, __size, sz))
> return __fread_alias (__ptr, __size, __n, __stream);
> +#if !__fortify_use_clang
> if (__glibc_unsafe_len (__n, __size, sz))
> return __fread_chk_warn (__ptr, sz, __size, __n, __stream);
> +#endif
> return __fread_chk (__ptr, sz, __size, __n, __stream);
> }
>
> #ifdef __USE_GNU
> __fortify_function __wur __fortified_attr_access (__write_only__, 1, 2)
> -__nonnull ((3)) char *
> -fgets_unlocked (char *__restrict __s, int __n, FILE *__restrict __stream)
> +__nonnull ((3)) __attribute_overloadable__ char *
> +fgets_unlocked (__fortify_clang_overload_arg (char *, __restrict, __s),
> + int __n, FILE *__restrict __stream)
> + __fortify_clang_warning (__fortify_clang_bos_static_lt (__n, __s) && __n > 0,
> + "fgets called with bigger size than length of "
> + "destination buffer")
> {
> size_t sz = __glibc_objsize (__s);
> if (__glibc_safe_or_unknown_len (__n, sizeof (char), sz))
> return __fgets_unlocked_alias (__s, __n, __stream);
> +#if !__fortify_use_clang
> if (__glibc_unsafe_len (__n, sizeof (char), sz))
> return __fgets_unlocked_chk_warn (__s, sz, __n, __stream);
> +#endif
OK. Interesting difference here.
> return __fgets_unlocked_chk (__s, sz, __n, __stream);
> }
> #endif
>
> #ifdef __USE_MISC
> # undef fread_unlocked
> -__fortify_function __wur __nonnull ((4)) size_t
> -fread_unlocked (void *__restrict __ptr, size_t __size, size_t __n,
> - FILE *__restrict __stream)
> +__fortify_function __wur __nonnull ((4)) __attribute_overloadable__ size_t
> +fread_unlocked (__fortify_clang_overload_arg0 (void *, __restrict, __ptr),
> + size_t __size, size_t __n, FILE *__restrict __stream)
> + __fortify_clang_warning (__fortify_clang_bos0_static_lt (__size * __n, __ptr)
> + && !__fortify_clang_mul_may_overflow (__size, __n),
> + "fread_unlocked called with bigger size * n than "
> + "length of destination buffer")
> {
> size_t sz = __glibc_objsize0 (__ptr);
> if (__glibc_safe_or_unknown_len (__n, __size, sz))
> @@ -261,8 +392,10 @@ fread_unlocked (void *__restrict __ptr, size_t __size, size_t __n,
> # endif
> return __fread_unlocked_alias (__ptr, __size, __n, __stream);
> }
> +# if !__fortify_use_clang
> if (__glibc_unsafe_len (__n, __size, sz))
> return __fread_unlocked_chk_warn (__ptr, sz, __size, __n, __stream);
> +# endif
OK.
> return __fread_unlocked_chk (__ptr, sz, __size, __n, __stream);
>
> }
--
Cheers,
Carlos.
next prev parent reply other threads:[~2024-02-20 22:06 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-08 18:46 [PATCH v3 00/10] Improve fortify support " Adhemerval Zanella
2024-02-08 18:46 ` [PATCH v3 01/10] cdefs.h: Add clang fortify directives Adhemerval Zanella
2024-02-20 19:48 ` Siddhesh Poyarekar
2024-02-21 5:48 ` Sam James
2024-02-22 18:21 ` Adhemerval Zanella Netto
2024-02-22 19:41 ` Siddhesh Poyarekar
2024-02-20 22:05 ` Carlos O'Donell
2024-02-20 22:45 ` Joseph Myers
2024-02-22 18:39 ` Adhemerval Zanella Netto
2024-02-08 18:46 ` [PATCH v3 02/10] libio: Improve fortify with clang Adhemerval Zanella
2024-02-20 22:06 ` Carlos O'Donell [this message]
2024-02-22 18:41 ` Adhemerval Zanella Netto
2024-02-08 18:46 ` [PATCH v3 03/10] string: " Adhemerval Zanella
2024-02-20 22:06 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 04/10] stdlib: " Adhemerval Zanella
2024-02-20 22:05 ` Carlos O'Donell
2024-02-22 18:45 ` Adhemerval Zanella Netto
2024-02-22 19:24 ` Adhemerval Zanella Netto
2024-02-26 14:07 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 05/10] unistd: " Adhemerval Zanella
2024-02-20 22:06 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 06/10] socket: " Adhemerval Zanella
2024-02-21 13:20 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 07/10] syslog: " Adhemerval Zanella
2024-02-20 22:05 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 08/10] wcsmbs: " Adhemerval Zanella
2024-02-20 22:05 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 09/10] debug: Improve fcntl.h fortify warnings " Adhemerval Zanella
2024-02-20 22:05 ` Carlos O'Donell
2024-02-08 18:46 ` [PATCH v3 10/10] debug: Improve mqueue.h " Adhemerval Zanella
2024-02-20 22:05 ` Carlos O'Donell
2024-02-20 13:17 ` [PATCH v3 00/10] Improve fortify support " Adhemerval Zanella Netto
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9c3980fd-692f-4a06-95b5-3c5650e29497@redhat.com \
--to=carlos@redhat.com \
--cc=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=siddhesh@gotplt.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).