[PATCH RFA] asan: poisoning promoted statics [PR113531]

[PATCH RFA] asan: poisoning promoted statics [PR113531]

[Jason Merrill]

Tested x86_64-pc-linux-gnu, OK for trunk?

-- 8< --

Since my r14-1500-g4d935f52b0d5c0 we promote an initializer_list backing
array to static storage where appropriate, but this happens after we decided
to add it to asan_poisoned_variables.  As a result we add unpoison/poison
for it to the gimple.  But then sanopt removes the unpoison.  So the second
time we call the function and want to load from the array asan still
considers it poisoned.

A simple fix seems to be to not expand unpoison/poison for such a variable,
since by that time we know it's static.

	PR c++/113531

gcc/ChangeLog:

	* asan.cc (asan_expand_mark_ifn): Check TREE_STATIC.

gcc/testsuite/ChangeLog:

	* g++.dg/asan/initlist1.C: New test.
---
 gcc/asan.cc                           |  8 ++++++++
 gcc/testsuite/g++.dg/asan/initlist1.C | 20 ++++++++++++++++++++
 2 files changed, 28 insertions(+)
 create mode 100644 gcc/testsuite/g++.dg/asan/initlist1.C

diff --git a/gcc/asan.cc b/gcc/asan.cc
index 0fd7dd1f3ed..efecac2ea2b 100644
--- a/gcc/asan.cc
+++ b/gcc/asan.cc
@@ -3762,6 +3762,14 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
 
   gcc_checking_assert (TREE_CODE (decl) == VAR_DECL);
 
