Re: [PATCH] extend -Wstringop-overflow to allocated objects (PR 91582)

[Christophe Lyon <christophe.lyon@linaro.org>]

On Thu, 5 Dec 2019 at 02:37, Martin Sebor <msebor@gmail.com> wrote:
>
> On 12/2/19 10:06 AM, Jeff Law wrote:
> > 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.
>
> Yes, it should be.  It's correct in my tree but didn't post
> the updated revision.
>
> >
> >
> >> 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'm sure it was copied and pasted from the definition.  It makes
> no difference either way so it didn't get caught by anything.
>
> >
> > 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.
> >
>
> I've fixed the nits above and committed r278983 after retesting.
>

Hi Martin,

I've noticed that this patch introduces a new FAIL:
gcc.dg/Warray-bounds-56.c  (test for warnings, line 82)
when GCC is configured with:
--target arm-none-linux-gnueabihf
--with-mode arm
--with-cpu cortex-a5
--with-fpu vfpv3-d16-fp16

This test passes when using cortex-a9, a15, a57.

Christophe


> This is an improvement in the buffer overflow detection but there
> is still the (arguably more important) second half of it:
> extending the strlen pass to detect the overflow that cannot be
> caught later (e.g., all stores by MEM_REFs are only handled in
> strlen): https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02340.html
>
> Martin