From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 38164 invoked by alias); 8 Apr 2016 08:52:52 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 38097 invoked by uid 89); 8 Apr 2016 08:52:50 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS,T_MANY_HDRS_LCASE autolearn=ham version=3.3.2 spammy=fits, HContent-type:multipart, HContent-type:boundary, Hx-spam-relays-external:ESMTPA X-HELO: mailout2.w1.samsung.com Received: from mailout2.w1.samsung.com (HELO mailout2.w1.samsung.com) (210.118.77.12) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 08 Apr 2016 08:52:40 +0000 Received: from eucpsbgm1.samsung.com (unknown [203.254.199.244]) by mailout2.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0O5B00AEU4NOY960@mailout2.w1.samsung.com> for gcc-patches@gcc.gnu.org; Fri, 08 Apr 2016 09:52:36 +0100 (BST) Received: from eusync3.samsung.com ( [203.254.199.213]) by eucpsbgm1.samsung.com (EUCPMTA) with SMTP id 57.A0.05254.35177075; Fri, 8 Apr 2016 09:52:35 +0100 (BST) Received: from [106.109.128.194] by eusync3.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0O5B0057J4NM8F30@eusync3.samsung.com>; Fri, 08 Apr 2016 09:52:35 +0100 (BST) From: Maxim Ostapenko Subject: [PATCH][PR sanitizer/70541] Fix unnoticed invalid dereference when using AddressSanitizer. To: GCC Patches Cc: Jakub Jelinek , Yury Gribov , Slava Garbuzov Message-id: <57077150.7030905@samsung.com> Date: Fri, 08 Apr 2016 08:52:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-version: 1.0 Content-type: multipart/mixed; boundary=------------020608030803020805040809 X-IsSubscribed: yes X-SW-Source: 2016-04/txt/msg00380.txt.bz2 This is a multi-part message in MIME format. --------------020608030803020805040809 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Content-length: 703 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 --------------020608030803020805040809 Content-Type: text/x-diff; name="pr70541.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="pr70541.diff" Content-length: 5810 gcc/ChangeLog: 2016-04-08 Maxim Ostapenko 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 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 +#include + +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); --------------020608030803020805040809--