middle-end/104854: Avoid overread warning for strnlen and strndup

  The size argument larger than size of SRC for strnlen and strndup is
problematic only if SRC is not NULL terminated, which invokes undefined
behaviour.  In all other cases, as long as SRC is large enough to have a
NULL char (i.e. size 1 or more), a larger N should not invoke a warning
during compilation.

Such a warning may be a suitable check for the static analyzer instead
with slightly different wording suggesting that choice of size argument
makes the function call equivalent to strlen/strdup.

This change results in the following code going through without a
warning:

------------------
char *buf;

char *
foo (void)
{
  buf = __builtin_malloc (4);
  __builtin_memset (buf, 'A', 4);

  return __builtin_strndup (buf,  5);
}

int
main ()
{
  __builtin_printf ("%s\n", foo ());
}
------------------

but the problem above is a missing NULL, not N being larger than the
size of SRC and the overread warning in this context is confusing at
best and misleading (and hinting at the wrong solution) in the worst
case.

gcc/ChangeLog:

	middle-end/104854
	* gimple-ssa-warn-access.cc (check_access<GimpleOrTree>):
	New parameter.  Skip warning if in read-only mode, source string
	is NULL terminated and has non-zero object size.
	(check_access<gimple *>): New parameter.
	(check_access<tree>): Adjust.
	(check_read_access): New parameter.  Adjust for check_access
	change.
	(pass_waccess::check_builtin): Adjust check_read_access call for
	memcmp, memchr.
	(pass_waccess::maybe_check_access_sizes): Likewise.

gcc/testsuite/ChangeLog:

	middle-end/104854
	* gcc.dg/Wstringop-overread.c
	(test_strnlen_array, test_strndup_array): Don't expect warning
	for non-zero source sizes.
	* gcc.dg/attr-nonstring-4.c (strnlen_range): Likewise.
	* gcc.dg/pr78902.c: Likewise.
	* gcc.dg/warn-strnlen-no-nul.c: Likewise.

Signed-off-by: Siddhesh Poyarekar <siddhesh@gotplt.org>
---
Tested with an x86_64 bootstrap.  strncmp has a similar issue, I'll post a
separate patch for it.

 gcc/gimple-ssa-warn-access.cc              | 35 ++++++++++++++--------
 gcc/testsuite/gcc.dg/Wstringop-overread.c  | 26 ++++++++--------
 gcc/testsuite/gcc.dg/attr-nonstring-4.c    |  2 +-
 gcc/testsuite/gcc.dg/pr78902.c             |  1 -
 gcc/testsuite/gcc.dg/warn-strnlen-no-nul.c | 16 +++++-----
 5 files changed, 45 insertions(+), 35 deletions(-)
  

On 10/03/2022 06:09, Siddhesh Poyarekar wrote:
> The size argument larger than size of SRC for strnlen and strndup is
> problematic only if SRC is not NULL terminated, which invokes undefined
> behaviour.  In all other cases, as long as SRC is large enough to have a
> NULL char (i.e. size 1 or more), a larger N should not invoke a warning
> during compilation.
> 
> Such a warning may be a suitable check for the static analyzer instead
> with slightly different wording suggesting that choice of size argument
> makes the function call equivalent to strlen/strdup.

This fix is too aggressive, I need to take another pass at this once 
stage 1 opens.

Siddhesh
  

On 3/9/2022 5:39 PM, Siddhesh Poyarekar wrote:
> The size argument larger than size of SRC for strnlen and strndup is
> problematic only if SRC is not NULL terminated, which invokes undefined
> behaviour.  In all other cases, as long as SRC is large enough to have a
> NULL char (i.e. size 1 or more), a larger N should not invoke a warning
> during compilation.
>
> Such a warning may be a suitable check for the static analyzer instead
> with slightly different wording suggesting that choice of size argument
> makes the function call equivalent to strlen/strdup.
>
> This change results in the following code going through without a
> warning:
>
> ------------------
> char *buf;
>
> char *
> foo (void)
> {
>    buf = __builtin_malloc (4);
>    __builtin_memset (buf, 'A', 4);
>
>    return __builtin_strndup (buf,  5);
> }
>
> int
> main ()
> {
>    __builtin_printf ("%s\n", foo ());
> }
> ------------------
>
> but the problem above is a missing NULL, not N being larger than the
> size of SRC and the overread warning in this context is confusing at
> best and misleading (and hinting at the wrong solution) in the worst
> case.
>
> gcc/ChangeLog:
>
> 	middle-end/104854
> 	* gimple-ssa-warn-access.cc (check_access<GimpleOrTree>):
> 	New parameter.  Skip warning if in read-only mode, source string
> 	is NULL terminated and has non-zero object size.
> 	(check_access<gimple *>): New parameter.
> 	(check_access<tree>): Adjust.
> 	(check_read_access): New parameter.  Adjust for check_access
> 	change.
> 	(pass_waccess::check_builtin): Adjust check_read_access call for
> 	memcmp, memchr.
> 	(pass_waccess::maybe_check_access_sizes): Likewise.
>
> gcc/testsuite/ChangeLog:
>
> 	middle-end/104854
> 	* gcc.dg/Wstringop-overread.c
> 	(test_strnlen_array, test_strndup_array): Don't expect warning
> 	for non-zero source sizes.
> 	* gcc.dg/attr-nonstring-4.c (strnlen_range): Likewise.
> 	* gcc.dg/pr78902.c: Likewise.
> 	* gcc.dg/warn-strnlen-no-nul.c: Likewise.
I know this is old and the BZ has been set as CLOSED/INVALID.  But it 
was in my TODO list,  and I've got thoughts here so I might as well 
chime in ;-)