+  if (TREE_STATIC (decl))
+    {
+      /* Don't poison a variable with static storage; it might have gotten
+	 marked before gimplify_init_constructor promoted it to static.  */
+      gsi_remove (iter, true);
+      return false;
+    }
+
   if (hwasan_sanitize_p ())
     {
       gcc_assert (param_hwasan_instrument_stack);
diff --git a/gcc/testsuite/g++.dg/asan/initlist1.C b/gcc/testsuite/g++.dg/asan/initlist1.C
new file mode 100644
index 00000000000..6cd5b7d3aba
--- /dev/null
+++ b/gcc/testsuite/g++.dg/asan/initlist1.C
@@ -0,0 +1,20 @@
+// PR c++/113531
+// { dg-do run { target c++11 } }
+// { dg-additional-options "-fsanitize=address" }
+
+#include <initializer_list>
+
+void f(int) { }
+
+void g()
+{
+  for (auto i : { 1, 2, 3 })
+    f (i);
+  f(42);
+}
+
+int main()
+{
+  g();
+  g();
+}

base-commit: 209fc1e5f6c67e55e579b69f617b0b678b1bfdf0
-- 
2.39.3

Re: [PATCH RFA] asan: poisoning promoted statics [PR113531]

[Richard Biener]

On Wed, Jan 31, 2024 at 4:38 AM Jason Merrill <jason@redhat.com> wrote:
>
> Tested x86_64-pc-linux-gnu, OK for trunk?

It's a quite "late" fixup, I suppose you have tried to avoid marking it
during gimplification?  I see we do parts of this during BIND_EXPR
processing which is indeed a bit early but possibly difficult to rectify.

So, OK if you think fixing during gimplification is overly messy.

Richard.

> -- 8< --
>
> Since my r14-1500-g4d935f52b0d5c0 we promote an initializer_list backing
> array to static storage where appropriate, but this happens after we decided
> to add it to asan_poisoned_variables.  As a result we add unpoison/poison
> for it to the gimple.  But then sanopt removes the unpoison.  So the second
> time we call the function and want to load from the array asan still
> considers it poisoned.
>
> A simple fix seems to be to not expand unpoison/poison for such a variable,
> since by that time we know it's static.
>
>         PR c++/113531
>
> gcc/ChangeLog:
>
>         * asan.cc (asan_expand_mark_ifn): Check TREE_STATIC.
>
> gcc/testsuite/ChangeLog:
>
>         * g++.dg/asan/initlist1.C: New test.
> ---
>  gcc/asan.cc                           |  8 ++++++++
>  gcc/testsuite/g++.dg/asan/initlist1.C | 20 ++++++++++++++++++++
>  2 files changed, 28 insertions(+)
>  create mode 100644 gcc/testsuite/g++.dg/asan/initlist1.C
>
> diff --git a/gcc/asan.cc b/gcc/asan.cc
> index 0fd7dd1f3ed..efecac2ea2b 100644
> --- a/gcc/asan.cc
> +++ b/gcc/asan.cc
> @@ -3762,6 +3762,14 @@ asan_expand_mark_ifn (gimple_stmt_iterator *iter)
>
>    gcc_checking_assert (TREE_CODE (decl) == VAR_DECL);
>
> +  if (TREE_STATIC (decl))
> +    {
> +      /* Don't poison a variable with static storage; it might have gotten
> +        marked before gimplify_init_constructor promoted it to static.  */
> +      gsi_remove (iter, true);
> +      return false;
> +    }
> +
>    if (hwasan_sanitize_p ())
>      {
>        gcc_assert (param_hwasan_instrument_stack);
> diff --git a/gcc/testsuite/g++.dg/asan/initlist1.C b/gcc/testsuite/g++.dg/asan/initlist1.C
> new file mode 100644
> index 00000000000..6cd5b7d3aba
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/asan/initlist1.C
> @@ -0,0 +1,20 @@
> +// PR c++/113531
> +// { dg-do run { target c++11 } }
> +// { dg-additional-options "-fsanitize=address" }
> +
> +#include <initializer_list>
> +
> +void f(int) { }
> +
> +void g()
> +{
> +  for (auto i : { 1, 2, 3 })
> +    f (i);
> +  f(42);
> +}
> +
> +int main()
> +{
> +  g();
> +  g();
> +}
>
> base-commit: 209fc1e5f6c67e55e579b69f617b0b678b1bfdf0
> --
> 2.39.3
>

Re: [PATCH RFA] asan: poisoning promoted statics [PR113531]

[Jakub Jelinek]

On Wed, Jan 31, 2024 at 09:51:05AM +0100, Richard Biener wrote:
> On Wed, Jan 31, 2024 at 4:38 AM Jason Merrill <jason@redhat.com> wrote:
> >
> > Tested x86_64-pc-linux-gnu, OK for trunk?
> 
> It's a quite "late" fixup, I suppose you have tried to avoid marking it
> during gimplification?  I see we do parts of this during BIND_EXPR
> processing which is indeed a bit early but possibly difficult to rectify.

Indeed.  But what we could do is try to fold_stmt those .ASAN_MARK calls
away earlier (but sure, the asan.cc change would be still required because
that would be just an optimization).  But that can be handled incrementally,
so I think the patch is ok as is (and I can handle the incremental part
myself).

Note, the handling of global vars in asan is done only at the end
(asan_finish_file), so I think such late TREE_STATIC marked vars will still
be correctly treated as global vars if varpool knows about them (and if
varpool doesn't, then lots of other things would break).

	Jakub

[PATCH] gimple-fold: Remove .ASAN_MARK calls on TREE_STATIC variables [PR113531]

[Jakub Jelinek]

On Wed, Jan 31, 2024 at 10:07:28AM +0100, Jakub Jelinek wrote:
> Indeed.  But what we could do is try to fold_stmt those .ASAN_MARK calls
> away earlier (but sure, the asan.cc change would be still required because
> that would be just an optimization).  But that can be handled incrementally,
> so I think the patch is ok as is (and I can handle the incremental part
> myself).

Like this, so far just tested on the testcase.  Ok for trunk if it passes
bootstrap/regtest on top of Jason's patch?

2024-01-31  Jakub Jelinek  <jakub@redhat.com>

	PR c++/113531
	* gimple-fold.cc (gimple_fold_call): Remove .ASAN_MARK calls
	on variables which were promoted to TREE_STATIC.

--- gcc/gimple-fold.cc.jj	2024-01-03 11:51:27.236790799 +0100
+++ gcc/gimple-fold.cc	2024-01-31 12:09:14.853348505 +0100
@@ -5722,6 +5722,21 @@ gimple_fold_call (gimple_stmt_iterator *
 	      }
 	  }
 	  break;
+        case IFN_ASAN_MARK:
+          {
+            tree base = gimple_call_arg (stmt, 1);
+            gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
+            tree decl = TREE_OPERAND (base, 0);
+            if (VAR_P (decl) && TREE_STATIC (decl))
+	      {
+	        /* Don't poison a variable with static storage; it might have
+		   gotten marked before gimplify_init_constructor promoted it
+		   to static.  */
+		replace_call_with_value (gsi, NULL_TREE);
+		return true;
+	      }
+          }
+	  break;
 	case IFN_GOACC_DIM_SIZE:
 	case IFN_GOACC_DIM_POS:
 	  result = fold_internal_goacc_dim (stmt);


	Jakub

Re: [PATCH] gimple-fold: Remove .ASAN_MARK calls on TREE_STATIC variables [PR113531]

[Richard Biener]

On Wed, Jan 31, 2024 at 12:18 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Wed, Jan 31, 2024 at 10:07:28AM +0100, Jakub Jelinek wrote:
> > Indeed.  But what we could do is try to fold_stmt those .ASAN_MARK calls
> > away earlier (but sure, the asan.cc change would be still required because
> > that would be just an optimization).  But that can be handled incrementally,
> > so I think the patch is ok as is (and I can handle the incremental part
> > myself).
>
> Like this, so far just tested on the testcase.  Ok for trunk if it passes
> bootstrap/regtest on top of Jason's patch?

Note we fold all - well, all builtin - calls during gimple lowering.
Maybe we can put this special-casing there instead? (gimple-low.cc:797,
you possibly have to replace with a GIMPLE_NOP)

> 2024-01-31  Jakub Jelinek  <jakub@redhat.com>
>
>         PR c++/113531
>         * gimple-fold.cc (gimple_fold_call): Remove .ASAN_MARK calls
>         on variables which were promoted to TREE_STATIC.
>
> --- gcc/gimple-fold.cc.jj       2024-01-03 11:51:27.236790799 +0100
> +++ gcc/gimple-fold.cc  2024-01-31 12:09:14.853348505 +0100
> @@ -5722,6 +5722,21 @@ gimple_fold_call (gimple_stmt_iterator *
>               }
>           }
>           break;
> +        case IFN_ASAN_MARK:
> +          {
> +            tree base = gimple_call_arg (stmt, 1);
> +            gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
> +            tree decl = TREE_OPERAND (base, 0);
> +            if (VAR_P (decl) && TREE_STATIC (decl))
> +             {
> +               /* Don't poison a variable with static storage; it might have
> +                  gotten marked before gimplify_init_constructor promoted it
> +                  to static.  */
> +               replace_call_with_value (gsi, NULL_TREE);
> +               return true;
> +             }
> +          }
> +         break;
>         case IFN_GOACC_DIM_SIZE:
>         case IFN_GOACC_DIM_POS:
>           result = fold_internal_goacc_dim (stmt);
>
>
>         Jakub
>

[PATCH] gimple-low: Remove .ASAN_MARK calls on TREE_STATIC variables [PR113531]

[Jakub Jelinek]

On Wed, Jan 31, 2024 at 01:04:22PM +0100, Richard Biener wrote:
> > Like this, so far just tested on the testcase.  Ok for trunk if it passes
> > bootstrap/regtest on top of Jason's patch?
> 
> Note we fold all - well, all builtin - calls during gimple lowering.

Internal calls aren't builtin, so those aren't folded.

> Maybe we can put this special-casing there instead? (gimple-low.cc:797,
> you possibly have to replace with a GIMPLE_NOP)

But sure, we can do it there as well.
In that case I think we don't need it in asan.cc though, because
gimple-low.cc is guaranteed to be performed on all statements from
gimplification that survive in the IL.

So like this?

2024-01-31  Jakub Jelinek  <jakub@redhat.com>
	    Jason Merrill  <jason@redhat.com>

	PR c++/113531
	* gimple-low.cc (lower_stmt): Remove .ASAN_MARK calls
	on variables which were promoted to TREE_STATIC.

	* g++.dg/asan/initlist1.C: New test.

--- gcc/gimple-low.cc.jj	2024-01-03 11:51:22.133861623 +0100
+++ gcc/gimple-low.cc	2024-01-31 16:16:21.326927572 +0100
@@ -790,6 +790,21 @@ lower_stmt (gimple_stmt_iterator *gsi, s
 	    return;
 	  }
 
+	if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
+	  {
+	    tree base = gimple_call_arg (stmt, 1);
+	    gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
+	    tree decl = TREE_OPERAND (base, 0);
+	    if (VAR_P (decl) && TREE_STATIC (decl))
+	      {
+		/* Don't poison a variable with static storage; it might have
+		   gotten marked before gimplify_init_constructor promoted it
+		   to static.  */
+		gsi_remove (gsi, true);
+		return;
+	      }
+	  }
+
 	/* We delay folding of built calls from gimplification to
 	   here so the IL is in consistent state for the diagnostic
 	   machineries job.  */
--- gcc/testsuite/g++.dg/asan/initlist1.C.jj	2024-01-31 16:09:58.071289511 +0100
+++ gcc/testsuite/g++.dg/asan/initlist1.C	2024-01-31 16:09:58.071289511 +0100
@@ -0,0 +1,20 @@
+// PR c++/113531
+// { dg-do run { target c++11 } }
+// { dg-additional-options "-fsanitize=address" }
+
+#include <initializer_list>
+
+void f(int) { }
+
+void g()
+{
+  for (auto i : { 1, 2, 3 })
+    f (i);
+  f(42);
+}
+
+int main()
+{
+  g();
+  g();
+}


	Jakub

Re: [PATCH] gimple-low: Remove .ASAN_MARK calls on TREE_STATIC variables [PR113531]

[Richard Biener]

> Am 31.01.2024 um 16:20 schrieb Jakub Jelinek <jakub@redhat.com>:
> 
> On Wed, Jan 31, 2024 at 01:04:22PM +0100, Richard Biener wrote:
>>> Like this, so far just tested on the testcase.  Ok for trunk if it passes
>>> bootstrap/regtest on top of Jason's patch?
>> 
>> Note we fold all - well, all builtin - calls during gimple lowering.
> 
> Internal calls aren't builtin, so those aren't folded.
> 
>> Maybe we can put this special-casing there instead? (gimple-low.cc:797,
>> you possibly have to replace with a GIMPLE_NOP)
> 
> But sure, we can do it there as well.
> In that case I think we don't need it in asan.cc though, because
> gimple-low.cc is guaranteed to be performed on all statements from
> gimplification that survive in the IL.
> 
> So like this?

Lgtm,

Thanks,
Richard 

> 2024-01-31  Jakub Jelinek  <jakub@redhat.com>
>        Jason Merrill  <jason@redhat.com>
> 
>    PR c++/113531
>    * gimple-low.cc (lower_stmt): Remove .ASAN_MARK calls
>    on variables which were promoted to TREE_STATIC.
> 
>    * g++.dg/asan/initlist1.C: New test.
> 
> --- gcc/gimple-low.cc.jj    2024-01-03 11:51:22.133861623 +0100
> +++ gcc/gimple-low.cc    2024-01-31 16:16:21.326927572 +0100
> @@ -790,6 +790,21 @@ lower_stmt (gimple_stmt_iterator *gsi, s
>        return;
>      }
> 
> +    if (gimple_call_internal_p (stmt, IFN_ASAN_MARK))
> +      {
> +        tree base = gimple_call_arg (stmt, 1);
> +        gcc_checking_assert (TREE_CODE (base) == ADDR_EXPR);
> +        tree decl = TREE_OPERAND (base, 0);
> +        if (VAR_P (decl) && TREE_STATIC (decl))
> +          {
> +        /* Don't poison a variable with static storage; it might have
> +           gotten marked before gimplify_init_constructor promoted it
> +           to static.  */
> +        gsi_remove (gsi, true);
> +        return;
> +          }
> +      }
> +
>    /* We delay folding of built calls from gimplification to
>       here so the IL is in consistent state for the diagnostic
>       machineries job.  */
> --- gcc/testsuite/g++.dg/asan/initlist1.C.jj    2024-01-31 16:09:58.071289511 +0100
> +++ gcc/testsuite/g++.dg/asan/initlist1.C    2024-01-31 16:09:58.071289511 +0100
> @@ -0,0 +1,20 @@
> +// PR c++/113531
> +// { dg-do run { target c++11 } }
> +// { dg-additional-options "-fsanitize=address" }
> +
> +#include <initializer_list>
> +
> +void f(int) { }
> +
> +void g()
> +{
> +  for (auto i : { 1, 2, 3 })
> +    f (i);
> +  f(42);
> +}
> +
> +int main()
> +{
> +  g();
> +  g();
> +}
> 
> 
>    Jakub
> 

Re: [PATCH RFA] asan: poisoning promoted statics [PR113531]

[Jason Merrill]

On 1/31/24 03:51, Richard Biener wrote:
> On Wed, Jan 31, 2024 at 4:38 AM Jason Merrill <jason@redhat.com> wrote:
>>
>> Tested x86_64-pc-linux-gnu, OK for trunk?
> 
> It's a quite "late" fixup, I suppose you have tried to avoid marking it
> during gimplification?  I see we do parts of this during BIND_EXPR
> processing which is indeed a bit early but possibly difficult to rectify.

I also considered

> diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
> index 7f79b3cc7e6..c906d927a09 100644
> --- a/gcc/gimplify.cc
> +++ b/gcc/gimplify.cc
> @@ -1249,6 +1249,10 @@ asan_poison_variable (tree decl, bool poison, gimple_stmt_iterator *it,
>    if (zerop (unit_size))
>      return;
> 
> +  /* Or variables in static storage.  */
> +  if (TREE_STATIC (decl))
> +    return;
> +
>    /* It's necessary to have all stack variables aligned to ASAN granularity
>       bytes.  */
>    gcc_assert (!hwasan_sanitize_p () || hwasan_sanitize_stack_p ());

which fixes the bug by avoiding the poison mark, but it's too late to 
avoid the unpoison mark--though the unpoison is still removed by sanopt, 
so the end result is the same.  I decided to send the other patch 
because it applies to both, but I'm happy with either approach.

Jason