From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [207.211.31.120]) by sourceware.org (Postfix) with ESMTP id B1201396DC3B for ; Fri, 1 May 2020 02:42:18 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org B1201396DC3B Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-135-LHKl9aOMMHaguM1toOERGg-1; Thu, 30 Apr 2020 22:42:16 -0400 X-MC-Unique: LHKl9aOMMHaguM1toOERGg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id A73CB80B70A; Fri, 1 May 2020 02:42:15 +0000 (UTC) Received: from greed.delorie.com (ovpn-112-52.phx2.redhat.com [10.3.112.52]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6553C277C1; Fri, 1 May 2020 02:42:15 +0000 (UTC) Received: from greed.delorie.com.redhat.com (localhost [127.0.0.1]) by greed.delorie.com (8.14.7/8.14.7) with ESMTP id 0412gDCO008968; Thu, 30 Apr 2020 22:42:13 -0400 From: DJ Delorie To: Martin Sebor Cc: libc-alpha@sourceware.org Subject: Re: [PATCH] improve out-of-bounds checking with GCC 10 attribute access [BZ #25219] 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) Date: Thu, 30 Apr 2020 22:42:13 -0400 Message-ID: MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-13.1 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_1, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 May 2020 02:42:21 -0000 Mostly ok; a few buggy ones and some questions... Martin Sebor via Libc-alpha writes: > extern char *__fgets_chk (char *__restrict __s, size_t __size, int __n, > -=09=09=09 FILE *__restrict __stream) __wur; > +=09=09=09 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 bac= kwards. > #ifdef __USE_GNU > extern char *__fgets_unlocked_chk (char *__restrict __s, size_t __size, > -=09=09=09=09 int __n, FILE *__restrict __stream) __wur; > +=09=09=09=09 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, [, ]) */ 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, > -=09=09=09 size_t __buflen) __wur; > +=09=09=09 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, > -=09=09=09=09=09 size_t __nbytes), read) __wur; > +=09=09=09=09=09 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, > -=09=09=09 __off_t __offset, size_t __bufsize) __wur; > +=09=09=09 __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, > -=09=09=09 __off64_t __offset, size_t __bufsize) __wur; > +=09=09=09 __off64_t __offset, size_t __bufsize) > + __wur __attr_access ((__write_only__, 2, 3)); __buf[__nbytes] ok > extern ssize_t __REDIRECT (__pread_alias, > =09=09=09 (int __fd, void *__buf, size_t __nbytes, > -=09=09=09 __off_t __offset), pread) __wur; > +=09=09=09 __off_t __offset), pread) > + __wur __attr_access ((__write_only__, 2, 3)); __buf[__nbytes] ok > extern ssize_t __REDIRECT (__pread64_alias, > =09=09=09 (int __fd, void *__buf, size_t __nbytes, > -=09=09=09 __off64_t __offset), pread64) __wur; > +=09=09=09 __off64_t __offset), pread64) > + __wur __attr_access ((__write_only__, 2, 3)); __buf[__nbytes] ok > extern ssize_t __readlink_chk (const char *__restrict __path, > =09=09=09 char *__restrict __buf, size_t __len, > =09=09=09 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, > =09=09=09 (const char *__restrict __path, > =09=09=09=09char *__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= , > =09=09=09=09 char *__restrict __buf, size_t __len, > =09=09=09=09 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, > =09=09=09 (int __fd, const char *__restrict __path, > =09=09=09=09char *__restrict __buf, size_t __len), > =09=09=09 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, > -=09=09=09 (char *__buf, size_t __size), getcwd) __wur; > +=09=09=09 (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, > -=09=09=09 size_t __buflen) __THROW; > +=09=09=09 size_t __buflen) __THROW > + __attr_access ((__write_only__, 2, 3)); __buf[__len] ok > extern size_t __REDIRECT_NTH (__confstr_alias, (int __name, char *__buf, > -=09=09=09=09=09=09size_t __len), confstr); > +=09=09=09=09=09=09size_t __len), confstr) > + __attr_access ((__write_only__, 2, 3)); __buf[__len] ok > extern int __getgroups_chk (int __size, __gid_t __list[], size_t __listl= en) > - __THROW __wur; > + __THROW __wur __attr_access ((__write_only__, 2, 1)); __list[__size] ok > extern int __REDIRECT_NTH (__getgroups_alias, (int __size, __gid_t __lis= t[]), > -=09=09=09 getgroups) __wur; > +=09=09=09 getgroups) __wur __attr_access ((__write_only__, 2, 1)); __list[__size] ok > extern int __ttyname_r_chk (int __fd, char *__buf, size_t __buflen, > -=09=09=09 size_t __nreal) __THROW __nonnull ((2)); > +=09=09=09 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 __nrea= l) > - __nonnull ((1)); > + __nonnull ((1)) __attr_access ((__write_only__, 1, 2)); __buf[__buflen] ok > extern int __gethostname_chk (char *__buf, size_t __buflen, size_t __nre= al) > - __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 __b= uflen), > -=09=09=09 gethostname) __nonnull ((1)); > +=09=09=09 gethostname) > + __nonnull ((1)) __attr_access ((__write_only__, 1, 2)); __buf[__buflen] ok > extern int __getdomainname_chk (char *__buf, size_t __buflen, size_t __n= real) > - __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, > =09=09=09=09=09=09 size_t __buflen), > -=09=09=09 getdomainname) __nonnull ((1)) __wur; > +=09=09=09 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, > -=09=09 __off_t __offset) __wur; > +=09=09 __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, > -=09=09 __off_t __offset) __wur; > +=09=09 __off_t __offset) __wur > + __attr_access ((__read_only__, 2, 3)); __buf[__n] ok > extern ssize_t __REDIRECT (pread, (int __fd, void *__buf, size_t __nbyte= s, > =09=09=09=09 __off64_t __offset), > -=09=09=09 pread64) __wur; > +=09=09=09 pread64) __wur > + __attr_access ((__write_only__, 2, 3)); __buf[__nbytes] ok > extern ssize_t __REDIRECT (pwrite, (int __fd, const void *__buf, > =09=09=09=09 size_t __nbytes, __off64_t __offset), > -=09=09=09 pwrite64) __wur; > +=09=09=09 pwrite64) __wur > + __attr_access ((__read_only__, 2, 3)); __buf[__nbytes] ok > extern ssize_t pread64 (int __fd, void *__buf, size_t __nbytes, > -=09=09=09__off64_t __offset) __wur; > +=09=09=09__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, > -=09=09=09 __off64_t __offset) __wur; > +=09=09=09 __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 > =20 > =20 > -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, > =09=09=09 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, > =09=09=09 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, > -=09=09 ssize_t __n) __THROW __nonnull ((1, 2)); > +=09=09 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, > -=09=09=09 size_t __nreal) __THROW __nonnull ((2)); > +=09=09=09 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, > =09=09=09=09=09 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, > =09=09=09 const char *__restrict __src, > -=09=09=09 size_t __len, size_t __dstlen) __THROW; > +=09=09=09 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, > =09=09=09 (wchar_t *__restrict __dst, > =09=09=09 const char *__restrict __src, > -=09=09=09 size_t __len), mbstowcs); > +=09=09=09 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, > =09=09=09 const wchar_t *__restrict __src, > -=09=09=09 size_t __len, size_t __dstlen) __THROW; > +=09=09=09 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, > =09=09=09 (char *__restrict __dst, > =09=09=09 const wchar_t *__restrict __src, > -=09=09=09 size_t __len), wcstombs); > +=09=09=09 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, > -=09=09=09const char *__restrict __s, size_t __n) __THROW; > +=09=09=09const 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, > =09=09=09const 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, > -=09=09=09 size_t __destlen) __THROW; > +=09=09=09 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, > =09=09 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, > =09=09 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, > -=09=09=09 locale_t __l) __THROW __nonnull ((2, 4)); > +=09=09=09 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, > =09=09 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, > =09=09=09 (int __errnum, char *__buf, size_t __buflen), > -=09=09=09 __xpg_strerror_r) __nonnull ((2)); > +=09=09=09 __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 acces= s */ 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.