| Message ID | 20240131033759.2236614-1-jason@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [RFA] asan: poisoning promoted statics [PR113531] | expand |
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 >
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
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
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(); +}