Re: [PATCH] more out of bounds checking improvements

[Martin Sebor <msebor@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2383 bytes --]

On 10/26/20 10:08 AM, Joseph Myers wrote:
> On Mon, 26 Oct 2020, Martin Sebor via Libc-alpha wrote:
> 
>> The patch introduces the _L_tmpnam macro to avoid polluting
>> the POSIX <unistd.h> namespace with L_tmpnam when the latter is
>> only supposed to be defined in <stdio.h>.  This in turn causes
>> the a number of POSIX conformance test failures that I haven't
>> been able to figure how to deal with and need some help with.
>>
>> In file included from ../include/unistd.h:2,
>>                   from /tmp/tmpzm39v4n3/test.c:1:
>> ../posix/unistd.h:1159:32: error: ‘_L_ctermid’ undeclared here (not in a
>> function)
>>   extern char *ctermid (char __s[_L_ctermid]) __THROW
>>                                  ^~~~~~~~~~
>>
>> I expected adding the new macros to stdio-common/stdio_lim.h.in
>> would do the trick but clearly something else is needed and I'm
>> at a lost as to what that might be.  I haven't been able to find
> 
> <unistd.h> doesn't include <bits/stdio_lim.h>, and you're making
> <unistd.h> use _L_ctermid, and you're only defining _L_ctermid in
> <bits/stdio_lim.h>.  You need to define it in a header that <unistd.h>
> includes - which also needs to be one whose contents are namespace-clean
> for inclusion in <unistd.h> (which <bits/stdio_lim.h> isn't).
> 
> The obvious way would be to have a new installed (i.e. add to "headers" in
> the relevant Makefile) header for the new macros that can be included in
> both <stdio.h> and <unistd.h>.  Suggestion: the existing scheme for
> automatic generation of bits/stdio_lim.h is overly complicated, it would
> be better to use sysdeps headers in the normal way like for other bits/
> headers where the values may depend on the glibc configuration (and then
> to have testcases that verify consistently of OPEN_MAX and FOPEN_MAX / of
> PATH_MAX and FILENAME_MAX, when both are defined).

I don't know enough about the Glibc build infrastructure to
understand your suggestion but either approach sounds more involved
than I have cycles for so I propose the scaled patch instead, without
the ctermid and cuserid changes (and without the nonnull attribute
on readv/writev(*)).  Hopefully someone with more experience with
the existing scheme will find a way to define the two macros and
make use of them to enable the detection for these two functions
as well.

Martin

[*] I'll submit that patch separately.

[-- Attachment #2: glibc-attr-access.diff --]
[-- Type: text/x-patch, Size: 17104 bytes --]

diff --git a/inet/if_index.c b/inet/if_index.c
index ddef419230..36f1806ac1 100644
--- a/inet/if_index.c
+++ b/inet/if_index.c
@@ -31,7 +31,7 @@ libc_hidden_weak (if_nametoindex)
 stub_warning (if_nametoindex)
 
 char *
-__if_indextoname (unsigned int ifindex, char *ifname)
+__if_indextoname (unsigned int ifindex, char ifname[IF_NAMESIZE])
 {
   __set_errno (ENOSYS);
   return NULL;
diff --git a/io/bits/poll2.h b/io/bits/poll2.h
index dca49717db..8cbedb7542 100644
--- a/io/bits/poll2.h
+++ b/io/bits/poll2.h
@@ -26,13 +26,14 @@ __BEGIN_DECLS
 extern int __REDIRECT (__poll_alias, (struct pollfd *__fds, nfds_t __nfds,
 				      int __timeout), poll);
 extern int __poll_chk (struct pollfd *__fds, nfds_t __nfds, int __timeout,
-		       __SIZE_TYPE__ __fdslen);
+		       __SIZE_TYPE__ __fdslen)
+    __attr_access ((__write_only__, 1, 2));
 extern int __REDIRECT (__poll_chk_warn, (struct pollfd *__fds, nfds_t __nfds,
 					 int __timeout, __SIZE_TYPE__ __fdslen),
 		       __poll_chk)
   __warnattr ("poll called with fds buffer too small file nfds entries");
 
-__fortify_function int
+__fortify_function __attr_access ((__write_only__, 1, 2)) int
 poll (struct pollfd *__fds, nfds_t __nfds, int __timeout)
 {
   if (__bos (__fds) != (__SIZE_TYPE__) -1)
@@ -53,7 +54,8 @@ extern int __REDIRECT (__ppoll_alias, (struct pollfd *__fds, nfds_t __nfds,
 				       const __sigset_t *__ss), ppoll);
 extern int __ppoll_chk (struct pollfd *__fds, nfds_t __nfds,
 			const struct timespec *__timeout,
-			const __sigset_t *__ss, __SIZE_TYPE__ __fdslen);
+			const __sigset_t *__ss, __SIZE_TYPE__ __fdslen)
+    __attr_access ((__write_only__, 1, 2));
 extern int __REDIRECT (__ppoll_chk_warn, (struct pollfd *__fds, nfds_t __nfds,
 					  const struct timespec *__timeout,
 					  const __sigset_t *__ss,
@@ -61,7 +63,7 @@ extern int __REDIRECT (__ppoll_chk_warn, (struct pollfd *__fds, nfds_t __nfds,
 		       __ppoll_chk)
   __warnattr ("ppoll called with fds buffer too small file nfds entries");
 
-__fortify_function int
+__fortify_function __attr_access ((__write_only__, 1, 2)) int
 ppoll (struct pollfd *__fds, nfds_t __nfds, const struct timespec *__timeout,
        const __sigset_t *__ss)
 {
diff --git a/io/sys/poll.h b/io/sys/poll.h
index 857be0f5ac..aa145a1737 100644
--- a/io/sys/poll.h
+++ b/io/sys/poll.h
@@ -51,7 +51,8 @@ __BEGIN_DECLS
 
    This function is a cancellation point and therefore not marked with
    __THROW.  */
-extern int poll (struct pollfd *__fds, nfds_t __nfds, int __timeout);
+extern int poll (struct pollfd *__fds, nfds_t __nfds, int __timeout)
+    __attr_access ((__write_only__, 1, 2));
 
 #ifdef __USE_GNU
 /* Like poll, but before waiting the threads signal mask is replaced
@@ -62,7 +63,9 @@ extern int poll (struct pollfd *__fds, nfds_t __nfds, int __timeout);
    __THROW.  */
 extern int ppoll (struct pollfd *__fds, nfds_t __nfds,
 		  const struct timespec *__timeout,
-		  const __sigset_t *__ss);
+		  const __sigset_t *__ss)
+    __attr_access ((__write_only__, 1, 2));
+
 #endif
 
 __END_DECLS
diff --git a/libio/stdio.h b/libio/stdio.h
index 998470943e..fb44f1e40f 100644
--- a/libio/stdio.h
+++ b/libio/stdio.h
@@ -184,12 +184,12 @@ extern FILE *tmpfile64 (void) __wur;
 #endif
 
 /* Generate a temporary filename.  */
-extern char *tmpnam (char *__s) __THROW __wur;
+extern char *tmpnam (char[L_tmpnam]) __THROW __wur;
 
 #ifdef __USE_MISC
 /* This is the reentrant variant of `tmpnam'.  The only difference is
    that it does not allow S to be NULL.  */
-extern char *tmpnam_r (char *__s) __THROW __wur;
+extern char *tmpnam_r (char __s[L_tmpnam]) __THROW __wur;
 #endif
 
 
@@ -808,13 +808,15 @@ extern int pclose (FILE *__stream);
 
 #ifdef	__USE_POSIX
 /* Return the name of the controlling terminal.  */
-extern char *ctermid (char *__s) __THROW;
+extern char *ctermid (char *__s) __THROW
+  __attr_access ((__write_only__, 1));
 #endif /* Use POSIX.  */
 
 
 #if (defined __USE_XOPEN && !defined __USE_XOPEN2K) || defined __USE_GNU
 /* Return the name of the current user.  */
-extern char *cuserid (char *__s);
+extern char *cuserid (char *__s)
+  __attr_access ((__write_only__, 1));
 #endif /* Use X/Open, but not issue 6.  */
 
 
diff --git a/misc/sys/uio.h b/misc/sys/uio.h
index 26d87c9f34..2e09858adc 100644
--- a/misc/sys/uio.h
+++ b/misc/sys/uio.h
@@ -39,7 +39,7 @@ __BEGIN_DECLS
    This function is a cancellation point and therefore not marked with
    __THROW.  */
 extern ssize_t readv (int __fd, const struct iovec *__iovec, int __count)
-  __wur;
+  __wur __attr_access ((__read_only__, 2, 3));
 
 /* Write data pointed by the buffers described by IOVEC, which
    is a vector of COUNT 'struct iovec's, to file descriptor FD.
@@ -50,7 +50,7 @@ extern ssize_t readv (int __fd, const struct iovec *__iovec, int __count)
    This function is a cancellation point and therefore not marked with
    __THROW.  */
 extern ssize_t writev (int __fd, const struct iovec *__iovec, int __count)
-  __wur;
+  __wur __attr_access ((__read_only__, 2, 3));
 
 
 #ifdef __USE_MISC
@@ -65,7 +65,8 @@ extern ssize_t writev (int __fd, const struct iovec *__iovec, int __count)
    This function is a cancellation point and therefore not marked with
    __THROW.  */
 extern ssize_t preadv (int __fd, const struct iovec *__iovec, int __count,
-		       __off_t __offset) __wur;
+		       __off_t __offset)
+  __wur __attr_access ((__read_only__, 2, 3));
 
 /* Write data pointed by the buffers described by IOVEC, which is a
    vector of COUNT 'struct iovec's, to file descriptor FD at the given
@@ -77,16 +78,19 @@ extern ssize_t preadv (int __fd, const struct iovec *__iovec, int __count,
    This function is a cancellation point and therefore not marked with
    __THROW.  */
 extern ssize_t pwritev (int __fd, const struct iovec *__iovec, int __count,
-			__off_t __offset) __wur;
+			__off_t __offset)
+  __wur __attr_access ((__read_only__, 2, 3));
 
 # else
 #  ifdef __REDIRECT
 extern ssize_t __REDIRECT (preadv, (int __fd, const struct iovec *__iovec,
 				    int __count, __off64_t __offset),
-			   preadv64) __wur;
+			   preadv64)
+  __wur __attr_access ((__read_only__, 2, 3));
 extern ssize_t __REDIRECT (pwritev, (int __fd, const struct iovec *__iovec,
 				     int __count, __off64_t __offset),
-			   pwritev64) __wur;
+			   pwritev64)
+  __wur __attr_access ((__read_only__, 2, 3));
 #  else
 #   define preadv preadv64
 #   define pwritev pwritev64
@@ -104,7 +108,8 @@ extern ssize_t __REDIRECT (pwritev, (int __fd, const struct iovec *__iovec,
    This function is a cancellation point and therefore not marked with
    __THROW.  */
 extern ssize_t preadv64 (int __fd, const struct iovec *__iovec, int __count,
-			 __off64_t __offset) __wur;
+			 __off64_t __offset)
+  __wur __attr_access ((__read_only__, 2, 3));
 
 /* Write data pointed by the buffers described by IOVEC, which is a
    vector of COUNT 'struct iovec's, to file descriptor FD at the given
@@ -116,7 +121,8 @@ extern ssize_t preadv64 (int __fd, const struct iovec *__iovec, int __count,
    This function is a cancellation point and therefore not marked with
    __THROW.  */
 extern ssize_t pwritev64 (int __fd, const struct iovec *__iovec, int __count,
-			  __off64_t __offset) __wur;
+			  __off64_t __offset)
+  __wur __attr_access ((__read_only__, 2, 3));
 # endif
 #endif	/* Use misc.  */
 
@@ -125,7 +131,8 @@ extern ssize_t pwritev64 (int __fd, const struct iovec *__iovec, int __count,
 # ifndef __USE_FILE_OFFSET64
 /* Same as preadv but with an additional flag argumenti defined at uio.h.  */
 extern ssize_t preadv2 (int __fp, const struct iovec *__iovec, int __count,
-			__off_t __offset, int ___flags) __wur;
+			__off_t __offset, int ___flags)
+  __wur __attr_access ((__read_only__, 2, 3));
 
 /* Same as preadv but with an additional flag argument defined at uio.h.  */
 extern ssize_t pwritev2 (int __fd, const struct iovec *__iodev, int __count,
@@ -136,11 +143,13 @@ extern ssize_t pwritev2 (int __fd, const struct iovec *__iodev, int __count,
 extern ssize_t __REDIRECT (pwritev2, (int __fd, const struct iovec *__iovec,
 				      int __count, __off64_t __offset,
 				      int __flags),
-			   pwritev64v2) __wur;
+			   pwritev64v2)
+  __wur __attr_access ((__read_only__, 2, 3));
 extern ssize_t __REDIRECT (preadv2, (int __fd, const struct iovec *__iovec,
 				     int __count, __off64_t __offset,
 				     int __flags),
-			   preadv64v2) __wur;
+			   preadv64v2)
+  __wur __attr_access ((__read_only__, 2, 3));
 #  else
 #   define preadv2 preadv64v2
 #   define pwritev2 pwritev64v2
@@ -151,12 +160,14 @@ extern ssize_t __REDIRECT (preadv2, (int __fd, const struct iovec *__iovec,
 /* Same as preadv but with an additional flag argumenti defined at uio.h.  */
 extern ssize_t preadv64v2 (int __fp, const struct iovec *__iovec,
 			   int __count, __off64_t __offset,
-			   int ___flags) __wur;
+			   int ___flags)
+  __wur __attr_access ((__read_only__, 2, 3));
 
 /* Same as preadv but with an additional flag argument defined at uio.h.  */
 extern ssize_t pwritev64v2 (int __fd, const struct iovec *__iodev,
 			    int __count, __off64_t __offset,
-			    int __flags) __wur;
+			    int __flags)
+  __wur __attr_access ((__read_only__, 2, 3));
 # endif
 #endif /* Use GNU.  */
 
diff --git a/posix/regex.h b/posix/regex.h
index 5fe41c8685..75c9201fc6 100644
--- a/posix/regex.h
+++ b/posix/regex.h
@@ -536,7 +536,8 @@ extern reg_syntax_t re_set_syntax (reg_syntax_t __syntax);
    'regcomp', with a malloc'ed value, or set to NULL before calling
    'regfree'.  */
 extern const char *re_compile_pattern (const char *__pattern, size_t __length,
-				       struct re_pattern_buffer *__buffer);
+				       struct re_pattern_buffer *__buffer)
+    __attr_access ((__read_only__, 1, 2));
 
 
 /* Compile a fastmap for the compiled pattern in BUFFER; used to
@@ -553,7 +554,8 @@ extern int re_compile_fastmap (struct re_pattern_buffer *__buffer);
 extern regoff_t re_search (struct re_pattern_buffer *__buffer,
 			   const char *__String, regoff_t __length,
 			   regoff_t __start, regoff_t __range,
-			   struct re_registers *__regs);
+			   struct re_registers *__regs)
+    __attr_access ((__read_only__, 2, 3));
 
 
 /* Like 're_search', but search in the concatenation of STRING1 and
@@ -563,14 +565,17 @@ extern regoff_t re_search_2 (struct re_pattern_buffer *__buffer,
 			     const char *__string2, regoff_t __length2,
 			     regoff_t __start, regoff_t __range,
 			     struct re_registers *__regs,
-			     regoff_t __stop);
+			     regoff_t __stop)
+    __attr_access ((__read_only__, 2, 3))
+    __attr_access ((__read_only__, 4, 5));
 
 
 /* Like 're_search', but return how many characters in STRING the regexp
    in BUFFER matched, starting at position START.  */
 extern regoff_t re_match (struct re_pattern_buffer *__buffer,
 			  const char *__String, regoff_t __length,
-			  regoff_t __start, struct re_registers *__regs);
+			  regoff_t __start, struct re_registers *__regs)
+    __attr_access ((__read_only__, 2, 3));
 
 
 /* Relates to 're_match' as 're_search_2' relates to 're_search'.  */
