From: DJ Delorie <dj@redhat.com>
To: Martin Sebor <msebor@gmail.com>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH] improve out-of-bounds checking with GCC 10 attribute access [BZ #25219]
Date: Thu, 30 Apr 2020 22:42:13 -0400 [thread overview]
Message-ID: <xn4kt08jga.fsf@greed.delorie.com> (raw)
In-Reply-To: <1b67cd2a-3254-fbd4-4345-7bc282f7cdd0@gmail.com> (message from Martin Sebor via Libc-alpha on Thu, 30 Apr 2020 16:12:32 -0600)
Mostly ok; a few buggy ones and some questions...
Martin Sebor via Libc-alpha <libc-alpha@sourceware.org> writes:
> extern char *__fgets_chk (char *__restrict __s, size_t __size, int __n,
> - FILE *__restrict __stream) __wur;
> + FILE *__restrict __stream)
> + __wur __attr_access ((__read_only__, 1, 2));
__s[__size]
Does __read_only__ mean that fgets only reads from __s? Because that's backwards.
> #ifdef __USE_GNU
> extern char *__fgets_unlocked_chk (char *__restrict __s, size_t __size,
> - int __n, FILE *__restrict __stream) __wur;
> + int __n, FILE *__restrict __stream)
> + __wur __attr_access ((__write_only__, 1, 2));
__s[__size] ok.
> +#if __GNUC_PREREQ (10, 0)
> +/* Denotes a function pointer argument ref-index that can be used
> + to access size-index elements of the pointed-to array according
> + to access mode:
> + access (access-mode, <ref-index> [, <size-index>]) */
IMHO comment should state that the first argument is index 1.
IMHO should document what happens when size-index is missing.
> +#define __attr_access(x) __attribute__ ((__access__ x))
> +#else
> +# define __attr_access(x)
> +#endif
> +
Ok.
> extern ssize_t __read_chk (int __fd, void *__buf, size_t __nbytes,
> - size_t __buflen) __wur;
> + size_t __buflen)
> + __wur __attr_access ((__write_only__, 2, 3));
read_chk() writes to buf
> extern ssize_t __REDIRECT (__read_alias, (int __fd, void *__buf,
> - size_t __nbytes), read) __wur;
> + size_t __nbytes), read)
> + __wur __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t __pread_chk (int __fd, void *__buf, size_t __nbytes,
> - __off_t __offset, size_t __bufsize) __wur;
> + __off_t __offset, size_t __bufsize)
> + __wur __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t __pread64_chk (int __fd, void *__buf, size_t __nbytes,
> - __off64_t __offset, size_t __bufsize) __wur;
> + __off64_t __offset, size_t __bufsize)
> + __wur __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t __REDIRECT (__pread_alias,
> (int __fd, void *__buf, size_t __nbytes,
> - __off_t __offset), pread) __wur;
> + __off_t __offset), pread)
> + __wur __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t __REDIRECT (__pread64_alias,
> (int __fd, void *__buf, size_t __nbytes,
> - __off64_t __offset), pread64) __wur;
> + __off64_t __offset), pread64)
> + __wur __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t __readlink_chk (const char *__restrict __path,
> char *__restrict __buf, size_t __len,
> size_t __buflen)
> - __THROW __nonnull ((1, 2)) __wur;
> + __THROW __nonnull ((1, 2)) __wur __attr_access ((__write_only__, 2, 3));
__buf[__len] ok
> extern ssize_t __REDIRECT_NTH (__readlink_alias,
> (const char *__restrict __path,
> char *__restrict __buf, size_t __len), readlink)
> - __nonnull ((1, 2)) __wur;
> + __nonnull ((1, 2)) __wur __attr_access ((__write_only__, 2, 3));
__buf[__len] ok
> extern ssize_t __readlinkat_chk (int __fd, const char *__restrict __path,
> char *__restrict __buf, size_t __len,
> size_t __buflen)
> - __THROW __nonnull ((2, 3)) __wur;
> + __THROW __nonnull ((2, 3)) __wur __attr_access ((__write_only__, 3, 4));
__buf[__len] ok
> extern ssize_t __REDIRECT_NTH (__readlinkat_alias,
> (int __fd, const char *__restrict __path,
> char *__restrict __buf, size_t __len),
> readlinkat)
> - __nonnull ((2, 3)) __wur;
> + __nonnull ((2, 3)) __wur __attr_access ((__write_only__, 3, 4));
__buf[__len] ok
> extern char *__getcwd_chk (char *__buf, size_t __size, size_t __buflen)
> - __THROW __wur;
> + __THROW __wur __attr_access ((__write_only__, 1, 2));
__buf[__size] ok
> extern char *__REDIRECT_NTH (__getcwd_alias,
> - (char *__buf, size_t __size), getcwd) __wur;
> + (char *__buf, size_t __size), getcwd)
> + __wur __attr_access ((__write_only__, 1, 2));
__buf[__size] ok
> extern char *__getwd_chk (char *__buf, size_t buflen)
> - __THROW __nonnull ((1)) __wur;
> + __THROW __nonnull ((1)) __wur __attr_access ((__write_only__, 1, 2));
__buf[__size] ok
> extern size_t __confstr_chk (int __name, char *__buf, size_t __len,
> - size_t __buflen) __THROW;
> + size_t __buflen) __THROW
> + __attr_access ((__write_only__, 2, 3));
__buf[__len] ok
> extern size_t __REDIRECT_NTH (__confstr_alias, (int __name, char *__buf,
> - size_t __len), confstr);
> + size_t __len), confstr)
> + __attr_access ((__write_only__, 2, 3));
__buf[__len] ok
> extern int __getgroups_chk (int __size, __gid_t __list[], size_t __listlen)
> - __THROW __wur;
> + __THROW __wur __attr_access ((__write_only__, 2, 1));
__list[__size] ok
> extern int __REDIRECT_NTH (__getgroups_alias, (int __size, __gid_t __list[]),
> - getgroups) __wur;
> + getgroups) __wur __attr_access ((__write_only__, 2, 1));
__list[__size] ok
> extern int __ttyname_r_chk (int __fd, char *__buf, size_t __buflen,
> - size_t __nreal) __THROW __nonnull ((2));
> + size_t __nreal) __THROW __nonnull ((2))
> + __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> extern int __getlogin_r_chk (char *__buf, size_t __buflen, size_t __nreal)
> - __nonnull ((1));
> + __nonnull ((1)) __attr_access ((__write_only__, 1, 2));
__buf[__buflen] ok
> extern int __gethostname_chk (char *__buf, size_t __buflen, size_t __nreal)
> - __THROW __nonnull ((1));
> + __THROW __nonnull ((1)) __attr_access ((__write_only__, 1, 2));
__buf[__buflen] ok
> extern int __REDIRECT_NTH (__gethostname_alias, (char *__buf, size_t __buflen),
> - gethostname) __nonnull ((1));
> + gethostname)
> + __nonnull ((1)) __attr_access ((__write_only__, 1, 2));
__buf[__buflen] ok
> extern int __getdomainname_chk (char *__buf, size_t __buflen, size_t __nreal)
> - __THROW __nonnull ((1)) __wur;
> + __THROW __nonnull ((1)) __wur __attr_access ((__write_only__, 1, 2));
__buf[__buflen] ok
> extern int __REDIRECT_NTH (__getdomainname_alias, (char *__buf,
> size_t __buflen),
> - getdomainname) __nonnull ((1)) __wur;
> + getdomainname) __nonnull ((1))
> + __wur __attr_access ((__write_only__, 1, 2));
__buf[__buflen] ok
> -extern ssize_t read (int __fd, void *__buf, size_t __nbytes) __wur;
> +extern ssize_t read (int __fd, void *__buf, size_t __nbytes) __wur
> + __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> -extern ssize_t write (int __fd, const void *__buf, size_t __n) __wur;
> +extern ssize_t write (int __fd, const void *__buf, size_t __n) __wur
> + __attr_access ((__read_only__, 2, 3));
__buf[__n] ok.
> extern ssize_t pread (int __fd, void *__buf, size_t __nbytes,
> - __off_t __offset) __wur;
> + __off_t __offset) __wur
> + __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t pwrite (int __fd, const void *__buf, size_t __n,
> - __off_t __offset) __wur;
> + __off_t __offset) __wur
> + __attr_access ((__read_only__, 2, 3));
__buf[__n] ok
> extern ssize_t __REDIRECT (pread, (int __fd, void *__buf, size_t __nbytes,
> __off64_t __offset),
> - pread64) __wur;
> + pread64) __wur
> + __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t __REDIRECT (pwrite, (int __fd, const void *__buf,
> size_t __nbytes, __off64_t __offset),
> - pwrite64) __wur;
> + pwrite64) __wur
> + __attr_access ((__read_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t pread64 (int __fd, void *__buf, size_t __nbytes,
> - __off64_t __offset) __wur;
> + __off64_t __offset) __wur
> + __attr_access ((__write_only__, 2, 3));
__buf[__nbytes] ok
> extern ssize_t pwrite64 (int __fd, const void *__buf, size_t __n,
> - __off64_t __offset) __wur;
> + __off64_t __offset) __wur
> + __attr_access ((__read_only__, 2, 3));
__buf[__n] ok
> extern char *getwd (char *__buf)
> - __THROW __nonnull ((1)) __attribute_deprecated__ __wur;
> + __THROW __nonnull ((1)) __attribute_deprecated__ __wur
> + __attr_access ((__write_only__, 1));
__buf[???]
> -extern size_t confstr (int __name, char *__buf, size_t __len) __THROW;
> +extern size_t confstr (int __name, char *__buf, size_t __len) __THROW
> + __attribute__ ((access (__write_only__, 2, 3)));
__buf[__len] ok
NOTE: does not use the __attr_access macro
> #endif
>
>
> -extern int getgroups (int __size, __gid_t __list[]) __THROW __wur;
> -
> +extern int getgroups (int __size, __gid_t __list[]) __THROW __wur
> + __attr_access ((__write_only__, 2, 1));
__list[__size] ok
> extern int ttyname_r (int __fd, char *__buf, size_t __buflen)
> - __THROW __nonnull ((2)) __wur;
> + __THROW __nonnull ((2)) __wur __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> extern ssize_t readlink (const char *__restrict __path,
> char *__restrict __buf, size_t __len)
> - __THROW __nonnull ((1, 2)) __wur;
> + __THROW __nonnull ((1, 2)) __wur __attr_access ((__write_only__, 2, 3));
__buf[__len] ok
> extern ssize_t readlinkat (int __fd, const char *__restrict __path,
> char *__restrict __buf, size_t __len)
> - __THROW __nonnull ((2, 3)) __wur;
> + __THROW __nonnull ((2, 3)) __wur __attr_access ((__read_only__, 3, 4));
__buf[__len] ok
> -extern int getlogin_r (char *__name, size_t __name_len) __nonnull ((1));
> +extern int getlogin_r (char *__name, size_t __name_len) __nonnull ((1))
> + __attr_access ((__write_only__, 1, 2));
__name[__name_len] ok
> -extern int gethostname (char *__name, size_t __len) __THROW __nonnull ((1));
> +extern int gethostname (char *__name, size_t __len) __THROW __nonnull ((1))
> + __attr_access ((__write_only__, 1, 2));
__name[__len] ok
> extern int sethostname (const char *__name, size_t __len)
> - __THROW __nonnull ((1)) __wur;
> + __THROW __nonnull ((1)) __wur __attr_access ((__read_only__, 1, 2));
__name[__len] ok
> extern int getdomainname (char *__name, size_t __len)
> - __THROW __nonnull ((1)) __wur;
> + __THROW __nonnull ((1)) __wur __attr_access ((__write_only__, 1, 2));
__name[__len] ok
> extern int setdomainname (const char *__name, size_t __len)
> - __THROW __nonnull ((1)) __wur;
> -
> + __THROW __nonnull ((1)) __wur __attr_access ((__read_only__, 1, 2));
__name[__len] ok
> extern void swab (const void *__restrict __from, void *__restrict __to,
> - ssize_t __n) __THROW __nonnull ((1, 2));
> + ssize_t __n) __THROW __nonnull ((1, 2))
> + __attr_access ((__read_only__, 1, 3))
> + __attr_access ((__write_only__, 2, 3));
__from[__n], __to[__n] ok
> -int getentropy (void *__buffer, size_t __length) __wur;
> +int getentropy (void *__buffer, size_t __length) __wur
> + __attr_access ((__write_only__, 1, 2));
__buffer[__length] ok
> extern int __ptsname_r_chk (int __fd, char *__buf, size_t __buflen,
> - size_t __nreal) __THROW __nonnull ((2));
> + size_t __nreal) __THROW __nonnull ((2))
> + __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> extern int __REDIRECT_NTH (__ptsname_r_alias, (int __fd, char *__buf,
> size_t __buflen), ptsname_r)
> - __nonnull ((2));
> + __nonnull ((2)) __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> extern size_t __mbstowcs_chk (wchar_t *__restrict __dst,
> const char *__restrict __src,
> - size_t __len, size_t __dstlen) __THROW;
> + size_t __len, size_t __dstlen) __THROW
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__dst[__len], __src[???] ok
> extern size_t __REDIRECT_NTH (__mbstowcs_alias,
> (wchar_t *__restrict __dst,
> const char *__restrict __src,
> - size_t __len), mbstowcs);
> + size_t __len), mbstowcs)
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__dst[__len], __src[???] ok
> extern size_t __wcstombs_chk (char *__restrict __dst,
> const wchar_t *__restrict __src,
> - size_t __len, size_t __dstlen) __THROW;
> + size_t __len, size_t __dstlen) __THROW
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__dst[__len], __src[???] ok
> extern size_t __REDIRECT_NTH (__wcstombs_alias,
> (char *__restrict __dst,
> const wchar_t *__restrict __src,
> - size_t __len), wcstombs);
> + size_t __len), wcstombs)
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__dst[__len], __src[???] ok
> extern size_t mbstowcs (wchar_t *__restrict __pwcs,
> - const char *__restrict __s, size_t __n) __THROW;
> + const char *__restrict __s, size_t __n) __THROW
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__pwcs[__n], __s[???] ok
> extern size_t wcstombs (char *__restrict __s,
> const wchar_t *__restrict __pwcs, size_t __n)
> - __THROW;
> -
> + __THROW
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__s[__n], __pwcs[???] ok
> extern int ptsname_r (int __fd, char *__buf, size_t __buflen)
> - __THROW __nonnull ((2));
> + __THROW __nonnull ((2)) __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> void __explicit_bzero_chk (void *__dest, size_t __len, size_t __destlen)
> - __THROW __nonnull ((1));
> + __THROW __nonnull ((1)) __attr_access ((__write_only__, 1, 2));
__dest[__len] ok
> extern char *__stpncpy_chk (char *__dest, const char *__src, size_t __n,
> - size_t __destlen) __THROW;
> + size_t __destlen) __THROW
> + __attr_access ((__write_only__, 1, 3)) __attr_access ((__read_only__, 2));
__dest[__n], __src[???] ok
> extern void *memccpy (void *__restrict __dest, const void *__restrict __src,
> int __c, size_t __n)
> - __THROW __nonnull ((1, 2));
> + __THROW __nonnull ((1, 2)) __attr_access ((__write_only__, 1, 4));
__dest[__n] - missing __src[__n] ?
> extern "C++" void *memrchr (void *__s, int __c, size_t __n)
> - __THROW __asm ("memrchr") __attribute_pure__ __nonnull ((1));
> + __THROW __asm ("memrchr") __attribute_pure__ __nonnull ((1))
> + __attr_access ((__read_only__, 1, 3));
__s[__n] ok
> extern "C++" const void *memrchr (const void *__s, int __c, size_t __n)
> - __THROW __asm ("memrchr") __attribute_pure__ __nonnull ((1));
> + __THROW __asm ("memrchr") __attribute_pure__ __nonnull ((1))
> + __attr_access ((__read_only__, 1, 3));
__s[__n] ok
> extern size_t strxfrm (char *__restrict __dest,
> const char *__restrict __src, size_t __n)
> - __THROW __nonnull ((2));
> + __THROW __nonnull ((2)) __attr_access ((__write_only__, 1, 3));
__dest[__n] - missing __src[???] ?
> extern size_t strxfrm_l (char *__dest, const char *__src, size_t __n,
> - locale_t __l) __THROW __nonnull ((2, 4));
> + locale_t __l) __THROW __nonnull ((2, 4))
> + __attr_access ((__write_only__, 1, 3));
__dest[__n] - missing __src[???] ?
> extern void *memmem (const void *__haystack, size_t __haystacklen,
> const void *__needle, size_t __needlelen)
> - __THROW __attribute_pure__ __nonnull ((1, 3));
> + __THROW __attribute_pure__ __nonnull ((1, 3))
> + __attr_access ((__read_only__, 1, 2))
> + __attr_access ((__read_only__, 3, 4));
__haystack[__haystacklen], __needle[__needlelen] ok
> extern int __REDIRECT_NTH (strerror_r,
> (int __errnum, char *__buf, size_t __buflen),
> - __xpg_strerror_r) __nonnull ((2));
> + __xpg_strerror_r) __nonnull ((2))
> + __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> extern int __xpg_strerror_r (int __errnum, char *__buf, size_t __buflen)
> - __THROW __nonnull ((2));
> + __THROW __nonnull ((2)) __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> extern char *strerror_r (int __errnum, char *__buf, size_t __buflen)
> - __THROW __nonnull ((2)) __wur;
> + __THROW __nonnull ((2)) __wur __attr_access ((__write_only__, 2, 3));
__buf[__buflen] ok
> /* Set N bytes of S to 0. The compiler will not delete a call to this
> function, even if S is dead after the call. */
> -extern void explicit_bzero (void *__s, size_t __n) __THROW __nonnull ((1));
> +extern void explicit_bzero (void *__s, size_t __n) __THROW __nonnull ((1))
> + __attr_access ((__write_only__, 1, 2));
__s[__n] ok
> -extern void *memfrob (void *__s, size_t __n) __THROW __nonnull ((1));
> +extern void *memfrob (void *__s, size_t __n) __THROW __nonnull ((1))
> + __attr_access ((__write_only__, 1, 2));
__s[__n] ok
> [3:text/x-csrc Hide Save:test-access-warn.c (13kB)]
>
> /* BZ #25219 - improve out-of-bounds checking with GCC 10 attribute access */
There are ways of adding such tests into glibc, but I'll defer any
review of such efforts until/unless we actually want to add one.
next prev parent reply other threads:[~2020-05-01 2:42 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-30 22:12 Martin Sebor
2020-04-30 22:37 ` Dmitry V. Levin
2020-05-01 2:42 ` DJ Delorie [this message]
2020-05-01 19:54 ` Martin Sebor
2020-05-01 22:02 ` DJ Delorie
2020-05-04 17:34 ` Martin Sebor
2020-05-04 18:40 ` Martin Sebor
2020-05-06 20:44 ` Joseph Myers
2020-05-06 21:08 ` DJ Delorie
2020-05-06 22:09 ` Martin Sebor
2020-05-06 22:27 ` Joseph Myers
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=xn4kt08jga.fsf@greed.delorie.com \
--to=dj@redhat.com \
--cc=libc-alpha@sourceware.org \
--cc=msebor@gmail.com \
/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).