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
