On 08/04/16 12:12, Jakub Jelinek wrote:
On Fri, Apr 08, 2016 at 11:52:32AM +0300, Maxim Ostapenko wrote:
2016-04-08 Maxim Ostapenko <m.ostape...@samsung.com>
PR sanitizer/70541
* asan.c (instrument_derefs): If we get unknown location, extract it
with EXPR_LOCATION.
(maybe_instrument_call): Instrument gimple_call's arguments if needed.
* tree-inline.c (initialize_inlined_parameters): Set location for
inlined parameters initialization statements.
gcc/testsuite/ChangeLog:
2016-04-08 Maxim Ostapenko <m.ostape...@samsung.com>
PR sanitizer/70541
* c-c++-common/asan/pr70541-1.c: New test.
* c-c++-common/pr70541-2.c: Likewise.
+ /* Walk through gimple_call arguments and check them id needed. */
+ unsigned args_num = gimple_call_num_args (stmt);
+ for (unsigned i = 0; i < args_num; ++i)
+ {
+ tree arg = gimple_call_arg (stmt, i);
+ gcc_assert (TREE_CODE (arg) != CALL_EXPR);
Please remove this assert, of course in GIMPLE arguments can't be
CALL_EXPRs, they must satisfy is_gimple_val or is_gimple_lvalue, but
that is verified in verify_gimple_call.
+ /* If ARG is not a non-aggregate register variable, compiler in general
+ creates temporary for it and pass it as argument to gimple call.
+ But in some cases, e.g. when we pass by value a small structure that
+ fits to register, compiler can avoid extra overhead by pulling out
+ these temporaries. In this case, we should check the argument. */
+ if (!is_gimple_reg (arg))
Actually, I believe this should be
if (!is_gimple_reg (arg) && !is_gimple_min_invariant (arg))
because constants, or addresses etc. also aren't memory references.
I know instrument_derefs exits early if the t isn't one of the selected
few trees, but still, it looks unclean to call instrument_derefs on
constants or addresses.
The asan.c changes look good to me with those changes, feel free to commit
them separately from the tree-inline.c change (perhaps with something in the
testcase XFAILed for now).
Ok, thanks, landed ASan part as r234827. I'm not sure how to XFAIL
output pattern tests only, so I enabled testcase only with -O0 for now.
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -3247,7 +3247,8 @@ initialize_inlined_parameters (copy_body_data *id, gimple
*stmt,
{
tree val;
val = i < gimple_call_num_args (stmt) ? gimple_call_arg (stmt, i) :
NULL;
- setup_one_parameter (id, p, val, fn, bb, &vars);
+ if (gimple *new_stmt = setup_one_parameter (id, p, val, fn, bb, &vars))
+ gimple_set_location (new_stmt, gimple_location (stmt));
}
/* After remapping parameters remap their types. This has to be done
in a second loop over all parameters to appropriately remap
@@ -3285,7 +3286,9 @@ initialize_inlined_parameters (copy_body_data *id, gimple
*stmt,
/* No static chain? Seems like a bug in tree-nested.c. */
gcc_assert (static_chain);
- setup_one_parameter (id, p, static_chain, fn, bb, &vars);
+ if (gimple *new_stmt = setup_one_parameter (id, p, static_chain, fn, bb,
+ &vars))
+ gimple_set_location (new_stmt, gimple_location (stmt));
}
declare_inline_vars (id->block, vars);
The tree-inline.c change looks much more risky at this point, could we defer
that for GCC 7? Unlike the asan change this is going to affect debugging,
etc.
Maybe e.g. pass gimple_location (stmt) as argument to setup_one_parameter
and set it in there instead? Sometimes there are multiple statements
created in there, not just one, etc. In any case, I'd prefer second pair of
eyes on the tree-inline.c changes, so I'm CCing Richi and Honza.
Perhaps we should move this issue to separate bug in Bugzilla?
Jakub
-Maxim
gcc/ChangeLog:
2016-04-08 Maxim Ostapenko <m.ostape...@samsung.com>
PR sanitizer/70541
* asan.c (instrument_derefs): If we get unknown location, extract it
with EXPR_LOCATION.
(maybe_instrument_call): Instrument gimple_call's arguments if needed.
gcc/testsuite/ChangeLog:
2016-04-08 Maxim Ostapenko <m.ostape...@samsung.com>
PR sanitizer/70541
* c-c++-common/asan/pr70541.c: New test.
diff --git a/gcc/asan.c b/gcc/asan.c
index 47bfdcd..71095fb 100644
--- a/gcc/asan.c
+++ b/gcc/asan.c
@@ -1766,6 +1766,8 @@ instrument_derefs (gimple_stmt_iterator *iter, tree t,
tree type, base;
HOST_WIDE_INT size_in_bytes;
+ if (location == UNKNOWN_LOCATION)
+ location = EXPR_LOCATION (t);
type = TREE_TYPE (t);
switch (TREE_CODE (t))
@@ -2049,6 +2051,7 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
gsi_insert_before (iter, g, GSI_SAME_STMT);
}
+ bool instrumented = false;
if (gimple_store_p (stmt))
{
tree ref_expr = gimple_call_lhs (stmt);
@@ -2056,11 +2059,30 @@ maybe_instrument_call (gimple_stmt_iterator *iter)
gimple_location (stmt),
/*is_store=*/true);
- gsi_next (iter);
- return true;
+ instrumented = true;
}
- return false;
+ /* Walk through gimple_call arguments and check them id needed. */
+ unsigned args_num = gimple_call_num_args (stmt);
+ for (unsigned i = 0; i < args_num; ++i)
+ {
+ tree arg = gimple_call_arg (stmt, i);
+ /* If ARG is not a non-aggregate register variable, compiler in general
+ creates temporary for it and pass it as argument to gimple call.
+ But in some cases, e.g. when we pass by value a small structure that
+ fits to register, compiler can avoid extra overhead by pulling out
+ these temporaries. In this case, we should check the argument. */
+ if (!is_gimple_reg (arg) && !is_gimple_min_invariant (arg))
+ {
+ instrument_derefs (iter, arg,
+ gimple_location (stmt),
+ /*is_store=*/false);
+ instrumented = true;
+ }
+ }
+ if (instrumented)
+ gsi_next (iter);
+ return instrumented;
}
/* Walk each instruction of all basic block and instrument those that
diff --git a/gcc/testsuite/c-c++-common/asan/pr70541.c b/gcc/testsuite/c-c++-common/asan/pr70541.c
new file mode 100644
index 0000000..b2a4bd5
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr70541.c
@@ -0,0 +1,34 @@
+/* { dg-do run } */
+/* { dg-options "-fno-builtin-malloc -fno-builtin-free" } */
+/* { dg-skip-if "" { *-*-* } { "*" } { "-O0" } } */
+/* { dg-shouldfail "asan" } */
+
+#include <stdio.h>
+#include <stdlib.h>
+
+struct Simple {
+ int value;
+};
+
+int f(struct Simple simple) {
+ return simple.value;
+}
+
+int main() {
+ struct Simple *psimple = (struct Simple *) malloc(sizeof(struct Simple));
+ psimple->value = 42;
+ free(psimple);
+ printf("%d\n", f(*psimple));
+ return 0;
+}
+
+/* { dg-output "ERROR: AddressSanitizer:? heap-use-after-free on address\[^\n\r]*" } */
+/* { dg-output "0x\[0-9a-f\]+ at pc 0x\[0-9a-f\]+ bp 0x\[0-9a-f\]+ sp 0x\[0-9a-f\]+\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*READ of size 4 at 0x\[0-9a-f\]+ thread T0\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output " #0 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*pr70541.c:21|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*freed by thread T0 here:\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output " #0 0x\[0-9a-f\]+ +(in _*(interceptor_|wrap_|)free|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output " #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*pr70541.c:20|\[^\n\r]*:0)|\[(\]).*(\n|\r\n|\r)" } */
+/* { dg-output "\[^\n\r]*previously allocated by thread T0 here:\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output " #0 0x\[0-9a-f\]+ +(in _*(interceptor_|wrap_|)malloc|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
+/* { dg-output " #1 0x\[0-9a-f\]+ +(in _*main (\[^\n\r]*pr70541.c:18|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
