Various ASan exemptions to allow Oilpan pre-sweep poisoning of unmarkeds.

Various ASan exemptions to allow Oilpan pre-sweep poisoning of unmarkeds.

Heap objects must not access other dead objects while finalizing.
To help catch out those that do, the Oilpan GC can now poison 
unmarked, to-be-swept objects. To be able to make use of that
with ASan enabled, a couple of benign accesses that run afoul
of that strict check are still needed & must be allowed to
make this feasible.

Here provided together -- the ones related to Timer stopping &
removal from the timer heap aren't something we'd typically want
to impose one the Blink codebase in general, but beside the
goal of better ASan coverage, the timer heap will soon be removed
and the need for these should fall away.

(Reland of r195609, non-Oilpan compilation issue addressed.)

R=haraken,tkent,yutak
BUG=420515
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195613

[haraken, 2015-05-01 15:12:31]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-05-01 15:12:32]

I haven't looked at the timer heap, but the NO_SANITIZE_ADDRESS macros in the
heap makes sense and is worth landing probably?

https://codereview.chromium.org/1120943002/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/1120943002/diff/1/Source/platform/heap/Handle...
Source/platform/heap/Handle.h:56: NO_SANITIZE_ADDRESS

This looks valid.

https://codereview.chromium.org/1120943002/diff/1/Source/platform/heap/Handle...
Source/platform/heap/Handle.h:128: NO_SANITIZE_ADDRESS

This looks valid.

https://codereview.chromium.org/1120943002/diff/1/Source/wtf/LinkedHashSet.h
File Source/wtf/LinkedHashSet.h (right):

https://codereview.chromium.org/1120943002/diff/1/Source/wtf/LinkedHashSet.h#...
Source/wtf/LinkedHashSet.h:57: NO_SANITIZE_ADDRESS

This looks valid.

[haraken, 2015-05-07 07:27:34]

haraken@chromium.org changed reviewers:
+ yutak@chromium.org

[haraken, 2015-05-07 07:27:35]

+yutak: This is a CL Sigbjorn created to suppress false-positive errors detected
by ASan poisoning.

[haraken, 2015-05-18 23:12:51]

LGTM (Let's add more information to the CL description.)

If yutak is ok with landing this CL, let's land this.

[Yuta Kitamura, 2015-05-19 03:56:13]

I've found a few more places that use-after-poison happens
on my Linux workstation. Now I'm looking at these.

I'm fine with landing just these NO_SANITIZE_ADDRESS changes
first without the removal of "#if 0" in ThreadState.cpp.
Seems like we need more testing before we can enable
poisoning.

[Yuta Kitamura, 2015-05-19 06:08:28]

With that said, I'd like to note that I still don't
understand rationales of all of the NO_SANITIZE_ADDRESS
annotations...

These are probably the results of playing a whack-a-mole
(which I'm doing now), and the reason why each annotation
is safe may not be understood, I suppose.

[sof, 2015-05-19 06:12:25]

On 2015/05/19 03:56:13, Yuta Kitamura wrote:
> I've found a few more places that use-after-poison happens
> on my Linux workstation. Now I'm looking at these.
> 
> I'm fine with landing just these NO_SANITIZE_ADDRESS changes
> first without the removal of "#if 0" in ThreadState.cpp.
> Seems like we need more testing before we can enable
> poisoning.

Thanks, I suspect there are other timer-related methods where ASan might trip up
(shared timers, perhaps), depending on..timing.

If this really needs to be landed right now, then I would prefer it if we make
the annotations to Timer entry points Oilpan specific -- i.e.,
NO_OILPAN_SANITIZE_ADDRESS (or NO_LAZY_SWEEPING_SANITIZE_ADDRESS) -- so that we
don't impact ASan testing elsewhere.

I expect this Timer code to be gone by the time we're ready to re-try lazy
sweeping with non-Oilpan again.

[sof, 2015-05-19 13:34:00]

On 2015/05/19 06:12:25, sof wrote:
> On 2015/05/19 03:56:13, Yuta Kitamura wrote:
> > I've found a few more places that use-after-poison happens
> > on my Linux workstation. Now I'm looking at these.
> > 
> > I'm fine with landing just these NO_SANITIZE_ADDRESS changes
> > first without the removal of "#if 0" in ThreadState.cpp.
> > Seems like we need more testing before we can enable
> > poisoning.
> 
> Thanks, I suspect there are other timer-related methods where ASan might trip
up
> (shared timers, perhaps), depending on..timing.
> 
> If this really needs to be landed right now, then I would prefer it if we make
> the annotations to Timer entry points Oilpan specific -- i.e.,
> NO_OILPAN_SANITIZE_ADDRESS (or NO_LAZY_SWEEPING_SANITIZE_ADDRESS) -- so that
we
> don't impact ASan testing elsewhere.
> 

Did this in ps#2 (sorry for being so slow in following up.)

[sof, 2015-05-19 13:36:45]

https://codereview.chromium.org/1120943002/diff/20001/Source/wtf/LinkedHashSet.h
File Source/wtf/LinkedHashSet.h (right):

https://codereview.chromium.org/1120943002/diff/20001/Source/wtf/LinkedHashSe...
Source/wtf/LinkedHashSet.h:25: #include "platform/heap/AddressSanitizer.h"
a layering/dependency violation per se.

[haraken, 2015-05-19 23:08:16]

LGTM

[sof, 2015-05-20 06:18:57]

sigbjornf@opera.com changed reviewers:
+ tkent@chromium.org

[sof, 2015-05-20 06:18:58]

Thanks.

I've avoided the improper wtf/ dependency (tkent@: could you take a look,
please?) + enabled this poisioning iff ENABLE(OILPAN) -- yutak@: is that
acceptable to you?

And removed the use of the word "hack" in the description, but it still applies
in some ways, I think :) But better ASan coverage is our friend.

