| Message ID | 20220310003955.1784701-1-siddhesh@gotplt.org |
|---|---|
| State | New |
| Headers | show |
| Series | middle-end/104854: Avoid overread warning for strnlen and strndup | expand |
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
diff --git a/gcc/gimple-ssa-warn-access.cc b/gcc/gimple-ssa-warn-access.cc index c36cd5d45d4..972e80e4b62 100644 --- a/gcc/gimple-ssa-warn-access.cc +++ b/gcc/gimple-ssa-warn-access.cc @@ -1256,7 +1256,7 @@ static bool check_access (GimpleOrTree exp, tree dstwrite, tree maxread, tree srcstr, tree dstsize, access_mode mode, const access_data *pad, - range_query *rvals) + range_query *rvals, bool null_terminated) { /* The size of the largest object is half the address space, or PTRDIFF_MAX. (This is way too permissive.) */ @@ -1431,6 +1431,15 @@ check_access (GimpleOrTree exp, tree dstwrite, } } + /* For functions that take string inputs and stop reading on encountering a + NULL, if remaining size in the source is non-zero, it is legitimate for + such functions to pass a larger size (that perhaps is the maximum object + size of all possible inputs), making the MAXREAD comparison noisy. */ + if (null_terminated + && pad && pad->mode == access_read_only + && pad->src.size_remaining () != 0) + return true; + /* Check the maximum length of the source sequence against the size of the destination object if known, or against the maximum size of an object. */ @@ -1522,10 +1531,10 @@ static bool check_access (gimple *stmt, tree dstwrite, tree maxread, tree srcstr, tree dstsize, access_mode mode, const access_data *pad, - range_query *rvals) + range_query *rvals, bool null_terminated = true) { return check_access<gimple *> (stmt, dstwrite, maxread, srcstr, dstsize, - mode, pad, rvals); + mode, pad, rvals, null_terminated); } bool @@ -1534,7 +1543,7 @@ check_access (tree expr, tree dstwrite, access_mode mode, const access_data *pad /* = NULL */) { return check_access<tree> (expr, dstwrite, maxread, srcstr, dstsize, - mode, pad, nullptr); + mode, pad, nullptr, true); } /* Return true if STMT is a call to an allocation function. Unless @@ -2109,7 +2118,8 @@ private: void check_stxncpy (gcall *); void check_strncmp (gcall *); void check_memop_access (gimple *, tree, tree, tree); - void check_read_access (gimple *, tree, tree = NULL_TREE, int = 1); + void check_read_access (gimple *, tree, tree = NULL_TREE, int = 1, + bool = true); void maybe_check_dealloc_call (gcall *); void maybe_check_access_sizes (rdwr_map *, tree, tree, gimple *); @@ -2745,7 +2755,7 @@ pass_waccess::check_memop_access (gimple *stmt, tree dest, tree src, tree size) tree dstsize = compute_objsize (dest, stmt, 0, &data.dst, &m_ptr_qry); check_access (stmt, size, /*maxread=*/NULL_TREE, srcsize, dstsize, - data.mode, &data, m_ptr_qry.rvals); + data.mode, &data, m_ptr_qry.rvals, false); } /* A convenience wrapper for check_access to check access by a read-only @@ -2754,7 +2764,8 @@ pass_waccess::check_memop_access (gimple *stmt, tree dest, tree src, tree size) void pass_waccess::check_read_access (gimple *stmt, tree src, tree bound /* = NULL_TREE */, - int ost /* = 1 */) + int ost /* = 1 */, + bool null_terminated /* = true */) { if (m_early_checks_p || !warn_stringop_overread) return; @@ -2770,7 +2781,7 @@ pass_waccess::check_read_access (gimple *stmt, tree src, compute_objsize (src, stmt, ost, &data.src, &m_ptr_qry); check_access (stmt, /*dstwrite=*/ NULL_TREE, /*maxread=*/ bound, /*srcstr=*/ src, /*dstsize=*/ NULL_TREE, data.mode, - &data, m_ptr_qry.rvals); + &data, m_ptr_qry.rvals, null_terminated); } /* Return true if memory model ORD is constant in the context of STMT and @@ -3230,8 +3241,8 @@ pass_waccess::check_builtin (gcall *stmt) tree a1 = call_arg (stmt, 0); tree a2 = call_arg (stmt, 1); tree len = call_arg (stmt, 2); - check_read_access (stmt, a1, len, 0); - check_read_access (stmt, a2, len, 0); + check_read_access (stmt, a1, len, 0, false); + check_read_access (stmt, a2, len, 0, false); return true; } @@ -3250,7 +3261,7 @@ pass_waccess::check_builtin (gcall *stmt) { tree src = call_arg (stmt, 0); tree len = call_arg (stmt, 2); - check_read_access (stmt, src, len, 0); + check_read_access (stmt, src, len, 0, false); return true; } @@ -3546,7 +3557,7 @@ pass_waccess::maybe_check_access_sizes (rdwr_map *rwm, tree fndecl, tree fntype, if (mode == access_deferred) mode = TYPE_READONLY (argtype) ? access_read_only : access_read_write; check_access (stmt, access_size, /*maxread=*/ NULL_TREE, srcsize, - dstsize, mode, &data, m_ptr_qry.rvals); + dstsize, mode, &data, m_ptr_qry.rvals, false); if (warning_suppressed_p (stmt, OPT_Wstringop_overflow_)) opt_warned = OPT_Wstringop_overflow_; diff --git a/gcc/testsuite/gcc.dg/Wstringop-overread.c b/gcc/testsuite/gcc.dg/Wstringop-overread.c index 7db74029819..709e7fff63e 100644 --- a/gcc/testsuite/gcc.dg/Wstringop-overread.c +++ b/gcc/testsuite/gcc.dg/Wstringop-overread.c @@ -313,7 +313,7 @@ void test_strnlen_array (int i, int i0, unsigned n) T (strnlen (a1, 0)); T (strnlen (a1, 1)); - T (strnlen (a1, 2)); // { dg-warning "'strnlen' specified bound 2 exceeds source size 1" "pr87492" { xfail *-*-* } } + T (strnlen (a1, 2)); T (strnlen (a1, n)); T (strnlen (a1 + 1, 0)); @@ -323,16 +323,16 @@ void test_strnlen_array (int i, int i0, unsigned n) T (strnlen (a1 + 1, n)); T (strnlen (a1 + i, 0)); T (strnlen (a1 + i, 1)); - T (strnlen (a1 + i, 2)); // { dg-warning "'strnlen' specified bound 2 exceeds source size 1" } + T (strnlen (a1 + i, 2)); T (strnlen (a1 + i, n)); T (strnlen (a1 + i + 1, 0)); T (strnlen (a1 + i + 1, 1)); - T (strnlen (a1 + i + 1, 2)); // { dg-warning "'strnlen' specified bound 2 exceeds source size 1" } + T (strnlen (a1 + i + 1, 2)); T (strnlen (a1 + i + 1, n)); T (strnlen (a1 + i0, 0)); T (strnlen (a1 + i0, 1)); - T (strnlen (a1 + i0, 2)); // { dg-warning "'strnlen' specified bound 2 exceeds source size 1" } + T (strnlen (a1 + i0, 2)); T (strnlen (a1 + i0, n)); T (strnlen (a1 + i0 + 1, 0)); T (strnlen (a1 + i0 + 1, 1)); // { dg-warning "'strnlen' specified bound 1 exceeds source size 0" } @@ -344,7 +344,7 @@ void test_strnlen_array (int i, int i0, unsigned n) T (strnlen (a2, n)); T (strnlen (a2 + 1, 0)); T (strnlen (a2 + 1, 1)); - T (strnlen (a2 + 1, 2)); // { dg-warning "'strnlen' specified bound 2 exceeds source size 1" "pr87492" } + T (strnlen (a2 + 1, 2)); T (strnlen (a2 + 1, n)); T (strnlen (a2 + 2, 0)); T (strnlen (a2 + 2, 1)); // { dg-warning "'strnlen' specified bound 1 exceeds source size 0" } @@ -360,12 +360,12 @@ void test_strnlen_array (int i, int i0, unsigned n) T (strnlen (a2 + i0, 0)); T (strnlen (a2 + i0, 1)); T (strnlen (a2 + i0, 2)); - T (strnlen (a2 + i0, 3)); // { dg-warning "'strnlen' specified bound 3 exceeds source size 2" } + T (strnlen (a2 + i0, 3)); T (strnlen (a2 + i0, n)); T (strnlen (a2 + i0 + 1, 0)); T (strnlen (a2 + i0 + 1, 1)); - T (strnlen (a2 + i0 + 1, 2)); // { dg-warning "'strnlen' specified bound 2 exceeds source size 1" } + T (strnlen (a2 + i0 + 1, 2)); T (strnlen (a2 + i0 + 1, n)); T (strnlen (a2 + i0 + 2, 0)); @@ -509,7 +509,7 @@ void test_strndup_array (int i, int i0, unsigned n) T (strndup (a1, 0)); T (strndup (a1, 1)); - T (strndup (a1, 2)); // { dg-warning "'strndup' specified bound 2 exceeds source size 1" } + T (strndup (a1, 2)); T (strndup (a1, n)); T (strndup (a1 + 1, 0)); T (strndup (a1 + 1, 1)); // { dg-warning "'strndup' specified bound 1 exceeds source size 0" } @@ -518,11 +518,11 @@ void test_strndup_array (int i, int i0, unsigned n) T (strndup (a1 + 1, n)); T (strndup (a1 + i, 0)); T (strndup (a1 + i, 1)); - T (strndup (a1 + i, 2)); // { dg-warning "'strndup' specified bound 2 exceeds source size 1" } + T (strndup (a1 + i, 2)); T (strndup (a1 + i, n)); T (strndup (a1 + i + 1, 0)); T (strndup (a1 + i + 1, 1)); - T (strndup (a1 + i + 1, 2)); // { dg-warning "'strndup' specified bound 2 exceeds source size 1" } + T (strndup (a1 + i + 1, 2)); T (strndup (a1 + i + 1, n)); T (strndup (a1 + i0, 0)); @@ -538,7 +538,7 @@ void test_strndup_array (int i, int i0, unsigned n) T (strndup (a2, n)); T (strndup (a2 + 1, 0)); T (strndup (a2 + 1, 1)); - T (strndup (a2 + 1, 2)); // { dg-warning "'strndup' specified bound 2 exceeds source size 1" } + T (strndup (a2 + 1, 2)); T (strndup (a2 + 1, n)); T (strndup (a2 + 2, 0)); T (strndup (a2 + 2, 1)); // { dg-warning "'strndup' specified bound 1 exceeds source size 0" } @@ -554,12 +554,12 @@ void test_strndup_array (int i, int i0, unsigned n) T (strndup (a2 + i0, 0)); T (strndup (a2 + i0, 1)); T (strndup (a2 + i0, 2)); - T (strndup (a2 + i0, 3)); // { dg-warning "'strndup' specified bound 3 exceeds source size 2" } + T (strndup (a2 + i0, 3)); T (strndup (a2 + i0, n)); T (strndup (a2 + i0 + 1, 0)); T (strndup (a2 + i0 + 1, 1)); - T (strndup (a2 + i0 + 1, 2)); // { dg-warning "'strndup' specified bound 2 exceeds source size 1" } + T (strndup (a2 + i0 + 1, 2)); T (strndup (a2 + i0 + 1, n)); T (strndup (a2 + i0 + 2, 0)); diff --git a/gcc/testsuite/gcc.dg/attr-nonstring-4.c b/gcc/testsuite/gcc.dg/attr-nonstring-4.c index 6f03a562026..0ce062e513b 100644 --- a/gcc/testsuite/gcc.dg/attr-nonstring-4.c +++ b/gcc/testsuite/gcc.dg/attr-nonstring-4.c @@ -53,7 +53,7 @@ void strnlen_range (void) T (STR, /* [] */, n); T (STR, /* [] */, n + 1); /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */ - T (STR, 1, n); /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds source size 1" } */ + T (STR, 1, n); T (STR, 2, n + 1); /* { dg-warning "specified bound \\\[\[0-9\]+, \[0-9\]+] exceeds maximum object size \[0-9\]+" } */ T (NS, /* [] */, n); diff --git a/gcc/testsuite/gcc.dg/pr78902.c b/gcc/testsuite/gcc.dg/pr78902.c index a9a617cde23..51b42548071 100644 --- a/gcc/testsuite/gcc.dg/pr78902.c +++ b/gcc/testsuite/gcc.dg/pr78902.c @@ -12,5 +12,4 @@ void foo(void) __builtin_aligned_alloc (10, 16); /* { dg-warning "ignoring return value of '__builtin_aligned_alloc' declared with attribute 'warn_unused_result'" } */ __builtin_strdup ("pes"); /* { dg-warning "ignoring return value of '__builtin_strdup' declared with attribute 'warn_unused_result'" } */ __builtin_strndup ("pes", 10); /* { dg-warning "ignoring return value of '__builtin_strndup' declared with attribute 'warn_unused_result'" } */ - /* { dg-warning "\\\[-Wstringop-overread" "strndup excessive bound" { target *-*-* } .-1 } */ } diff --git a/gcc/testsuite/gcc.dg/warn-strnlen-no-nul.c b/gcc/testsuite/gcc.dg/warn-strnlen-no-nul.c index 70f6a432b97..b9ee69c0a2a 100644 --- a/gcc/testsuite/gcc.dg/warn-strnlen-no-nul.c +++ b/gcc/testsuite/gcc.dg/warn-strnlen-no-nul.c @@ -147,11 +147,11 @@ T (v0 ? b[3] : "", bsz); the strnlen calls are safe because the reads are bounded by the length of the constant arguments. Most of the calls are not diagnosed anymore as a result of the fix for PR 103215. */ -T (v0 ? "" : b[0], bsz + 1); /* { dg-warning "bound 6 exceeds source size 5" } */ +T (v0 ? "" : b[0], bsz + 1); T (v0 ? "" : b[1], bsz + 1); T (v0 ? "" : b[2], bsz + 1); T (v0 ? "" : b[3], bsz + 1); -T (v0 ? b[0] : "", bsz + 1); /* { dg-warning "bound 6 exceeds source size 5" } */ +T (v0 ? b[0] : "", bsz + 1); T (v0 ? b[1] : "", bsz + 1); T (v0 ? b[2] : "", bsz + 1); T (v0 ? b[3] : "", bsz + 1); @@ -218,10 +218,10 @@ T (&s.a[i1] + v0, asz); T (s.a, asz + 1); T (&s.a[0], asz + 1); T (&s.a[0] + 1, asz + 1); -T (&s.a[0] + v0, asz + 1); /* { dg-warning "specified bound 6 exceeds source size 5 " } */ +T (&s.a[0] + v0, asz + 1); T (&s.a[1], asz + 1); T (&s.a[1] + 1, asz + 1); -T (&s.a[1] + v0, asz + 1); /* { dg-bogus "specified bound 6 exceeds source size 5" "pr95794" { xfail *-*-* } } */ +T (&s.a[1] + v0, asz + 1); T (&s.a[i0], asz + 1); T (&s.a[i0] + i1, asz + 1); @@ -272,10 +272,10 @@ const struct B ba[] = { T (ba[0].a[0].a, asz + 1); T (&ba[0].a[0].a[0], asz + 1); T (&ba[0].a[0].a[0] + 1, asz + 1); -T (&ba[0].a[0].a[0] + v0, asz + 1); /* { dg-bogus "specified bound 6 exceeds source size 5" pr95794" { xfail *-*-* } } */ +T (&ba[0].a[0].a[0] + v0, asz + 1); T (&ba[0].a[0].a[1], asz + 1); T (&ba[0].a[0].a[1] + 1, asz + 1); -T (&ba[0].a[0].a[1] + v0, asz + 1); /* { dg-bogus "specified bound 6 exceeds source size 5" pr95794" { xfail *-*-* } } */ +T (&ba[0].a[0].a[1] + v0, asz + 1); T (ba[0].a[0].b, bsz); T (&ba[0].a[0].b[0], bsz); @@ -308,10 +308,10 @@ T (&ba[0].a[1].a[1] + v0, asz + 1); /* { dg-warning "unterminated" } */ T (ba[0].a[1].b, bsz + 1); T (&ba[0].a[1].b[0], bsz + 1); T (&ba[0].a[1].b[0] + 1, bsz + 1); -T (&ba[0].a[1].b[0] + v0, bsz + 1); /* { dg-bogus "specified bound 6 exceeds source size 5" pr95794" { xfail *-*-* } } */ +T (&ba[0].a[1].b[0] + v0, bsz + 1); T (&ba[0].a[1].b[1], bsz + 1); T (&ba[0].a[1].b[1] + 1, bsz + 1); -T (&ba[0].a[1].b[1] + v0, bsz + 1); /* { dg-bogus "specified bound 6 exceeds source size 5" pr95794" { xfail *-*-* } } */ +T (&ba[0].a[1].b[1] + v0, bsz + 1); T (ba[1].a[0].a, asz); T (&ba[1].a[0].a[0], asz);
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(-)