* [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer.
@ 2016-04-08 8:52 Maxim Ostapenko
2016-04-08 9:12 ` Jakub Jelinek
0 siblings, 1 reply; 3+ messages in thread
From: Maxim Ostapenko @ 2016-04-08 8:52 UTC (permalink / raw)
To: GCC Patches; +Cc: Jakub Jelinek, Yury Gribov, Slava Garbuzov
[-- Attachment #1: Type: text/plain, Size: 703 bytes --]
Hi,
this patch actually fixes two issues:
* In some cases the dereferences in function argument list are not
instrumented and ASan can miss errors, e.g. heap-use-after-free. To
resolve this, we can just iterate through gimple_call's arguments and
add ASAN_CHECK statements for them if needed.
* As Jakub pointed out, if gimple_call was inlined, ASan catches an
error on invalid dereference, but fails to obtain its source location,
because it was missed in early inline pass. To resolve this, we can set
corresponding source location for inlined parameter in einline pass in
initialize_inlined_parameters routine.
Regtested and bootstrapped on x86_64-unknown-linux-gnu, OK to commit?
-Maxim
[-- Attachment #2: pr70541.diff --]
[-- Type: text/x-diff, Size: 5810 bytes --]
gcc/ChangeLog:
2016-04-08 Maxim Ostapenko <m.ostapenko@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.ostapenko@samsung.com>
PR sanitizer/70541
* c-c++-common/asan/pr70541-1.c: New test.
* c-c++-common/pr70541-2.c: Likewise.
diff --git a/gcc/asan.c b/gcc/asan.c
index 47bfdcd..14f97a2e 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,31 @@ 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);
+ gcc_assert (TREE_CODE (arg) != CALL_EXPR);
+ /* 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))
+ {
+ 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-1.c b/gcc/testsuite/c-c++-common/asan/pr70541-1.c
new file mode 100644
index 0000000..af853e7
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr70541-1.c
@@ -0,0 +1,33 @@
+/* { dg-do run } */
+/* { dg-options "-fno-builtin-malloc -fno-builtin-free" } */
+/* { 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-1.c:20|\[^\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-1.c:19|\[^\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-1.c:17|\[^\n\r]*:0)|\[(\])\[^\n\r]*(\n|\r\n|\r)" } */
diff --git a/gcc/testsuite/c-c++-common/pr70541-2.c b/gcc/testsuite/c-c++-common/pr70541-2.c
new file mode 100644
index 0000000..1fd6517
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/pr70541-2.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-options " -O2 -fdump-tree-einline-lineno" } */
+
+extern int printf (const char *format, ...);
+
+struct Simple
+{
+ int value;
+};
+
+int f (struct Simple simple)
+{
+ return simple.value;
+}
+
+int main ()
+{
+ struct Simple psimple;
+ struct Simple *psimple_ptr = &psimple;
+ printf ("%d\n", f (*psimple_ptr));
+ return 0;
+}
+
+/* { dg-final { scan-tree-dump "c:17:\[^\n\r\]*simple =" "einline" } } */
diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index 9d4f8f7..24bbd0a 100644
--- 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);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer.
2016-04-08 8:52 [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer Maxim Ostapenko
@ 2016-04-08 9:12 ` Jakub Jelinek
2016-04-08 10:51 ` Maxim Ostapenko
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Jelinek @ 2016-04-08 9:12 UTC (permalink / raw)
To: Maxim Ostapenko, Jan Hubicka, Richard Biener
Cc: GCC Patches, Yury Gribov, Slava Garbuzov
On Fri, Apr 08, 2016 at 11:52:32AM +0300, Maxim Ostapenko wrote:
> 2016-04-08 Maxim Ostapenko <m.ostapenko@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.ostapenko@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).
> --- 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.
Jakub
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer.
2016-04-08 9:12 ` Jakub Jelinek
@ 2016-04-08 10:51 ` Maxim Ostapenko
0 siblings, 0 replies; 3+ messages in thread
From: Maxim Ostapenko @ 2016-04-08 10:51 UTC (permalink / raw)
To: Jakub Jelinek, Jan Hubicka, Richard Biener
Cc: GCC Patches, Yury Gribov, Slava Garbuzov
[-- Attachment #1: Type: text/plain, Size: 3913 bytes --]
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.ostapenko@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.ostapenko@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
[-- Attachment #2: pr70541-2.diff --]
[-- Type: text/x-diff, Size: 3899 bytes --]
gcc/ChangeLog:
2016-04-08 Maxim Ostapenko <m.ostapenko@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.ostapenko@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)" } */
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-08 10:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-08 8:52 [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer Maxim Ostapenko
2016-04-08 9:12 ` Jakub Jelinek
2016-04-08 10:51 ` Maxim Ostapenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).