public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714]
@ 2022-05-25  9:32 Jakub Jelinek
  2022-05-25  9:55 ` Richard Biener
  0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2022-05-25  9:32 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches

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?

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714]
  2022-05-25  9:32 [PATCH] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714] Jakub Jelinek
@ 2022-05-25  9:55 ` Richard Biener
  0 siblings, 0 replies; 2+ messages in thread
From: Richard Biener @ 2022-05-25  9:55 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: gcc-patches

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)

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2022-05-25  9:55 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-25  9:32 [PATCH] asan: Fix up instrumentation of assignments which are both loads and stores [PR105714] Jakub Jelinek
2022-05-25  9:55 ` Richard Biener

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).