From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 266FC385609E for ; Wed, 25 May 2022 09:55:31 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 266FC385609E Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id CD07C1F381; Wed, 25 May 2022 09:55:29 +0000 (UTC) Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id C2A2D2C141; Wed, 25 May 2022 09:55:29 +0000 (UTC) Date: Wed, 25 May 2022 09:55:29 +0000 (UTC) From: Richard Biener To: Jakub Jelinek cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714] In-Reply-To: Message-ID: References: User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-4.9 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 May 2022 09:55:32 -0000 On Wed, 25 May 2022, Jakub Jelinek wrote: > Hi! > > On the following testcase with -Os asan pass sees: > [local count: 354334800]: > # h_21 = PHI > *c.3_5 = *d.2_4; > h_15 = h_21 + 1; > if (h_15 != 3) > goto ; [75.00%] > else > goto ; [25.00%] > > [local count: 118111600]: > *c.3_5 = MEM[(struct a *)&b + 12B]; > _13 = c.3_5->x; > return _13; > It instruments the > *c.3_5 = *d.2_4; > assignment by adding > .ASAN_CHECK (7, c.3_5, 4, 4); > .ASAN_CHECK (6, d.2_4, 4, 4); > before it (which later lowers to checking the corresponding shadow > memory). But when considering instrumentation of > *c.3_5 = MEM[(struct a *)&b + 12B]; > it doesn't instrument anything, because it sees that *c.3_5 store is > already instrumented in a dominating block and so there is no need > to instrument *c.3_5 store again (i.e. add another > .ASAN_CHECK (7, c.3_5, 4, 4); > ). That is true, but misses the fact that we still want to > instrument the MEM[(struct a *)&b + 12B] load. > > The following patch fixes that by changing has_stmt_been_instrumented_p > to consider both store and load in the assignment if it does both > (returning true iff both have been instrumented). > That matches how we handle e.g. builtin calls, where we also perform AND > of all the memory locs involved in the call. > > I've verified that we still don't add the redundant > .ASAN_CHECK (7, c.3_5, 4, 4); > call but just add > _18 = &MEM[(struct a *)&b + 12B]; > .ASAN_CHECK (6, _18, 4, 4); > to instrument the load. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? OK. Thanks, Richard. > 2022-05-25 Jakub Jelinek > > PR sanitizer/105714 > * asan.cc (has_stmt_been_instrumented_p): For assignments which > are both stores and loads, return true only if both destination > and source have been instrumented. > > * gcc.dg/asan/pr105714.c: New test. > > --- gcc/asan.cc.jj 2022-05-12 08:27:56.923834018 +0200 > +++ gcc/asan.cc 2022-05-24 11:39:28.527258357 +0200 > @@ -1285,7 +1285,20 @@ has_stmt_been_instrumented_p (gimple *st > > if (get_mem_ref_of_assignment (as_a (stmt), &r, > &r_is_store)) > - return has_mem_ref_been_instrumented (&r); > + { > + if (!has_mem_ref_been_instrumented (&r)) > + return false; > + if (r_is_store && gimple_assign_load_p (stmt)) > + { > + asan_mem_ref src; > + asan_mem_ref_init (&src, NULL, 1); > + src.start = gimple_assign_rhs1 (stmt); > + src.access_size = int_size_in_bytes (TREE_TYPE (src.start)); > + if (!has_mem_ref_been_instrumented (&src)) > + return false; > + } > + return true; > + } > } > else if (gimple_call_builtin_p (stmt, BUILT_IN_NORMAL)) > { > --- gcc/testsuite/gcc.dg/asan/pr105714.c.jj 2022-05-24 11:50:26.753570348 +0200 > +++ gcc/testsuite/gcc.dg/asan/pr105714.c 2022-05-24 11:51:01.074225766 +0200 > @@ -0,0 +1,33 @@ > +/* PR sanitizer/105714 */ > +/* { dg-do run } */ > +/* { dg-skip-if "" { *-*-* } { "*" } { "-Os" } } */ > +/* { dg-shouldfail "asan" } */ > + > +struct A { int x; }; > +struct A b[2]; > +struct A *c = b, *d = b; > +int e; > + > +int > +foo () > +{ > + for (e = 0; e < 1; e++) > + { > + int i[1]; > + i; > + } > + for (int h = 0; h < 3; h++) > + *c = *d; > + *c = *(b + 3); > + return c->x; > +} > + > +int > +main () > +{ > + foo (); > + return 0; > +} > + > +/* { dg-output "ERROR: AddressSanitizer: global-buffer-overflow on address.*(\n|\r\n|\r)" } */ > +/* { dg-output "READ of size.*" } */ > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)