Re: [PATCH] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714]

[Richard Biener <rguenther@suse.de>]

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  <jakub@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 <rguenther@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)