Re: [PATCH] remove attribute access from regexec

[Martin Sebor <msebor@gmail.com>]

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

On 8/13/21 2:11 PM, Paul Eggert wrote:
> On 8/13/21 11:26 AM, Martin Sebor via Libc-alpha wrote:
>> -    __attr_access ((__write_only__, 4, 3));
>> +    __attr_access ((__read_write__, 4, 3));
> 
> Wouldn't it be simpler to remove the __attr_access instead?
> 
> What utility does __attr_access ((__read_write__, 4, 3)) have? FWIW, 
> Glibc currently does not use such an attribute anywhere.

The attribute serves two purposes: it specifies 1) how the function
might access the array (to determine whether it should be initialized
by the caller) and 2) the minimum number of elements the caller must
provide and the maximum number of elements the function definition
might access.

GCC checks to make sure these constraints are satisfied, both at call
sites and in the definitions of these functions.

The read_write mode means that a function may both read from and write
to the array, and expects it to be initialized.  But since regexec
might just write to the array and not read from it, depending on
flags, the read_write mode wouldn't be correct either.

There is no attribute access mode that would capture this but in C99
(though sadly not in C++) and thanks to the nmatch argument preceding
pmatch, the VLA argument syntax does make the checking possible.  It's
like attribute access with an unspecified mode.  (It might be worth
extending the attribute to express this mode so it can be used when
the bound doesn't precede the VLA argument and in C++.)

Attached is a revised patch with this approach.

Martin

PS POSIX says regexec() ignores pnmatch when REG_NOSUB is set, so
strictly speaking, warning for such calls to it in that case is
also a false positive.  I think that's an acceptable trade-off for
the buffer overflow detection (passing a zero nmatch in that case
is a trivial way to avoid the warning).

[-- Attachment #2: glibc-28170.diff --]
[-- Type: text/x-patch, Size: 1753 bytes --]

diff --git a/include/regex.h b/include/regex.h
index 24eca2c297..d67ec65ad9 100644
--- a/include/regex.h
+++ b/include/regex.h
@@ -36,8 +36,15 @@ extern void __re_set_registers
 extern int __regcomp (regex_t *__preg, const char *__pattern, int __cflags);
 libc_hidden_proto (__regcomp)
 
+#if 199901L <= __STDC_VERSION__ && !defined (__STDC_NO_VLA__)
 extern int __regexec (const regex_t *__preg, const char *__string,
-		      size_t __nmatch, regmatch_t __pmatch[], int __eflags);
+		      size_t __nmatch, regmatch_t __pmatch[__nmatch],
+		      int __eflags);
+#else
+extern int __regexec (const regex_t *__preg, const char *__string,
+		      size_t __nmatch, regmatch_t __pmatch[],
+		      int __eflags);
+#endif
 libc_hidden_proto (__regexec)
 
 extern size_t __regerror (int __errcode, const regex_t *__preg,
diff --git a/posix/regex.h b/posix/regex.h
index 14fb1d8364..ebacbaf95d 100644
--- a/posix/regex.h
+++ b/posix/regex.h
@@ -652,11 +652,17 @@ extern int regcomp (regex_t *_Restrict_ __preg,
 		    const char *_Restrict_ __pattern,
 		    int __cflags);
 
+#if 199901L <= __STDC_VERSION__ && !defined (__STDC_NO_VLA__)
 extern int regexec (const regex_t *_Restrict_ __preg,
 		    const char *_Restrict_ __String, size_t __nmatch,
-		    regmatch_t __pmatch[_Restrict_arr_],
-		    int __eflags)
-    __attr_access ((__write_only__, 4, 3));
+		    regmatch_t __pmatch[_Restrict_arr_ __nmatch],
+		    int __eflags);
+#else
+extern int regexec (const regex_t *_Restrict_ __preg,
+		    const char *_Restrict_ __String, size_t __nmatch,
+		    regmatch_t __pmatch[_Restrict_arr],
+		    int __eflags);
+#endif
 
 extern size_t regerror (int __errcode, const regex_t *_Restrict_ __preg,
 			char *_Restrict_ __errbuf, size_t __errbuf_size)