The potential overread warning for that code seems quite reasonable to 
me.    Yes it is the case that the length argument is sometimes 
unrelated to the source string.  But even then where's the line for when 
we should and should not warn?

In my mind these middle end warnings can not and will not ever be 100% 
accurate.  Their goal is to say "hey, can someone please look at this 
code very closely because it might be wrong" -- and that's probably 
about the best we can do since many of these are trying to infer intent 
of the programmer.

It almost feels like we want not just to have finer grained control over 
this particular class of wrnings, but even within the class we may want 
levels (and just to be clear, I'm not generally a fan of leveled 
warnings as the higher levels rarely get used in practice).     The 
different levels could correspond to our ability to discern the source 
of the length operand -- ie, it is related to the source operand, some 
other operand or a constant and we could potentially treat each of those 
cases differently.

Jeff
  

On 10/07/2022 08:59, Jeff Law via Gcc-patches wrote:
> 
> 
> On 3/9/2022 5:39 PM, Siddhesh Poyarekar wrote:
>> The size argument larger than size of SRC for strnlen and strndup is
>> problematic only if SRC is not NULL terminated, which invokes undefined
>> behaviour.  In all other cases, as long as SRC is large enough to have a
>> NULL char (i.e. size 1 or more), a larger N should not invoke a warning
>> during compilation.
>>
>> Such a warning may be a suitable check for the static analyzer instead
>> with slightly different wording suggesting that choice of size argument
>> makes the function call equivalent to strlen/strdup.
>>
>> This change results in the following code going through without a
>> warning:
>>
>> ------------------
>> char *buf;
>>
>> char *
>> foo (void)
>> {
>>    buf = __builtin_malloc (4);
>>    __builtin_memset (buf, 'A', 4);
>>
>>    return __builtin_strndup (buf,  5);
>> }
>>
>> int
>> main ()
>> {
>>    __builtin_printf ("%s\n", foo ());
>> }
>> ------------------
>>
>> but the problem above is a missing NULL, not N being larger than the
>> size of SRC and the overread warning in this context is confusing at
>> best and misleading (and hinting at the wrong solution) in the worst
>> case.
>>
>> gcc/ChangeLog:
>>
>>     middle-end/104854
>>     * gimple-ssa-warn-access.cc (check_access<GimpleOrTree>):
>>     New parameter.  Skip warning if in read-only mode, source string
>>     is NULL terminated and has non-zero object size.
>>     (check_access<gimple *>): New parameter.
>>     (check_access<tree>): Adjust.
>>     (check_read_access): New parameter.  Adjust for check_access
>>     change.
>>     (pass_waccess::check_builtin): Adjust check_read_access call for
>>     memcmp, memchr.
>>     (pass_waccess::maybe_check_access_sizes): Likewise.
>>
>> gcc/testsuite/ChangeLog:
>>
>>     middle-end/104854
>>     * gcc.dg/Wstringop-overread.c
>>     (test_strnlen_array, test_strndup_array): Don't expect warning
>>     for non-zero source sizes.
>>     * gcc.dg/attr-nonstring-4.c (strnlen_range): Likewise.
>>     * gcc.dg/pr78902.c: Likewise.
>>     * gcc.dg/warn-strnlen-no-nul.c: Likewise.
> I know this is old and the BZ has been set as CLOSED/INVALID.  But it 
> was in my TODO list,  and I've got thoughts here so I might as well 
> chime in ;-)
> 
> The potential overread warning for that code seems quite reasonable to 
> me.    Yes it is the case that the length argument is sometimes 
> unrelated to the source string.  But even then where's the line for when 
> we should and should not warn?

The argument I was trying to make in the context of strnlen and strndup 
was that it is more likely in practice for the length argument to be a 
function of some other property, e.g. a destination buffer or an 
external limit that it is to be related to the source string.  However I 
don't have any concrete evidence (or the cycles to find it at the 
moment) to either back up my claim or refute it.  strndup for example 
seems popular for a substring alloc+copy and also for a general string 
copy with an application-specific upper bound, e.g. PATH_MAX.

