On 4/30/20 8:42 PM, DJ Delorie wrote:
> 
> Mostly ok; a few buggy ones and some questions...

Thanks for the careful review!

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

Yes, that's wrong.  Good catch!  I completely missed stdio when
testing so I also didn't notice I forgot to add the attribute to
fgets() itself.  I've fixed that in the updated patch.

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

I've tweaked the comment a bit.  I hesitate to go into a lot of
detail here and would expect people needing it to read the manual.

> 
>> +#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[???]

When size-index is missing at least one byte of the array must be
accessible (or the pointer must be null).  There's no way to specify
a constant size with the current syntax.  In the future I'd like to
try to teach GCC to get it from the argument itself (for ordinary
arrays as well as for VLAs):

   extern char *getwd (char __buf[PATH_MAX)
     __attr_access ((__write_only__, 1)) ...;

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

Fixed, thanks.

> 
>>   #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

Similar as in getwd, __src must be an array of at least one element
(or null).

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

Same as above (array of 1 w

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

Sounds good.  In the meantime, the attached revision fixes
the problems pointed out above.  I also include the enhanced
test I used.

Martin