On 11/8/19 3:11 PM, Martin Sebor wrote: > Unless it's used with _FORTIFY_SOURCE, -Wstringop-overflow > doesn't consider out-of-bounds accesses to objects allocated > by alloca, malloc, other functions declared with attribute > alloc_size, or even VLAs with variable bounds. This was > a known limitation of the checks (done just before expansion) > relying on the the object size pass when they were introduced > in GCC 7. > > But since its introduction in GCC 7, the warning has evolved > beyond some of the limitations of the object size pass. Unlike > it, the warning considers non-constant offsets and stores with > non-constant sizes. Attached is a simple enhancement that > (finally) adds the ability to also detect overflow in allocated > objects to the warning. > > With the patch GCC detects the overflow in code like this: > > char* f (void) > { > char s[] = "12345"; > char *p = malloc (strlen (s)); > strcpy (p, s); // warning here > return p; > } > > but not (yet) in something like this: > > char* g (const char *s) > { > char *p = malloc (strlen (s)); > strcpy (p, s); // no warning (yet) > return p; > } > > and quite a few other examples. Doing better requires extending > the strlen pass. I'm working on this extension and expect to > submit a patch before stage 1 ends. > > Martin > > PS I was originally planning to do all the allocation checking > in the strlen pass but it occurred to me that by also enhancing > the compute_objsize function, all warnings that use it will > benefit. Besides -Wstringop-overflow this includes a subset > of -Warray-bounds, -Wformat-overflow, and -Wrestrict. It's > nice when a small enhancement has such a broad positive effect.
> PR middle-end/91582 - missing heap overflow detection for strcpy > > gcc/ChangeLog: > > * builtins.c (gimple_call_alloc_size): New function. > (compute_objsize): Add argument. Call gimple_call_alloc_size. > Handle variable offsets and indices. > * builtins.h (gimple_call_alloc_size): Declare. > (compute_objsize): Add argument. > * tree-ssa-strlen.c (handle_store): Handle calls to allocated objects. > > gcc/testsuite/ChangeLog: > > * c-c++-common/Wstringop-truncation.c: Remove xfails. > * gcc/testsuite/g++.dg/ext/attr-alloc_size.C: Suppress -Warray-bounds. > * gcc.dg/Wstringop-overflow-22.c: New test. > * gcc/testsuite/gcc.dg/attr-alloc_size.c: Suppress -Warray-bounds. > * gcc/testsuite/gcc.dg/attr-copy-2.c: Same. > * gcc.dg/builtin-stringop-chk-5.c: Remove xfails. > * gcc.dg/builtin-stringop-chk-8.c: Same. Correct the text of expected > warnings. > * gcc.target/i386/pr82002-2a.c: Prune expected warning. > * gcc.target/i386/pr82002-2b.c: Same. [ ... ] > Index: gcc/builtins.c > =================================================================== > --- gcc/builtins.c (revision 277978) > +++ gcc/builtins.c (working copy) > @@ -3563,6 +3563,80 @@ check_access (tree exp, tree, tree, tree dstwrite, > return true; > } > > +/* If STMT is a call to an allocation function, returns the size > + of the object allocated by the call. */ > + > +tree > +gimple_call_alloc_size (gimple *stmt) > +{ > + tree size = gimple_call_arg (stmt, argidx1); > + tree n = argidx2 < nargs ? gimple_call_arg (stmt, argidx2) : > integer_one_node; > + > + /* To handle ranges do the math in wide_int and return the product > + of the upper bounds as a constant. Ignore anti-ranges. */ > + wide_int rng1[2]; > + if (TREE_CODE (size) == INTEGER_CST) > + rng1[0] = rng1[1] = wi::to_wide (size); > + else if (TREE_CODE (size) != SSA_NAME > + || get_range_info (size, rng1, rng1 + 1) != VR_RANGE) > + return NULL_TREE; > + > + wide_int rng2[2]; > + if (TREE_CODE (n) == INTEGER_CST) > + rng2[0] = rng2[1] = wi::to_wide (n); > + else if (TREE_CODE (n) != SSA_NAME > + || get_range_info (n, rng2 + 1, rng2 + 1) != VR_RANGE) > + return NULL_TREE; Should that 2nd call to get_range_info be "get_range_info (n, rng2, rng2 + 1)? I don't think it makes any difference in practice due to the implementation of get_range_info, but if it wasn't intentional let's get it fixed. > Index: gcc/builtins.h > =================================================================== > --- gcc/builtins.h (revision 277978) > +++ gcc/builtins.h (working copy) > @@ -134,7 +134,8 @@ extern tree fold_call_stmt (gcall *, bool); > extern void set_builtin_user_assembler_name (tree decl, const char *asmspec); > extern bool is_simple_builtin (tree); > extern bool is_inexpensive_builtin (tree); > -extern tree compute_objsize (tree, int, tree * = NULL); > +tree gimple_call_alloc_size (gimple *); > +extern tree compute_objsize (tree, int, tree * = NULL, tree * = NULL); > > extern bool readonly_data_expr (tree exp); > extern bool init_target_chars (void); Is there a reason there's no "extern" on the gimple_call_alloc_size prototype? I think this is fine with those nits fixed. You'll have a minor merge conflict with the compute_objsize changes due to recent fixes in the same hunk of code, but I don't think it warrants reposting/resubmission. Jeff