@@ -578,7 +583,9 @@ extern regoff_t re_match_2 (struct re_pattern_buffer *__buffer,
 			    const char *__string1, regoff_t __length1,
 			    const char *__string2, regoff_t __length2,
 			    regoff_t __start, struct re_registers *__regs,
-			    regoff_t __stop);
+			    regoff_t __stop)
+    __attr_access ((__read_only__, 2, 3))
+    __attr_access ((__read_only__, 4, 5));
 
 
 /* Set REGS to hold NUM_REGS registers, storing them in STARTS and
@@ -641,10 +648,12 @@ extern int regcomp (regex_t *_Restrict_ __preg,
 extern int regexec (const regex_t *_Restrict_ __preg,
 		    const char *_Restrict_ __String, size_t __nmatch,
 		    regmatch_t __pmatch[_Restrict_arr_],
-		    int __eflags);
+		    int __eflags)
+    __attr_access ((__write_only__, 4, 3));
 
 extern size_t regerror (int __errcode, const regex_t *_Restrict_ __preg,
-			char *_Restrict_ __errbuf, size_t __errbuf_size);
+			char *_Restrict_ __errbuf, size_t __errbuf_size)
+    __attr_access ((__write_only__, 3, 4));
 
 extern void regfree (regex_t *__preg);
 
diff --git a/pwd/pwd.h b/pwd/pwd.h
index bbc29479cd..065a19bade 100644
--- a/pwd/pwd.h
+++ b/pwd/pwd.h
@@ -139,20 +139,23 @@ extern struct passwd *getpwnam (const char *__name) __nonnull ((1));
 extern int getpwent_r (struct passwd *__restrict __resultbuf,
 		       char *__restrict __buffer, size_t __buflen,
 		       struct passwd **__restrict __result)
-		       __nonnull ((1, 2, 4));
+    __nonnull ((1, 2, 4))
+    __attr_access ((__write_only__, 2, 3));
 # endif
 
 extern int getpwuid_r (__uid_t __uid,
 		       struct passwd *__restrict __resultbuf,
 		       char *__restrict __buffer, size_t __buflen,
 		       struct passwd **__restrict __result)
-		       __nonnull ((2, 3, 5));
+    __nonnull ((2, 3, 5))
+    __attr_access ((__write_only__, 3, 4));
 
 extern int getpwnam_r (const char *__restrict __name,
 		       struct passwd *__restrict __resultbuf,
 		       char *__restrict __buffer, size_t __buflen,
 		       struct passwd **__restrict __result)
-		       __nonnull ((1, 2, 3, 5));
+    __nonnull ((1, 2, 3, 5))
+    __attr_access ((__write_only__, 3, 4));
 
 
 # ifdef	__USE_MISC
@@ -167,7 +170,8 @@ extern int fgetpwent_r (FILE *__restrict __stream,
 			struct passwd *__restrict __resultbuf,
 			char *__restrict __buffer, size_t __buflen,
 			struct passwd **__restrict __result)
-			__nonnull ((1, 2, 3, 5));
+    __nonnull ((1, 2, 3, 5))
+    __attr_access ((__write_only__, 3, 4));
 # endif
 
 #endif	/* POSIX or reentrant */
