Add ASan exemption when iterating cross-thread-persistents.

Add ASan exemption when iterating cross-thread-persistents.

When running a termination GC or tracing, the set/region of live
CrossThreadPersistent nodes are iterated over, checking if the objects
they point to belong to the current thread.

As heap objects can have CrossThreadPersistent<> fields, it is possible
for there to be CrossThreadPersistent nodes which point back to heap
objects about to be swept. When ASan is enabled, the page sweeping takes
care of poisioning all to-be-swept objects first.

The combination of the above two means that persistent iteration can
try to inspect one of these poisoned objects, which will trigger an
ASan error. The persistent will not be further used, as it doesn't
belong to the thread. To accommodate this, we do disable ASan while
performing the object lookup while iterating the CrossThreadPersistent
node set.

R=
BUG=620754
Committed: https://crrev.com/04cff368a4228a43d484d6e71828d1f795518a39
Cr-Commit-Position: refs/heads/master@{#401354}

[sof, 2016-06-22 13:30:54]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2016-06-22 13:30:54]

please take a look.

If we can move to CrossThreadPersistent usage & underlying representation that
would avoid having to touch them all when iterating, so much the better.

[haraken, 2016-06-22 14:57:37]

Very well spotted... LGTM.

> If we can move to CrossThreadPersistent usage & underlying representation that
> would avoid having to touch them all when iterating, so much the better.

Agreed.

[haraken, 2016-06-22 15:03:45]

Maybe I'm a bit confused. atomicGet() is called by
prepareForThreadStateTermination(). Why can the region have been poisoned at the
point?

[sof, 2016-06-22 15:14:53]

On 2016/06/22 15:03:45, haraken wrote:
> Maybe I'm a bit confused. atomicGet() is called by
> prepareForThreadStateTermination(). Why can the region have been poisoned at
the
> point?

The region isn't, but what its nodes may point back to --

 * Thread A & B completes a GC.
 * Thread B runs prefinalizers and starts eager sweeping (=> its arenas have
been poisoned.)
 * Thread A starts a thread termination GC.

If A starts iterating the cross-thread-persistents before B has swept out the
objects that contains CrossThreadPersistent<> fields, ASan poisoning errors are
possible.

I believe this can also happen for tracePersistentNode() over the CTP region.

[haraken, 2016-06-22 15:27:38]

On 2016/06/22 15:14:53, sof wrote:
> On 2016/06/22 15:03:45, haraken wrote:
> > Maybe I'm a bit confused. atomicGet() is called by
> > prepareForThreadStateTermination(). Why can the region have been poisoned at
> the
> > point?
> 
> The region isn't, but what its nodes may point back to --
> 
>  * Thread A & B completes a GC.
>  * Thread B runs prefinalizers and starts eager sweeping (=> its arenas have
> been poisoned.)
>  * Thread A starts a thread termination GC.
> 
> If A starts iterating the cross-thread-persistents before B has swept out the
> objects that contains CrossThreadPersistent<> fields, ASan poisoning errors
are
> possible.
> 
> I believe this can also happen for tracePersistentNode() over the CTP region.

Ah, thanks for the clarification! Makes sense.

I might prefer adding UNPOISON/POISON macros to the iteration to make it clear
what we're doing.

[sof, 2016-06-22 15:31:40]

On 2016/06/22 15:27:38, haraken wrote:
> On 2016/06/22 15:14:53, sof wrote:
> > On 2016/06/22 15:03:45, haraken wrote:
> > > Maybe I'm a bit confused. atomicGet() is called by
> > > prepareForThreadStateTermination(). Why can the region have been poisoned
at
> > the
> > > point?
> > 
> > The region isn't, but what its nodes may point back to --
> > 
> >  * Thread A & B completes a GC.
> >  * Thread B runs prefinalizers and starts eager sweeping (=> its arenas have
> > been poisoned.)
> >  * Thread A starts a thread termination GC.
> > 
> > If A starts iterating the cross-thread-persistents before B has swept out
the
> > objects that contains CrossThreadPersistent<> fields, ASan poisoning errors
> are
> > possible.
> > 
> > I believe this can also happen for tracePersistentNode() over the CTP
region.
> 
> Ah, thanks for the clarification! Makes sense.
> 
> I might prefer adding UNPOISON/POISON macros to the iteration to make it clear
> what we're doing.

