From patchwork Fri Feb 15 20:54:19 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 220848 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id C52392C0082 for ; Sat, 16 Feb 2013 07:54:54 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1361566496; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:From:To:Cc:Subject:Date:Message-ID:User-Agent: MIME-Version:Content-Type:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=dYqn6WJ8fdn/mvj5/+ktI4m3uCQ=; b=EMsTq7x9+BUtyY0 Ev0PYtDkDueLdT5qD1hNjLT5IajNLC1BdkqO/1v6W4W0e6XRs0FS+uzpW6AXBYDT Bm45Rh9ARe8f1Z1LEknG2pOrH0ScexeSzt0HoFFfWXSvWpxuaV6zfeAOCXQlVECr lhipVqrenSedRDHmwUurRFVckSX4= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Received:From:To:Cc:Subject:X-URL:Date:Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=Qo6wD1IMxnBS5yiXwOpp5hOICd6WJnNCUpfV+l3J1g8LEhzJ2qVbSuaDMI6zRi 1o2A2oYFj50RNU7sAvi5VZsKU9emUqIDI32c5JEgPEL8adu5GVj7NqGNf4/FnabO gOiyVtJIh6MYJq36rve2w7n7VvPZ4LSHeFdGvmgfUgRf0=; Received: (qmail 26169 invoked by alias); 15 Feb 2013 20:54:46 -0000 Received: (qmail 26157 invoked by uid 22791); 15 Feb 2013 20:54:45 -0000 X-SWARE-Spam-Status: No, hits=-5.9 required=5.0 tests=AWL, BAYES_00, KHOP_RCVD_UNTRUST, KHOP_SPAMHAUS_DROP, RCVD_IN_DNSWL_HI, RCVD_IN_HOSTKARMA_W, RP_MATCHES_RCVD, SPF_HELO_PASS, TW_TM X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 15 Feb 2013 20:54:25 +0000 Received: from int-mx02.intmail.prod.int.phx2.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id r1FKsNxv028028 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Fri, 15 Feb 2013 15:54:23 -0500 Received: from localhost (ovpn-116-33.ams2.redhat.com [10.36.116.33]) by int-mx02.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id r1FKsKNI031535 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 15 Feb 2013 15:54:23 -0500 Received: by localhost (Postfix, from userid 1000) id CCF034064A; Fri, 15 Feb 2013 21:54:19 +0100 (CET) From: Dodji Seketeli To: Konstantin Serebryany , Dmitry Vyukov , Jakub Jelinek Cc: GCC Patches Subject: [PATCH] [asan] Fix for PR asan/56330 X-URL: http://www.redhat.com Date: Fri, 15 Feb 2013 21:54:19 +0100 Message-ID: <874nhdb8n8.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org 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]]); +}