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'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