Thanks,
Sid
  

On 7/9/2022 11:27 PM, Siddhesh Poyarekar wrote:
> On 10/07/2022 08:59, Jeff Law via Gcc-patches wrote:
>>
>>
>> On 3/9/2022 5:39 PM, Siddhesh Poyarekar wrote:
>>> The size argument larger than size of SRC for strnlen and strndup is
>>> problematic only if SRC is not NULL terminated, which invokes undefined
>>> behaviour.  In all other cases, as long as SRC is large enough to 
>>> have a
>>> NULL char (i.e. size 1 or more), a larger N should not invoke a warning
>>> during compilation.
>>>
>>> Such a warning may be a suitable check for the static analyzer instead
>>> with slightly different wording suggesting that choice of size argument
>>> makes the function call equivalent to strlen/strdup.
>>>
>>> This change results in the following code going through without a
>>> warning:
>>>
>>> ------------------
>>> char *buf;
>>>
>>> char *
>>> foo (void)
>>> {
>>>    buf = __builtin_malloc (4);
>>>    __builtin_memset (buf, 'A', 4);
>>>
>>>    return __builtin_strndup (buf,  5);
>>> }
>>>
>>> int
>>> main ()
>>> {
>>>    __builtin_printf ("%s\n", foo ());
>>> }
>>> ------------------
>>>
>>> but the problem above is a missing NULL, not N being larger than the
>>> size of SRC and the overread warning in this context is confusing at
>>> best and misleading (and hinting at the wrong solution) in the worst
>>> case.
>>>
>>> gcc/ChangeLog:
>>>
>>>     middle-end/104854
>>>     * gimple-ssa-warn-access.cc (check_access<GimpleOrTree>):
>>>     New parameter.  Skip warning if in read-only mode, source string
>>>     is NULL terminated and has non-zero object size.
>>>     (check_access<gimple *>): New parameter.
>>>     (check_access<tree>): Adjust.
>>>     (check_read_access): New parameter.  Adjust for check_access
>>>     change.
>>>     (pass_waccess::check_builtin): Adjust check_read_access call for
>>>     memcmp, memchr.
>>>     (pass_waccess::maybe_check_access_sizes): Likewise.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>>     middle-end/104854
>>>     * gcc.dg/Wstringop-overread.c
>>>     (test_strnlen_array, test_strndup_array): Don't expect warning
>>>     for non-zero source sizes.
>>>     * gcc.dg/attr-nonstring-4.c (strnlen_range): Likewise.
>>>     * gcc.dg/pr78902.c: Likewise.
>>>     * gcc.dg/warn-strnlen-no-nul.c: Likewise.
>> I know this is old and the BZ has been set as CLOSED/INVALID. But it 
>> was in my TODO list,  and I've got thoughts here so I might as well 
>> chime in ;-)
>>
>> The potential overread warning for that code seems quite reasonable 
>> to me.    Yes it is the case that the length argument is sometimes 
>> unrelated to the source string.  But even then where's the line for 
>> when we should and should not warn?
>
> The argument I was trying to make in the context of strnlen and 
> strndup was that it is more likely in practice for the length argument 
> to be a function of some other property, e.g. a destination buffer or 
> an external limit that it is to be related to the source string.  
> However I don't have any concrete evidence (or the cycles to find it 
> at the moment) to either back up my claim or refute it.  strndup for 
> example seems popular for a substring alloc+copy and also for a 
> general string copy with an application-specific upper bound, e.g. 
> PATH_MAX.
Yea, and those are going to be the really tough cases -- constants for 
the length argument.  However, those are the ones that probably need a 
human to really look at to determine safety or not, so warning seems 
appropriate to me.

Getting away from #defines towards const/constexpr like constructs would 
be a step forward, but I can't see that happening in a meaningful way in 
the C world.

This may all argue that these warnings don't belong in -Wall, which is 
obviously a distinct, but vitally important discussion.  I've always 
believed that we can make an educated guess about whether or not to 
include any given warning in -Wall, but we have to be flexible enough to 
take in feedback and adjust.  That's why I was always so interested in 
using Fedora mass builds to get data to drive these decisions.

Jeff
  

On 10/07/2022 21:44, Jeff Law wrote:
> This may all argue that these warnings don't belong in -Wall, which is 
> obviously a distinct, but vitally important discussion.  I've always 
> believed that we can make an educated guess about whether or not to 
> include any given warning in -Wall, but we have to be flexible enough to 
> take in feedback and adjust.  That's why I was always so interested in 
> using Fedora mass builds to get data to drive these decisions.

Yeah, that's the thing I'm trying to find some time to do, hopefully 
later in the year.

Thanks,
Sid