diff mbox series

[PING,sanitizer/106558] asan: fix unsafe optimization of Asan checks.

Message ID CAJOtW+5=-WA_i7cXxrWSOVKDc_PQbtNOoaLmEQJrk3oU=uLUdw@mail.gmail.com
State Accepted
Headers
Series [PING,sanitizer/106558] asan: fix unsafe optimization of Asan checks. |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Yuri Gribov Nov. 21, 2022, 9:57 a.m. UTC 
  Hi,

This patch fixes incorrect Asan optimization in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106558 . It successfully
passes bootstrap-asan, regular bootstrap and regression testing (on
x86/amd64).

With this patch number of optimizations has reduced only slightly
(146062 -> 145824 on bootstrap-asan) so I decided to skip the more
complicated alias oracle-based approach that was suggested by Jakub in
the PR.

Best regards,
Yuri

Comments

Jakub Jelinek Nov. 21, 2022, 10:02 a.m. UTC | #1 
On Mon, Nov 21, 2022 at 12:57:15PM +0300, Yuri Gribov wrote:
> From 4729f2db3f1b6b40ef0124e4a645788d7f66f426 Mon Sep 17 00:00:00 2001
> From: Yuri Gribov <y.gribov@samsung.com>
> Date: Sun, 14 Aug 2022 08:42:44 +0300
> Subject: [PATCH] asan: fix unsafe optimization of Asan checks.
> 
> gcc/
>         PR sanitizer/106558
>         * sanopt.c: Do not optimize out checks for non-SSA addresses.
> 
> gcc/testsuite/
>         PR sanitizer/106558
>         * c-c++-common/asan/pr106558.c: New test.
> ---
>  gcc/sanopt.cc                              | 40 +++++++++++++++++-----
>  gcc/testsuite/c-c++-common/asan/pr106558.c | 23 +++++++++++++
>  2 files changed, 54 insertions(+), 9 deletions(-)
>  create mode 100644 gcc/testsuite/c-c++-common/asan/pr106558.c
> 
> diff --git a/gcc/sanopt.cc b/gcc/sanopt.cc
> index e9d188d7889..13942a0b1da 100644
> --- a/gcc/sanopt.cc
> +++ b/gcc/sanopt.cc
> @@ -80,16 +80,16 @@ struct sanopt_info
>  
>  /* If T has a single definition of form T = T2, return T2.  */
>  
> -static tree
> +static gimple *
>  maybe_get_single_definition (tree t)
>  {
>    if (TREE_CODE (t) == SSA_NAME)
>      {
>        gimple *g = SSA_NAME_DEF_STMT (t);
>        if (gimple_assign_single_p (g))
> -	return gimple_assign_rhs1 (g);
> +	return g;
>      }
> -  return NULL_TREE;
> +  return NULL;
>  }
>  
>  /* Tree triplet for vptr_check_map.  */
> @@ -618,11 +618,30 @@ maybe_optimize_ubsan_vptr_ifn (class sanopt_ctx *ctx, gimple *stmt)
>    return true;
>  }
>  
> +/* Checks whether value of T in CHECK and USE is the same.  */
> +
> +static bool same_value_p (gimple *check, gimple *use, tree t)

Formatting.  Function name should be on another line:
static bool
same_value_p (gimple *check, gimple *use, tree t)

Otherwise LGTM.  Thanks and sorry for the review delay.

	Jakub
Martin Liška Nov. 25, 2022, 4:18 p.m. UTC | #2 
On 11/21/22 11:02, Jakub Jelinek wrote:
> Otherwise LGTM.  Thanks and sorry for the review delay.

Yuri, do you want to commit the patch soon?

If not, I can help if you want?

Cheers,
Martin
Martin Liška Nov. 28, 2022, 9:50 a.m. UTC | #3 
On 11/25/22 17:18, Martin Liška wrote:
> On 11/21/22 11:02, Jakub Jelinek wrote:
>> Otherwise LGTM.  Thanks and sorry for the review delay.
> 
> Yuri, do you want to commit the patch soon?
> 
> If not, I can help if you want?

Hey.

I've just installed the patch with function signature change
and changelog tweak. I'm testing multiple ASAN bugs and I need
this patch as it fixes quite something.

Thanks,
Martin

> 
> Cheers,
> Martin
diff mbox series

Patch

From 4729f2db3f1b6b40ef0124e4a645788d7f66f426 Mon Sep 17 00:00:00 2001
From: Yuri Gribov <y.gribov@samsung.com>
Date: Sun, 14 Aug 2022 08:42:44 +0300
Subject: [PATCH] asan: fix unsafe optimization of Asan checks.

gcc/
        PR sanitizer/106558
        * sanopt.c: Do not optimize out checks for non-SSA addresses.

