Hi,
On Wed, 26 Aug 2020 at 22:36, Jeff Law via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> On Tue, 2020-06-23 at 20:05 -0600, Martin Sebor wrote:
> > -Wstringop-overflow is issued for both writing and reading past
> > the end, even though the latter problem is often viewed as being
> > lower severity than the former, or at least sufficiently different
> > to triage separately. In CWE, for example, each of the two kinds
> > of problems has its own classification (CWE-787: Out-of-bounds
> > Write, and CWE-125: Out-of-bounds Read).
> >
> > To make this easier with GCC, the attached patch introduces
> > a new option called -Wstringop-overread and splits the current
> > indiscriminate warning into the respective two categories. Other
> > than the new option the improvements also exposed a few instances
> > of reading past the end that GCC doesn't detect that the new code
> > does thanks to more consistent checking.
> >
> > As usual, I tested the patch by building Binutils/GDB, Glibc, and
> > the Linux kernel with no unexpected (or any, in fact) instances
> > of the two warnings.
> >
> > The work involved more churn that I expected, and maintaining
> > the expected precedence of warnings (missing terminating nul,
> > excessive bounds, etc.) turned out to be more delicate and time
> > consuming than I would have liked. I think the result is cleaner
> > code, but I'm quite sure it can still stand to be made better.
> > That's also my plan for the next step when I'd like to move this
> > checking out of builtins.c and calls.c and into a file of its own.
> > Ultimately, Jeff and have would like to integrate it into the path
> > isolation pass.
> >
> > Martin
> >
> > PS There's a FIXME comment in expand_builtin_memory_chk as
> > a reminder of a future change. It currently has no bearing
> > on anything.
>
> > Add -Wstringop-overread for reading past the end by string functions.
> >
> > gcc/ChangeLog:
> >
> > * attribs.c (init_attr_rdwr_indices): Use global access_mode.
> > * attribs.h (struct attr_access): Same.
> > * builtins.c (fold_builtin_strlen): Add argument.
> > (compute_objsize): Declare.
> > (get_range): Declare.
> > (check_read_access): New function.
> > (access_ref::access_ref): Define ctor.
> > (warn_string_no_nul): Add arguments. Handle -Wstrintop-overread.
> > (check_nul_terminated_array): Handle source strings of different
> > ranges of sizes.
> > (expand_builtin_strlen): Remove warning code, call check_read_access
> > instead. Declare locals closer to their initialization.
> > (expand_builtin_strnlen): Same.
> > (maybe_warn_for_bound): New function.
> > (warn_for_access): Remove argument. Handle -Wstrintop-overread.
> > (inform_access): Change argument type.
> > (get_size_range): New function.
> > (check_access): Remove unused arguments. Add new arguments. Handle
> > -Wstrintop-overread. Move warning code to helpers and call them.
> > Call check_nul_terminated_array.
> > (check_memop_access): Remove unnecessary and provide additional
> > arguments in calls.
> > (expand_builtin_memchr): Call check_read_access.
> > (expand_builtin_strcat): Remove unnecessary and provide additional
> > arguments in calls.
> > (expand_builtin_strcpy): Same.
> > (expand_builtin_strcpy_args): Same. Avoid testing no-warning bit.
> > (expand_builtin_stpcpy_1): Remove unnecessary and provide additional
> > arguments in calls.
> > (expand_builtin_stpncpy): Same.
> > (check_strncat_sizes): Same.
> > (expand_builtin_strncat): Remove unnecessary and provide additional
> > arguments in calls. Adjust comments.
> > (expand_builtin_strncpy): Remove unnecessary and provide additional
> > arguments in calls.
> > (expand_builtin_memcmp): Remove warning code. Call check_access.
> > (expand_builtin_strcmp): Call check_access instead of
> > check_nul_terminated_array.
> > (expand_builtin_strncmp): Handle -Wstrintop-overread.
> > (expand_builtin_fork_or_exec): Call check_access instead of
> > check_nul_terminated_array.
> > (expand_builtin): Same.
> > (fold_builtin_1): Pass additional argument.
> > (fold_builtin_n): Same.
> > (fold_builtin_strpbrk): Remove calls to check_nul_terminated_array.
> > (expand_builtin_memory_chk): Add comments.
> > (maybe_emit_chk_warning): Remove unnecessary and provide additional
> > arguments in calls.
> > (maybe_emit_sprintf_chk_warning): Same. Adjust comments.
> > * builtins.h (warn_string_no_nul): Add arguments.
> > (struct access_ref): Add member and ctor argument.
> > (struct access_data): Add members and ctor.
> > (check_access): Adjust signature.
> > * calls.c (maybe_warn_nonstring_arg): Return an indication of
> > whether a warning was issued. Issue -Wstrintop-overread instead
> > of -Wstringop-overflow.
> > (append_attrname): Adjust to naming changes.
> > (maybe_warn_rdwr_sizes): Same. Remove unnecessary and provide
> > additional arguments in calls.
> > * calls.h (maybe_warn_nonstring_arg): Return bool.
> > * doc/invoke.texi (-Wstringop-overread): Document new option.
> > * gimple-fold.c (gimple_fold_builtin_strcpy): Provide an additional
> > argument in call.
> > (gimple_fold_builtin_stpcpy): Same.
> > * tree-ssa-uninit.c (maybe_warn_pass_by_reference): Adjust to naming
> > changes.
> > * tree.h (enum access_mode): New type.
> >
> > gcc/c-family/ChangeLog:
> >
> > * c.opt (Wstringop-overread): New option.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * c-c++-common/Warray-bounds-7.c: Adjust expected warnings.
> > * c-c++-common/Wrestrict.c: Remove xfail.
> > * c-c++-common/attr-nonstring-3.c: Adjust text of expected warnings.
> > * c-c++-common/attr-nonstring-6.c: Suppress -Wstringop-overread
> > instead of -Wstringop-overflow.
> > * c-c++-common/attr-nonstring-8.c: Adjust text of expected warnings.
> > * g++.dg/torture/Wsizeof-pointer-memaccess1.C: Also suppress
> > -Wstringop-overread.
> > * gcc.dg/Warray-bounds-39.c: Adjust expected warnings.
> > * gcc.dg/Warray-bounds-40.c: Also suppress -Wstringop-overread.
> > * gcc.dg/Warray-bounds-58.c: Remove xfail. Also expect
> > -Wstringop-overread. Adjust text of expected warnings.
> > * gcc.dg/Wsizeof-pointer-memaccess1.c: Also suppress
> > -Wstringop-overread.
> > * gcc.dg/Wstringop-overflow-22.c: Adjust text of expected warnings.
> > * gcc.dg/Wstringop-overflow-33.c: Expect -Wstringop-overread.
> > * gcc.dg/Wstringop-overflow-9.c: Expect -Wstringop-overread.
> > * gcc.dg/attr-nonstring-2.c: Adjust text of expected warnings.
> > * gcc.dg/attr-nonstring-3.c: Same.
> > * gcc.dg/attr-nonstring-4.c: Same.
> > * gcc.dg/attr-nonstring.c: Expect -Wstringop-overread.
> > * gcc.dg/builtin-stringop-chk-5.c: Adjust comment.
> > * gcc.dg/builtin-stringop-chk-8.c: Enable -Wstringop-overread instead
> > of -Wstringop-overflow.
> > * gcc.dg/pr78902.c: Also expect -Wstringop-overread.
> > * gcc.dg/pr79214.c: Adjust text of expected warnings.
> > * gcc.dg/strcmpopt_10.c: Suppress valid -Wno-stringop-overread.
> > * gcc.dg/strlenopt-57.c: Also expect -Wstringop-overread.
> > * gcc.dg/torture/Wsizeof-pointer-memaccess1.c: Also suppress valid
> > -Wno-stringop-overread.
> > * gcc.dg/warn-strnlen-no-nul-2.c: Adjust text of expected warning.
> > * gcc.dg/warn-strnlen-no-nul.c: Same.
> > * gcc.dg/Wstringop-overread-2.c: New test.
> > * gcc.dg/Wstringop-overread.c: New test.
I pushed a small aarch64 patch as obvious:
2020-08-31 Christophe Lyon <christophe.l...@linaro.org>
gcc/testsuite/
* gcc.target/aarch64/strcmpopt_6.c: Suppress -Wstringop-overread.
(same as you added for i386)
On arm, there is a regression:
FAIL: c-c++-common/Warray-bounds-6.c -Wc++-compat (test for excess errors)
Excess errors:
/gcc/testsuite/c-c++-common/Warray-bounds-6.c:16:3: warning: 'strncpy'
writing 1 or more bytes into a region of size 0 overflows the
destination [-Wstringop-overflow=]
and the new test has several failures:
gcc.dg/Wstringop-overread.c (test for warnings, line 100)
gcc.dg/Wstringop-overread.c (test for warnings, line 110)
gcc.dg/Wstringop-overread.c (test for warnings, line 167)
gcc.dg/Wstringop-overread.c (test for warnings, line 177)
gcc.dg/Wstringop-overread.c (test for warnings, line 279)
gcc.dg/Wstringop-overread.c (test for warnings, line 289)
gcc.dg/Wstringop-overread.c (test for warnings, line 338)
gcc.dg/Wstringop-overread.c (test for warnings, line 372)
gcc.dg/Wstringop-overread.c (test for warnings, line 374)
gcc.dg/Wstringop-overread.c (test for warnings, line 532)
gcc.dg/Wstringop-overread.c (test for warnings, line 566)
gcc.dg/Wstringop-overread.c (test for warnings, line 568)
gcc.dg/Wstringop-overread.c (test for warnings, line 74)
gcc.dg/Wstringop-overread.c (test for warnings, line 84)
FAIL: gcc.dg/Wstringop-overread.c (test for excess errors)
Excess errors:
/gcc/testsuite/gcc.dg/Wstringop-overread.c:302:3: warning: 'strnlen'
specified bound [1, 4294967295] exceeds source size 0
[-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:306:3: warning: 'strnlen'
specified bound [1, 4294967295] exceeds source size 0
[-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:312:3: warning: 'strnlen'
specified bound [1, 4294967295] exceeds source size 0
[-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:323:3: warning: 'strnlen'
reading 1 or more bytes from a region of size 0 [-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:351:3: warning: 'strnlen'
reading 1 or more bytes from a region of size 0 [-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:498:3: warning: 'strndup'
specified bound [1, 4294967295] exceeds source size 0
[-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:502:3: warning: 'strndup'
specified bound [1, 4294967295] exceeds source size 0
[-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:508:3: warning: 'strndup'
specified bound [1, 4294967295] exceeds source size 0
[-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:518:3: warning: 'strndup'
reading 1 or more bytes from a region of size 0 [-Wstringop-overread]
/gcc/testsuite/gcc.dg/Wstringop-overread.c:545:3: warning: 'strndup'
reading 1 or more bytes from a region of size 0 [-Wstringop-overread]
Can you check these?
Thanks,
Christophe
> I'll note you've got another mega patch here that should have been broken down
> into smaller independent changes for easier/quicker review.
>
> For example, there's a lot of churn because of the enum changes. That's not a
> functional change, it's just barely a step beyond a global search and replace
> and
> could have been immediately approved and would have cut down on a lot of the
> noise.
>
> Similarly for the mechanical changes to add additional arguments to existing
> functions. The way to handle that is to add the arguments in the signature
> and
> the call sites, but don't use them initially. That makes no behavior change,
> is
> trivial to ACK and again allows focus on the meat of the change.
>
>
> Please please start thinking about how to break this kind of stuff out of the
> main patch. What looks obvious, simple and straightforward to the author is
> often not for the reviewer. The easier something is to review, generally the
> faster it will get reviewed.
>
> OK for the trunk,
> jeff
>
>
>
>
