On August 10, 2017 7:26:33 AM GMT+02:00, Jeff Law <l...@redhat.com> wrote: >On 08/06/2017 02:07 PM, Martin Sebor wrote: >> Part 3 of the series contains the meat of the patch: the new >> -Wstringop-truncation option, and enhancements to -Wstringop- >> overflow, and -Wpointer-sizeof-memaccess to detect misuses of >> strncpy and strncat. >> >> Martin >> >> gcc-81117-3.diff >> >> >> PR c/81117 - Improve buffer overflow checking in strncpy >> >> gcc/ChangeLog: >> >> PR c/81117 >> * builtins.c (compute_objsize): Handle arrays that >> compute_builtin_object_size likes to fail for. Make extern. >> * builtins.h (compute_objsize): Declare. >> (check_strncpy_sizes): New function. >> (expand_builtin_strncpy): Call check_strncpy_sizes. >> * gimple-fold.c (gimple_fold_builtin_strncpy): Implement >> -Wstringop-truncation. >> (gimple_fold_builtin_strncat): Same. >> * gimple.c (gimple_build_call_from_tree): Set call location. >> * tree-ssa-strlen.c (strlen_to_stridx): New global variable. >> (maybe_diag_bound_equal_length, is_strlen_related_p): New functions. >> (handle_builtin_stxncpy, handle_builtin_strncat): Same. >> (handle_builtin_strlen): Use strlen_to_stridx. >> (strlen_optimize_stmt): Handle flavors of strncat, strncpy, and >> stpncpy. >> Use strlen_to_stridx. >> (pass_strlen::execute): Release strlen_to_stridx. >> * doc/invoke.texi (-Wsizeof-pointer-memaccess): Document >enhancement. >> (-Wstringop-truncation): Document new option. >> >> gcc/c-family/ChangeLog: >> >> PR c/81117 >> * c-warn.c (sizeof_pointer_memaccess_warning): Handle arrays. >> * c.opt (-Wstriingop-truncation): New option. >> >> gcc/testsuite/ChangeLog: >> >> PR c/81117 >> * c-c++-common/Wsizeof-pointer-memaccess3.c: New test. >> * c-c++-common/Wstringop-overflow.c: Same. >> * c-c++-common/Wstringop-truncation.c: Same. >> * c-c++-common/Wsizeof-pointer-memaccess2.c: Adjust. >> * c-c++-common/attr-nonstring-2.c: New test. >> * g++.dg/torture/Wsizeof-pointer-memaccess1.C: Adjust. >> * g++.dg/torture/Wsizeof-pointer-memaccess2.C: Same. >> * gcc.dg/torture/pr63554.c: Same. >> * gcc.dg/Walloca-1.c: Disable macro tracking. >> >> diff --git a/gcc/builtins.c b/gcc/builtins.c >> index 016f68d..1aa9e22 100644 >> --- a/gcc/builtins.c >> +++ b/gcc/builtins.c >[ ... ] >> + >> + if (TREE_CODE (type) == ARRAY_TYPE) >> + { >> + /* Return the constant size unless it's zero (that's a >zero-length >> + array likely at the end of a struct). */ >> + tree size = TYPE_SIZE_UNIT (type); >> + if (size && TREE_CODE (size) == INTEGER_CST >> + && !integer_zerop (size)) >> + return size; >> + } >Q. Do we have a canonical test for the trailing array idiom?
Array_at_struct_end_p In some >contexts isn't it size 1? ISTM This test needs slight improvement. >Ideally we'd use some canonical test for detect the trailing array >idiom >rather than open-coding it here. You might look at the array index >warnings in tree-vrp.c to see if it's got a canonical test you can call >or factor and use. > > > > >> @@ -3883,6 +3920,30 @@ expand_builtin_strncat (tree exp, rtx) >> return NULL_RTX; >> } >> >> +/* Helper to check the sizes of sequences and the destination of >calls >> + to __builtin_strncpy (DST, SRC, LEN) and __builtin___strncpy_chk. >> + Returns true on success (no overflow warning), false otherwise. >*/ >> + >> +static bool >> +check_strncpy_sizes (tree exp, tree dst, tree src, tree len) >> +{ >> + tree dstsize = compute_objsize (dst, warn_stringop_overflow - 1); >> + >> + if (!check_sizes (OPT_Wstringop_overflow_, >> + exp, len, /*maxlen=*/NULL_TREE, src, dstsize)) >> + return false; >> + >> + if (!dstsize || TREE_CODE (len) != INTEGER_CST) >> + return true; >> + >> + if (tree_int_cst_lt (dstsize, len)) >> + warning_at (EXPR_LOCATION (exp), OPT_Wstringop_truncation, >> + "%K%qD specified bound %E exceeds destination size %E", >> + exp, get_callee_fndecl (exp), len, dstsize); >> + >> + return true; >So in the case where you issue the warning, what should the return >value >be? According to the comment it should be false. It looks like you >got >the wrong return value for the tree_int_cst_lt (dstsize, len) test. > > > > > >> >> return false; >> diff --git a/gcc/tree-ssa-strlen.c b/gcc/tree-ssa-strlen.c >> index b0563fe..ac6503f 100644 >> --- a/gcc/tree-ssa-strlen.c >> +++ b/gcc/tree-ssa-strlen.c > >> + >> +/* A helper of handle_builtin_stxncpy. Check to see if the >specified >> + bound is a) equal to the size of the destination DST and if so, >b) >> + if it's immediately followed by DST[LEN - 1] = '\0'. If a) holds >> + and b) does not, warn. Otherwise, do nothing. Return true if >> + diagnostic has been issued. >> + >> + The purpose is to diagnose calls to strncpy and stpncpy that do >> + not nul-terminate the copy while allowing for the idiom where >> + such a call is immediately followed by setting the last element >> + to nul, as in: >> + char a[32]; >> + strncpy (a, s, sizeof a); >> + a[sizeof a - 1] = '\0'; >> +*/ >So using gsi_next to find the next statement could make the heuristic >fail to find the a[sizeof a - 1] = '\0'; statement when debugging is >enabled. > >gsi_next_nondebug would be better as it would skip over any debug >insns. > >What might be even better would be to use the immediate uses of the >memory tag. For your case there should be only one immediate use and >it >should point to the statement which NUL terminates the destination. Or >maybe that would be worse in that you only want to allow this exception >when the statements are consecutive. > > >> + >> + /* Look for dst[i] = '\0'; after the stxncpy() call and if found >> + avoid the truncation warning. */ >> + gsi_next (&gsi); >> + gimple *next_stmt = gsi_stmt (gsi); >Here's the gsi_next I'm referring to. > > >> + else >> + { >> + /* The source length is uknown. Try to determine the >destination >s/uknown/unknown/ > > > >> /* Handle a memcpy-like ({mem{,p}cpy,__mem{,p}cpy_chk}) call. >> If strlen of the second argument is known and length of the third >argument >> is that plus one, strlen of the first argument is the same after >this >> @@ -2512,6 +2789,19 @@ strlen_optimize_stmt (gimple_stmt_iterator >*gsi) >You still need to rename strlen_optimize_stmt since after your changes >it does both optimizations and warnings. > >I think we're going to need one more iteration on this patch within the >kit. I'm glazing over a bit tonight. > >jeff