Hello,
This PR uncovers an issue my latest optimization patch on Address
Sanitizer introduced.
This is in the context of instrumenting an access to a memory region,
like in:
void
foo (char *a, char *b, int s)
{
__builtin_memmove (a, b, s);
}
In this case, asan has to instrument accesses to two memory regions:
[a a+s[ and [b b+s[.
The way asan does instrument the access to e.g [a a+s[ his is that it
instruments the access to the byte pointed to by the pointer 'a', and
the access to the byte pointed to by the pointer 'a+s-1'.
Now what happens when we want to avoid doing redundant
instrumentations like in:
void
bar (char *a, char *b, char *c)
{
__builtin_memmove (a, b, c[b[0]]);
}
So here, we first instrument the access to the memory of b[0] (in the
expression c[b[0]]). So in the course of instrumenting the access for
the memory region [b b+s[, we don't want to instrument the access to
the memory pointed by 'p', as that was already been instrumented for
the b[0] expression. So we just instrument the access to memory
through b+s-1.
This is what I'd call 'partial memory region instrumentation'; it
happens in the function instrument_mem_region_access, and it was
basically corrupting the CFG because instrument_mem_region_access was
not correctly updating the statement iterator it receives in argument
so subsequent use of that iterator (to append instrumentation
statements) was leading to statements not being added to their correct
place. The compiler crash reported in this PR is due to this CFG
corruption.
There is another underlying issue that can lead to runtime errors.
When doing partial memory region instrumentation in e.g the case of
the bar function above I was forgetting to enclose the instrumentation
statements of [a, a+s[ and [b, b+s[ into a if (len != 0) {} clause
(len being c[b[0]] here) for the cases where len is not a constant.
The patch addresses this too.
Bootstrapped and tested on x86_64-unknown-linux-gnu against trunk, and
approved by Jakub in the audit trail of the PR at
http://gcc.gnu.org/PR56330. I am about to commit in a short while.
gcc/
* asan.c (get_mem_refs_of_builtin_call): White space and style
cleanup.
(instrument_mem_region_access): Do not forget to always put
instrumentation of the of 'base' and 'base + len' in a "if (len !=
0) statement, even for cases where either 'base' or 'base + len'
are not instrumented -- because they have been previously
instrumented. Simplify the logic by putting all the statements
instrument 'base + len' inside a sequence, and then insert that
sequence right before the current insertion point. Then, to
instrument 'base + len', just get an iterator on that statement.
And do not forget to update the pointer to iterator the function
received as argument.
gcc/testsuite/
* c-c++-common/asan/no-redundant-instrumentation-4.c: New test file.
* c-c++-common/asan/no-redundant-instrumentation-5.c: Likewise.
* c-c++-common/asan/no-redundant-instrumentation-6.c: Likewise.
* c-c++-common/asan/no-redundant-instrumentation-7.c: Likewise.
* c-c++-common/asan/no-redundant-instrumentation-8.c: Likewise.
* c-c++-common/asan/pr56330.c: Likewise.
* c-c++-common/asan/no-redundant-instrumentation-1.c (test1):
Ensure the size argument of __builtin_memcpy is a constant.
---
gcc/asan.c | 97 ++++++++++++----------
.../asan/no-redundant-instrumentation-1.c | 2 +-
.../asan/no-redundant-instrumentation-4.c | 13 +++
.../asan/no-redundant-instrumentation-5.c | 13 +++
.../asan/no-redundant-instrumentation-6.c | 14 ++++
.../asan/no-redundant-instrumentation-7.c | 22 +++++
.../asan/no-redundant-instrumentation-8.c | 14 ++++
gcc/testsuite/c-c++-common/asan/pr56330.c | 23 +++++
8 files changed, 153 insertions(+), 45 deletions(-)
create mode 100644
gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
create mode 100644
gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
create mode 100644
gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
create mode 100644
gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
create mode 100644
gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
create mode 100644 gcc/testsuite/c-c++-common/asan/pr56330.c
diff --git a/gcc/asan.c b/gcc/asan.c
index a569479..67236a9 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -747,20 +747,17 @@ get_mem_refs_of_builtin_call (const gimple call,
got_reference_p = true;
}
- else
- {
- if (dest)
- {
- dst->start = dest;
- dst->access_size = access_size;
- *dst_len = NULL_TREE;
- *dst_is_store = is_store;
- *dest_is_deref = true;
- got_reference_p = true;
- }
- }
+ else if (dest)
+ {
+ dst->start = dest;
+ dst->access_size = access_size;
+ *dst_len = NULL_TREE;
+ *dst_is_store = is_store;
+ *dest_is_deref = true;
+ got_reference_p = true;
+ }
- return got_reference_p;
+ return got_reference_p;
}
/* Return true iff a given gimple statement has been instrumented.
@@ -1535,8 +1532,15 @@ instrument_mem_region_access (tree base, tree len,
/* If the beginning of the memory region has already been
instrumented, do not instrument it. */
- if (has_mem_ref_been_instrumented (base, 1))
- goto after_first_instrumentation;
+ bool start_instrumented = has_mem_ref_been_instrumented (base, 1);
+
+ /* If the end of the memory region has already been instrumented, do
+ not instrument it. */
+ tree end = asan_mem_ref_get_end (base, len);
+ bool end_instrumented = has_mem_ref_been_instrumented (end, 1);
+
+ if (start_instrumented && end_instrumented)
+ return;
if (!is_gimple_constant (len))
{
@@ -1562,37 +1566,39 @@ instrument_mem_region_access (tree base, tree len,
/* The 'then block' of the 'if (len != 0) condition is where
we'll generate the asan instrumentation code now. */
- gsi = gsi_start_bb (then_bb);
+ gsi = gsi_last_bb (then_bb);
}
- /* Instrument the beginning of the memory region to be accessed,
- and arrange for the rest of the intrumentation code to be
- inserted in the then block *after* the current gsi. */
- build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1);
-
- if (then_bb)
- /* We are in the case where the length of the region is not
- constant; so instrumentation code is being generated in the
- 'then block' of the 'if (len != 0) condition. Let's arrange
- for the subsequent instrumentation statements to go in the
- 'then block'. */
- gsi = gsi_last_bb (then_bb);
- else
- *iter = gsi;
-
- update_mem_ref_hash_table (base, 1);
+ if (!start_instrumented)
+ {
+ /* Instrument the beginning of the memory region to be accessed,
+ and arrange for the rest of the intrumentation code to be
+ inserted in the then block *after* the current gsi. */
+ build_check_stmt (location, base, &gsi, /*before_p=*/true, is_store, 1);
+
+ if (then_bb)
+ /* We are in the case where the length of the region is not
+ constant; so instrumentation code is being generated in the
+ 'then block' of the 'if (len != 0) condition. Let's arrange
+ for the subsequent instrumentation statements to go in the
+ 'then block'. */
+ gsi = gsi_last_bb (then_bb);
+ else
+ {
+ *iter = gsi;
+ /* Don't remember this access as instrumented, if length
+ is unknown. It might be zero and not being actually
+ instrumented, so we can't rely on it being instrumented. */
+ update_mem_ref_hash_table (base, 1);
+ }
+ }
- after_first_instrumentation:
+ if (end_instrumented)
+ return;
/* We want to instrument the access at the end of the memory region,
which is at (base + len - 1). */
- /* If the end of the memory region has already been instrumented, do
- not instrument it. */
- tree end = asan_mem_ref_get_end (base, len);
- if (has_mem_ref_been_instrumented (end, 1))
- return;
-
/* offset = len - 1; */
len = unshare_expr (len);
tree offset;
@@ -1639,8 +1645,6 @@ instrument_mem_region_access (tree base, tree len,
base, NULL);
gimple_set_location (region_end, location);
gimple_seq_add_stmt_without_update (&seq, region_end);
- gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
- gsi_prev (&gsi);
/* _2 = _1 + offset; */
region_end =
@@ -1649,13 +1653,18 @@ instrument_mem_region_access (tree base, tree len,
gimple_assign_lhs (region_end),
offset);
gimple_set_location (region_end, location);
- gsi_insert_after (&gsi, region_end, GSI_NEW_STMT);
+ gimple_seq_add_stmt_without_update (&seq, region_end);
+ gsi_insert_seq_before (&gsi, seq, GSI_SAME_STMT);
/* instrument access at _2; */
+ gsi = gsi_for_stmt (region_end);
build_check_stmt (location, gimple_assign_lhs (region_end),
&gsi, /*before_p=*/false, is_store, 1);
- update_mem_ref_hash_table (end, 1);
+ if (then_bb == NULL)
+ update_mem_ref_hash_table (end, 1);
+
+ *iter = gsi_for_stmt (gsi_stmt (*iter));
}
/* Instrument the call (to the builtin strlen function) pointed to by
@@ -1783,7 +1792,7 @@ instrument_builtin_call (gimple_stmt_iterator *iter)
}
else if (src0_len || src1_len || dest_len)
{
- if (src0.start)
+ if (src0.start != NULL_TREE)
instrument_mem_region_access (src0.start, src0_len,
iter, loc, /*is_store=*/false);
if (src1.start != NULL_TREE)
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
index cc98fdb..6cf6441 100644
--- a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-1.c
@@ -45,7 +45,7 @@ test1 ()
/* There are 2 calls to __builtin___asan_report_store1 and 2 calls
to __builtin___asan_report_load1 to instrument the store to
(subset of) the memory region of tab. */
- __builtin_memcpy (&tab[1], foo, sizeof (tab) - 1);
+ __builtin_memcpy (&tab[1], foo, 3);
/* This should not generate a __builtin___asan_report_load1 because
the reference to tab[1] has been already instrumented above. */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
new file mode 100644
index 0000000..b2e7284
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-4.c
@@ -0,0 +1,13 @@
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+void
+foo (int *a, char *b, char *c)
+{
+ __builtin_memmove (c, b, a[c[0]]);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0"
} } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 1
"asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
new file mode 100644
index 0000000..ead3f58
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-5.c
@@ -0,0 +1,13 @@
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+void
+foo (int *a, char *b, char *c)
+{
+ __builtin_memmove (c, b, a[b[0]]);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 2 "asan0"
} } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2
"asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
new file mode 100644
index 0000000..e4691bc
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-6.c
@@ -0,0 +1,14 @@
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+void
+foo (int *a, char *b, char *c)
+{
+ __builtin_memmove (c, b, a[c[0]]);
+ __builtin_memmove (c, b, a[b[0]]);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 5 "asan0"
} } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2
"asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
new file mode 100644
index 0000000..aba409b
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-7.c
@@ -0,0 +1,22 @@
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+char e[200];
+
+struct S
+{
+ char a[100];
+ char b[100];
+} s;
+
+void
+foo (int *a, char *b, char *c)
+{
+ __builtin_memcmp (s.a, e, 100);
+ __builtin_memcmp (s.a, e, 200);
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load" 6 "asan0"
} } */
+/* { dg-final { scan-tree-dump-not "__builtin___asan_report_store" "asan0" } }
*/
+/* { dg-final { cleanup-tree-dump "asan" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
new file mode 100644
index 0000000..38ea7a2
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/no-redundant-instrumentation-8.c
@@ -0,0 +1,14 @@
+/* { dg-options "-fdump-tree-asan0" } */
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+
+char
+foo (int *a, char *b, char *c)
+{
+ __builtin_memmove (c, b, a[b[0]]);
+ return c[0] + b[0];
+}
+
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_load1" 3 "asan0"
} } */
+/* { dg-final { scan-tree-dump-times "__builtin___asan_report_store1" 2
"asan0" } } */
+/* { dg-final { cleanup-tree-dump "asan0" } } */
diff --git a/gcc/testsuite/c-c++-common/asan/pr56330.c
b/gcc/testsuite/c-c++-common/asan/pr56330.c
new file mode 100644
index 0000000..18f1065
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr56330.c
@@ -0,0 +1,23 @@
+/* PR sanitizer/56330 */
+/* { dg-do compile } */
+
+char e[200];
+
+struct S
+{
+ char a[100];
+ char b[100];
+} s;
+
+void
+foo (void)
+{
+ __builtin_memcmp (s.a, e, 100);
+ __builtin_memcmp (s.a, e, 200);
+}
+
+void
+bar (int *a, char *b, char *c)
+{
+ __builtin_memmove (c, b, a[b[0]]);
+}
--
Dodji
