This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
[PATCH] Fix ubsan handling of BIND_EXPR (PR sanitizer/71498)
- From: Jakub Jelinek <jakub at redhat dot com>
- To: "Joseph S. Myers" <joseph at codesourcery dot com>, Jason Merrill <jason at redhat dot com>, Marek Polacek <polacek at redhat dot com>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Mon, 13 Jun 2016 20:39:43 +0200
- Subject: [PATCH] Fix ubsan handling of BIND_EXPR (PR sanitizer/71498)
- Authentication-results: sourceware.org; auth=none
- Reply-to: Jakub Jelinek <jakub at redhat dot com>
Hi!
As has been discussed in the original -fsanitize=bounds submission,
walk_tree for BIND_EXPR walks the body and
DECL_INITIAL/DECL_SIZE/DECL_SIZE_UNIT of all the BIND_EXPR_VARS.
For -fsanitize=bounds instrumentation, we want to avoid walking DECL_INITIAL
of TREE_STATIC vars, so should set *walk_subtrees to 0 and walk it all
ourselves. But, what the committed code actually does is that for
BIND_EXPRs that contain no TREE_STATIC vars, it walks
DECL_INITIAL/DECL_SIZE/DECL_SIZE_UNIT of all the BIND_EXPR_VARS, and then
walks subtrees normally, which means walking the body (good) and all the
DECL_INITIAL/DECL_SIZE/DECL_SIZE_UNIT exprs again (waste of time, we use
hash_set for duplicates, so just inefficiency).
But, if any TREE_STATIC vars appears, we set *walk_subtrees to 0 and
forget to walk the body (the primary bug).
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?
2016-06-13 Jakub Jelinek <jakub@redhat.com>
PR sanitizer/71498
* c-gimplify.c (ubsan_walk_array_refs_r): Set *walk_subtrees = 0 on
all BIND_EXPRs, and on all BIND_EXPRs recurse also on BIND_EXPR_BODY.
* c-c++-common/ubsan/bounds-13.c: New test.
--- gcc/c-family/c-gimplify.c.jj 2016-01-27 19:47:27.000000000 +0100
+++ gcc/c-family/c-gimplify.c 2016-06-13 13:27:06.531549561 +0200
@@ -67,23 +67,23 @@ ubsan_walk_array_refs_r (tree *tp, int *
{
hash_set<tree> *pset = (hash_set<tree> *) data;
- /* Since walk_tree doesn't call the callback function on the decls
- in BIND_EXPR_VARS, we have to walk them manually. */
if (TREE_CODE (*tp) == BIND_EXPR)
{
+ /* Since walk_tree doesn't call the callback function on the decls
+ in BIND_EXPR_VARS, we have to walk them manually, so we can avoid
+ instrumenting DECL_INITIAL of TREE_STATIC vars. */
+ *walk_subtrees = 0;
for (tree decl = BIND_EXPR_VARS (*tp); decl; decl = DECL_CHAIN (decl))
{
if (TREE_STATIC (decl))
- {
- *walk_subtrees = 0;
- continue;
- }
+ continue;
walk_tree (&DECL_INITIAL (decl), ubsan_walk_array_refs_r, pset,
pset);
walk_tree (&DECL_SIZE (decl), ubsan_walk_array_refs_r, pset, pset);
walk_tree (&DECL_SIZE_UNIT (decl), ubsan_walk_array_refs_r, pset,
pset);
}
+ walk_tree (&BIND_EXPR_BODY (*tp), ubsan_walk_array_refs_r, pset, pset);
}
else if (TREE_CODE (*tp) == ADDR_EXPR
&& TREE_CODE (TREE_OPERAND (*tp, 0)) == ARRAY_REF)
--- gcc/testsuite/c-c++-common/ubsan/bounds-13.c.jj 2016-06-13 13:36:25.698316271 +0200
+++ gcc/testsuite/c-c++-common/ubsan/bounds-13.c 2016-06-13 13:39:57.240586520 +0200
@@ -0,0 +1,31 @@
+/* PR sanitizer/71498 */
+/* { dg-do run } */
+/* { dg-options "-fsanitize=bounds -Wno-array-bounds" } */
+
+struct S { int a[100]; int b, c; } s;
+
+__attribute__((noinline, noclone)) int
+foo (int x)
+{
+ return s.a[x];
+}
+
+__attribute__((noinline, noclone)) int
+bar (int x)
+{
+ static int *d = &s.a[99];
+ asm volatile ("" : : "r" (&d));
+ return s.a[x];
+}
+
+int
+main ()
+{
+ volatile int a = 0;
+ a += foo (100);
+ a += bar (100);
+ return 0;
+}
+
+/* { dg-output "index 100 out of bounds for type 'int \\\[100\\\]'\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*index 100 out of bounds for type 'int \\\[100\\\]'\[^\n\r]*(\n|\r\n|\r)" } */
Jakub