On Wed, 25 May 2022, Jakub Jelinek wrote:
> Hi!
>
> On the following testcase with -Os asan pass sees:
> <bb 6> [local count: 354334800]:
> # h_21 = PHI <h_15(6), 0(5)>
> *c.3_5 = *d.2_4;
> h_15 = h_21 + 1;
> if (h_15 != 3)
> goto <bb 6>; [75.00%]
> else
> goto <bb 7>; [25.00%]
>
> <bb 7> [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 <ja...@redhat.com>
>
> 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 <gassign *> (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 <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)