[Yuta Kitamura, 2015-05-20 06:37:29]

On 2015/05/20 06:18:58, sof wrote:
> I've avoided the improper wtf/ dependency (tkent@: could you take a look,
> please?) + enabled this poisioning iff ENABLE(OILPAN) -- yutak@: is that
> acceptable to you?

On my environment there are a few more failing signatures (some in Timer,
and other in WebGL something), so this is going to cause test failures
on my env (but not on the bots).

I'm okay with landing this first, so I can follow up later, without
breaking the bots. LGTM.

[tkent, 2015-05-20 06:46:43]

lgtm
We may move (a part of) platform/heap/AddressSanitizer.h to wtf/.

https://codereview.chromium.org/1120943002/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1120943002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:899: #if ENABLE(OILPAN
append ')'

[sof, 2015-05-20 06:48:15]

On 2015/05/20 06:37:29, Yuta Kitamura wrote:
> On 2015/05/20 06:18:58, sof wrote:
> > I've avoided the improper wtf/ dependency (tkent@: could you take a look,
> > please?) + enabled this poisioning iff ENABLE(OILPAN) -- yutak@: is that
> > acceptable to you?
> 
> On my environment there are a few more failing signatures (some in Timer,
> and other in WebGL something), so this is going to cause test failures
> on my env (but not on the bots).
> 
> I'm okay with landing this first, so I can follow up later, without
> breaking the bots. LGTM.

ok, thanks. It will cause failures on the Oilpan ASan bot, but I think that's
acceptable -- there won't be many, and we can easily iterate here on any
no-sanitize exemptions still needed. I hope that leaves some "real" bugs..

[Yuta Kitamura, 2015-05-20 06:58:22]

(My last comment was based on an assumption that there's no
 Oilpan+ASAN bots out there... is there any? If there's one,
 it shouldn't block the CQ, so my LGTM still stands.)

https://codereview.chromium.org/1120943002/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1120943002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:899: #if ENABLE(OILPAN
#if ENABLE(OILPAN
                 ^
                 ')' is missing here

[sof, 2015-05-20 06:59:14]

https://codereview.chromium.org/1120943002/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1120943002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:899: #if ENABLE(OILPAN
On 2015/05/20 06:46:43, tkent wrote:
> append ')'

Oops, thanks for catching this.

[sof, 2015-05-20 07:00:36]

On 2015/05/20 06:58:22, Yuta Kitamura wrote:
> (My last comment was based on an assumption that there's no
>  Oilpan+ASAN bots out there... is there any? If there's one,
>  it shouldn't block the CQ, so my LGTM still stands.)
> 

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Oilpan%...
- I check it throughout each day, I wish CQ did something similar :)

[sof, 2015-05-20 07:01:30]

On 2015/05/20 06:46:43, tkent wrote:
> lgtm
> We may move (a part of) platform/heap/AddressSanitizer.h to wtf/.
> 

thanks, makes sense to do that now -- will put up a CL.

[sof, 2015-05-20 08:23:50]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-20 08:23:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tkent@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1120943002/#ps100001
(title: "comment sync")

[commit-bot: I haz the power, 2015-05-20 08:24:09]

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

[commit-bot: I haz the power, 2015-05-20 10:28:49]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195609

[sof, 2015-05-20 11:05:41]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1144933004/ by sigbjornf@opera.com.

The reason for reverting is: Broke non-Oilpan ASAN compilation,


http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20ASAN/bu....

[sof, 2015-05-20 12:02:27]

relanding, non-Oilpan compilation error fixed. Verified locally to build.

[sof, 2015-05-20 12:02:34]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-20 12:02:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, tkent@chromium.org, yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1120943002/#ps140001
(title: "rebased")

[commit-bot: I haz the power, 2015-05-20 12:02:50]

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

[commit-bot: I haz the power, 2015-05-20 13:09:38]

Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195613

[Nico, 2016-10-02 00:54:39]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-10-02 00:54:40]

https://codereview.chromium.org/1120943002/diff/140001/Source/platform/heap/A...
File Source/platform/heap/AddressSanitizer.h (right):

https://codereview.chromium.org/1120943002/diff/140001/Source/platform/heap/A...
Source/platform/heap/AddressSanitizer.h:60: #define
NO_LAZY_SWEEP_SANITIZE_ADDRESS NO_SANITIZE_ADDRESS
Now that oilpan is on, do we still need this alternative spelling of
NO_SANITIZE_ADDRESS?

[haraken, 2016-10-03 02:47:39]

On 2016/10/02 00:54:40, Nico (mostly away until Oct 3) wrote:
>
https://codereview.chromium.org/1120943002/diff/140001/Source/platform/heap/A...
> File Source/platform/heap/AddressSanitizer.h (right):
> 
>
https://codereview.chromium.org/1120943002/diff/140001/Source/platform/heap/A...
> Source/platform/heap/AddressSanitizer.h:60: #define
> NO_LAZY_SWEEP_SANITIZE_ADDRESS NO_SANITIZE_ADDRESS
> Now that oilpan is on, do we still need this alternative spelling of
> NO_SANITIZE_ADDRESS?

You're right. Will be fixed in https://codereview.chromium.org/2390553002/.