[PATCH] extend -Wstringop-overflow to allocated objects (PR 91582)
Jeff Law
law@redhat.com
Mon Dec 2 17:06:00 GMT 2019
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
More information about the Gcc-patches
mailing list