diff --git a/stdio-common/tmpnam.c b/stdio-common/tmpnam.c
index a8e0ca5b3b..cdd64e1d54 100644
--- a/stdio-common/tmpnam.c
+++ b/stdio-common/tmpnam.c
@@ -24,7 +24,7 @@ static char tmpnam_buffer[L_tmpnam];
 
    This function is *not* thread safe!  */
 char *
-tmpnam (char *s)
+tmpnam (char s[L_tmpnam])
 {
   /* By using two buffers we manage to be thread safe in the case
      where S != NULL.  */
diff --git a/stdio-common/tmpnam_r.c b/stdio-common/tmpnam_r.c
index 49f762f392..e359e4808a 100644
--- a/stdio-common/tmpnam_r.c
+++ b/stdio-common/tmpnam_r.c
@@ -20,7 +20,7 @@
 /* Generate a unique filename in P_tmpdir.  If S is NULL return NULL.
    This makes this function thread safe.  */
 char *
-tmpnam_r (char *s)
+tmpnam_r (char s[L_tmpnam])
 {
   if (s == NULL)
     return NULL;
diff --git a/stdlib/monetary.h b/stdlib/monetary.h
index c9d3c64e14..37ee8ab6d2 100644
--- a/stdlib/monetary.h
+++ b/stdlib/monetary.h
@@ -37,7 +37,8 @@ __BEGIN_DECLS
 /* Formatting a monetary value according to the current locale.  */
 extern ssize_t strfmon (char *__restrict __s, size_t __maxsize,
 			const char *__restrict __format, ...)
-     __THROW __attribute_format_strfmon__ (3, 4);
+     __THROW __attribute_format_strfmon__ (3, 4)
+     __attr_access ((__write_only__, 1, 2));
 
 #ifdef __USE_XOPEN2K8
 /* POSIX.1-2008 extended locale interface (see locale.h).  */
@@ -47,7 +48,8 @@ extern ssize_t strfmon (char *__restrict __s, size_t __maxsize,
 extern ssize_t strfmon_l (char *__restrict __s, size_t __maxsize,
 			  locale_t __loc,
 			  const char *__restrict __format, ...)
-     __THROW __attribute_format_strfmon__ (4, 5);
+     __THROW __attribute_format_strfmon__ (4, 5)
+     __attr_access ((__write_only__, 1, 2));
 #endif
 
 #include <bits/floatn.h>
diff --git a/sysdeps/gnu/net/if.h b/sysdeps/gnu/net/if.h
index e587a5ce59..30f3e281e5 100644
--- a/sysdeps/gnu/net/if.h
+++ b/sysdeps/gnu/net/if.h
@@ -191,7 +191,9 @@ __BEGIN_DECLS
 
 /* Convert an interface name to an index, and vice versa.  */
 extern unsigned int if_nametoindex (const char *__ifname) __THROW;
-extern char *if_indextoname (unsigned int __ifindex, char *__ifname) __THROW;
+extern char *if_indextoname (unsigned int __ifindex,
+			     char __ifname[IF_NAMESIZE]) __THROW
+    __attr_access ((__write_only__, 2));
 
 /* Return a list of all interfaces and their indices.  */
 extern struct if_nameindex *if_nameindex (void) __THROW;
diff --git a/sysdeps/mach/hurd/if_index.c b/sysdeps/mach/hurd/if_index.c
index 32dceccdbf..f92cd5723a 100644
--- a/sysdeps/mach/hurd/if_index.c
+++ b/sysdeps/mach/hurd/if_index.c
@@ -166,7 +166,7 @@ libc_hidden_weak (if_nameindex)
    IFNAME (which has space for at least IFNAMSIZ characters).  Return
    IFNAME, or NULL on error.  */
 char *
-__if_indextoname (unsigned int ifindex, char *ifname)
+__if_indextoname (unsigned int ifindex, char ifname[IF_NAMESIZE])
 {
   struct ifreq ifr;
   int fd = __opensock ();
diff --git a/sysdeps/unix/sysv/linux/if_index.c b/sysdeps/unix/sysv/linux/if_index.c
index dffa363931..5509da2c21 100644
--- a/sysdeps/unix/sysv/linux/if_index.c
+++ b/sysdeps/unix/sysv/linux/if_index.c
@@ -215,7 +215,7 @@ libc_hidden_weak (if_nameindex)
 
 
 char *
-__if_indextoname (unsigned int ifindex, char *ifname)
+__if_indextoname (unsigned int ifindex, char ifname[IF_NAMESIZE])
 {
   /* We may be able to do the conversion directly, rather than searching a
      list.  This ioctl is not present in kernels before version 2.1.50.  */