On 12/6/19 8:44 AM, Christophe Lyon wrote:
On Thu, 5 Dec 2019 at 02:37, Martin Sebor <mse...@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.
Thanks for the heads up. It's most likely due to the same issue
as PR 92829 (seen on powerpc64*) that I started looking into
yesterday.
Martin
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