That might trip up other threads trying to use the CTP at the same time?

[haraken, 2016-06-22 15:44:45]

On 2016/06/22 15:31:40, sof wrote:
> On 2016/06/22 15:27:38, haraken wrote:
> > On 2016/06/22 15:14:53, sof wrote:
> > > On 2016/06/22 15:03:45, haraken wrote:
> > > > Maybe I'm a bit confused. atomicGet() is called by
> > > > prepareForThreadStateTermination(). Why can the region have been
poisoned
> at
> > > the
> > > > point?
> > > 
> > > The region isn't, but what its nodes may point back to --
> > > 
> > >  * Thread A & B completes a GC.
> > >  * Thread B runs prefinalizers and starts eager sweeping (=> its arenas
have
> > > been poisoned.)
> > >  * Thread A starts a thread termination GC.
> > > 
> > > If A starts iterating the cross-thread-persistents before B has swept out
> the
> > > objects that contains CrossThreadPersistent<> fields, ASan poisoning
errors
> > are
> > > possible.
> > > 
> > > I believe this can also happen for tracePersistentNode() over the CTP
> region.
> > 
> > Ah, thanks for the clarification! Makes sense.
> > 
> > I might prefer adding UNPOISON/POISON macros to the iteration to make it
clear
> > what we're doing.
> 
> That might trip up other threads trying to use the CTP at the same time?

You're right. PS1 looks good.

[sof, 2016-06-22 18:08:33]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2016-06-22 18:09:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2087253002/1

[commit-bot: I haz the power, 2016-06-22 18:20:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-06-22 18:22:43]

Description was changed from

==========
Add ASan exemption when iterating cross-thread-persistents.

When running a termination GC or tracing, the set/region of live
CrossThreadPersistent nodes are iterated over, checking if the objects
they point to belong to the current thread.

As heap objects can have CrossThreadPersistent<> fields, it is possible
for there to be CrossThreadPersistent nodes which point back to heap
objects about to be swept. When ASan is enabled, the page sweeping takes
care of poisioning all to-be-swept objects first.

The combination of the above two means that persistent iteration can
try to inspect one of these poisoned objects, which will trigger an
ASan error. The persistent will not be further used, as it doesn't
belong to the thread. To accommodate this, we do disable ASan while
performing the object lookup while iterating the CrossThreadPersistent
node set.

R=
BUG=620754
==========

to

==========
Add ASan exemption when iterating cross-thread-persistents.

When running a termination GC or tracing, the set/region of live
CrossThreadPersistent nodes are iterated over, checking if the objects
they point to belong to the current thread.

As heap objects can have CrossThreadPersistent<> fields, it is possible
for there to be CrossThreadPersistent nodes which point back to heap
objects about to be swept. When ASan is enabled, the page sweeping takes
care of poisioning all to-be-swept objects first.

The combination of the above two means that persistent iteration can
try to inspect one of these poisoned objects, which will trigger an
ASan error. The persistent will not be further used, as it doesn't
belong to the thread. To accommodate this, we do disable ASan while
performing the object lookup while iterating the CrossThreadPersistent
node set.

R=
BUG=620754

Committed: https://crrev.com/04cff368a4228a43d484d6e71828d1f795518a39
Cr-Commit-Position: refs/heads/master@{#401354}
==========

[commit-bot: I haz the power, 2016-06-22 18:22:44]

Patchset 1 (id:??) landed as
https://crrev.com/04cff368a4228a43d484d6e71828d1f795518a39
Cr-Commit-Position: refs/heads/master@{#401354}