gcc/testsuite/
        PR sanitizer/106558
        * c-c++-common/asan/pr106558.c: New test.
---
 gcc/sanopt.cc                              | 40 +++++++++++++++++-----
 gcc/testsuite/c-c++-common/asan/pr106558.c | 23 +++++++++++++
 2 files changed, 54 insertions(+), 9 deletions(-)
 create mode 100644 gcc/testsuite/c-c++-common/asan/pr106558.c

diff --git a/gcc/sanopt.cc b/gcc/sanopt.cc
index e9d188d7889..13942a0b1da 100644
--- a/gcc/sanopt.cc
+++ b/gcc/sanopt.cc
@@ -80,16 +80,16 @@  struct sanopt_info
 
 /* If T has a single definition of form T = T2, return T2.  */
 
-static tree
+static gimple *
 maybe_get_single_definition (tree t)
 {
   if (TREE_CODE (t) == SSA_NAME)
     {
       gimple *g = SSA_NAME_DEF_STMT (t);
       if (gimple_assign_single_p (g))
-	return gimple_assign_rhs1 (g);
+	return g;
     }
-  return NULL_TREE;
+  return NULL;
 }
 
 /* Tree triplet for vptr_check_map.  */
@@ -618,11 +618,30 @@  maybe_optimize_ubsan_vptr_ifn (class sanopt_ctx *ctx, gimple *stmt)
   return true;
 }
 
+/* Checks whether value of T in CHECK and USE is the same.  */
+
+static bool same_value_p (gimple *check, gimple *use, tree t)
+{
+  tree check_vuse = gimple_vuse (check);
+  tree use_vuse = gimple_vuse (use);
+
+  if (TREE_CODE (t) == SSA_NAME
+      || is_gimple_min_invariant (t)
+      || ! use_vuse)
+    return true;
+
+  if (check_vuse == use_vuse)
+    return true;
+
+  return false;
+}
+
 /* Returns TRUE if ASan check of length LEN in block BB can be removed
    if preceded by checks in V.  */
 
 static bool
-can_remove_asan_check (auto_vec<gimple *> &v, tree len, basic_block bb)
+can_remove_asan_check (auto_vec<gimple *> &v, tree len, basic_block bb,
+		       gimple *base_stmt, tree base_addr)
 {
   unsigned int i;
   gimple *g;
@@ -674,8 +693,10 @@  can_remove_asan_check (auto_vec<gimple *> &v, tree len, basic_block bb)
 
 	  last_bb = imm;
 	}
-      if (last_bb == gbb)
-	remove = true;
+      if (last_bb != gbb)
+	break;
+      // In case of base_addr residing in memory we also need to check aliasing
+      remove = ! base_addr || same_value_p (g, base_stmt, base_addr);
       break;
     }
 
@@ -718,7 +739,8 @@  maybe_optimize_asan_check_ifn (class sanopt_ctx *ctx, gimple *stmt)
 
   auto_vec<gimple *> *ptr_checks = &ctx->asan_check_map.get_or_insert (ptr);
 
-  tree base_addr = maybe_get_single_definition (ptr);
+  gimple *base_stmt = maybe_get_single_definition (ptr);
+  tree base_addr = base_stmt ? gimple_assign_rhs1 (base_stmt) : NULL_TREE;
   auto_vec<gimple *> *base_checks = NULL;
   if (base_addr)
     {
@@ -747,11 +769,11 @@  maybe_optimize_asan_check_ifn (class sanopt_ctx *ctx, gimple *stmt)
   bool remove = false;
 
   if (ptr_checks)
-    remove = can_remove_asan_check (*ptr_checks, len, bb);
+    remove = can_remove_asan_check (*ptr_checks, len, bb, NULL, NULL);
 
   if (!remove && base_checks)
     /* Try with base address as well.  */
-    remove = can_remove_asan_check (*base_checks, len, bb);
+    remove = can_remove_asan_check (*base_checks, len, bb, base_stmt, base_addr);
 
   if (!remove)
     {
diff --git a/gcc/testsuite/c-c++-common/asan/pr106558.c b/gcc/testsuite/c-c++-common/asan/pr106558.c
new file mode 100644
index 00000000000..d82b2dc7a83
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/asan/pr106558.c
@@ -0,0 +1,23 @@ 
+/* { dg-do run } */
+/* { dg-options "-w -fpermissive" } */
+/* { dg-shouldfail "asan" } */
+
+int a;
+int *b = &a;
+int **c = &b;
+int d[1];
+int *e = &d[1];
+
+static int f(int *g) {
+  *b = e;
+  *c = e;
+  *b = 2;
+  *g = 2;
+}
+
+int main() {
+    f(b);
+    return *b;
+}
+
+/* { dg-output "AddressSanitizer: global-buffer-overflow on address" } */
-- 
2.17.1