Oilpan: Fix ASan instrumentation around heap object headers.

Oilpan: Fix ASan instrumentation around heap object headers.

We poison the heap object headers because only our code should ever be
able to access them and only from a handful of methods. Poisoning the
headers lets us catch stray read/writes that end up in our headers and we
use the NO_SANITIZE_ADDRESS annotation on the handful of methods that
operate on the headers.

The ASan NO_SANITIZE_ADDRESS annotation does not propagate to acquireLoad.
Our first attempt to fix that was to unpoison the address accessed.
However, that does not work because we are using this code from multiple
threads without locking (which is the reason for using atomic ops).
Therefore, the threads will have races when it comes to poisoning.

This change fixes the issue by introducing asan aware
asanAcquireLoad/asanReleaseStore which will work on poisoned memory
when you know what you are doing.

Kostya, do you have any alternative suggestions?

R=erik.corry@gmail.com, kcc@chromium.org, oilpan-reviews@chromium.org, zerny@chromium.org
BUG=411712
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181638

[kcc1, 2014-09-08 16:01:18]

kcc@google.com changed reviewers:
+ dvyukov@chromium.org, glider@chromium.org, kcc@google.com

[kcc1, 2014-09-08 16:01:19]

I am OOO on a conference and this change may require more attention that I may
give now. 
I'd like glider/dvyukov to review this.

[zerny-chromium, 2014-09-09 06:01:29]

https://codereview.chromium.org/556443003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/556443003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:419: #if defined(ADDRESS_SANITIZER)
ditto

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

https://codereview.chromium.org/556443003/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1509: #if defined(ADDRESS_SANITIZER)
I'd prefer to avoid the code duplication by defining the asan variants for both
true and false values of defined(ADDRESS_SANITIZER).

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

https://codereview.chromium.org/556443003/diff/1/Source/wtf/Atomics.h#newcode219
Source/wtf/Atomics.h:219: 
Could we add:

#if !defined(ADDRESS_SANITIZER)
ALWAYS_INLINE unsigned asanAcquireLoad(volatile const unsigned* ptr)
{
    return acquireLoad(ptr);
}
...
#endif

and then avoid the #if-def'ing at the call sites.

https://codereview.chromium.org/556443003/diff/1/Source/wtf/Atomics.h#newcode233
Source/wtf/Atomics.h:233: #if defined(ADDRESS_SANITIZER)
and then remove the #if here too.

[wibling-chromium, 2014-09-09 07:20:07]

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

[wibling-chromium, 2014-09-09 07:20:07]

lgtm

[Mads Ager (chromium), 2014-09-09 08:22:15]

Updated, PTAL.

Erik, could you do the WTF review?

https://codereview.chromium.org/556443003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/556443003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:419: #if defined(ADDRESS_SANITIZER)
On 2014/09/09 06:01:28, zerny-chromium wrote:
> ditto

Done

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

https://codereview.chromium.org/556443003/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1509: #if defined(ADDRESS_SANITIZER)
On 2014/09/09 06:01:28, zerny-chromium wrote:
> I'd prefer to avoid the code duplication by defining the asan variants for
both
> true and false values of defined(ADDRESS_SANITIZER).

Done.

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

https://codereview.chromium.org/556443003/diff/1/Source/wtf/Atomics.h#newcode219
Source/wtf/Atomics.h:219: 
On 2014/09/09 06:01:28, zerny-chromium wrote:
> Could we add:
> 
> #if !defined(ADDRESS_SANITIZER)
> ALWAYS_INLINE unsigned asanAcquireLoad(volatile const unsigned* ptr)
> {
>     return acquireLoad(ptr);
> }
> ...
> #endif
> 
> and then avoid the #if-def'ing at the call sites.

Done.

https://codereview.chromium.org/556443003/diff/1/Source/wtf/Atomics.h#newcode233
Source/wtf/Atomics.h:233: #if defined(ADDRESS_SANITIZER)
On 2014/09/09 06:01:29, zerny-chromium wrote:
> and then remove the #if here too.

Done.

[zerny-chromium, 2014-09-09 08:26:50]

lgtm

[Erik Corry, 2014-09-09 08:36:40]

LGTM, but please wait for an LTGM from Kostya or someone from the ASAN team.

[Alexander Potapenko, 2014-09-09 09:40:30]

https://codereview.chromium.org/556443003/diff/40001/Source/wtf/Atomics.h
File Source/wtf/Atomics.h (right):

https://codereview.chromium.org/556443003/diff/40001/Source/wtf/Atomics.h#new...
Source/wtf/Atomics.h:135: __attribute__((no_sanitize_address)) ALWAYS_INLINE
void asanReleaseStore(volatile unsigned* ptr, unsigned value)
This is not gonna work.
ASan doesn't provide the __tsan_* functions. Moreover, if it did, those
functions would probably check the shadow values. But even ADDRESS_SANITIZER and
THREAD_SANITIZER flags can't be defined together in the same build.

You could probably copy the implementations of acquireLoad and ReleaseStore from
below here, but that's ugly (and platform-dependent).
I think what you really need here is C++11 atomics
(http://en.cppreference.com/w/cpp/atomic). They're understood by both ASan and
TSan, and both tools consider them as memory accesses, not inline functions, so
they'll inherit the no_sanitize_address attribute.

You can either use std::atomic_load/std::atomic_store inside
asanAcquireLoad/asanReleaseStore or just replace the asan* functions with their
std:: equivalents.

[Alexander Potapenko, 2014-09-09 09:46:39]

Ohh, and Chromium isn't C++11-ready yet..

[Mads Ager (chromium), 2014-09-09 09:51:36]

Yeah, we can't use the C++11 atomics yet.

https://codereview.chromium.org/556443003/diff/40001/Source/wtf/Atomics.h
File Source/wtf/Atomics.h (right):

https://codereview.chromium.org/556443003/diff/40001/Source/wtf/Atomics.h#new...
Source/wtf/Atomics.h:135: __attribute__((no_sanitize_address)) ALWAYS_INLINE
void asanReleaseStore(volatile unsigned* ptr, unsigned value)
On 2014/09/09 09:40:30, Alexander Potapenko wrote:
> This is not gonna work.
> ASan doesn't provide the __tsan_* functions. Moreover, if it did, those
> functions would probably check the shadow values. But even ADDRESS_SANITIZER
and
> THREAD_SANITIZER flags can't be defined together in the same build.
> 
> You could probably copy the implementations of acquireLoad and ReleaseStore
from
> below here, but that's ugly (and platform-dependent).
> I think what you really need here is C++11 atomics
> (http://en.cppreference.com/w/cpp/atomic). They're understood by both ASan and
> TSan, and both tools consider them as memory accesses, not inline functions,
so
> they'll inherit the no_sanitize_address attribute.
> 
> You can either use std::atomic_load/std::atomic_store inside
> asanAcquireLoad/asanReleaseStore or just replace the asan* functions with
their
> std:: equivalents.

Thanks! Since they can't be defined together in the same build I should just
remove this and it will take the non ADDRESS_SANITIZER ones from below when
running tsan.

[Alexander Potapenko, 2014-09-09 09:57:30]

LGTM

https://codereview.chromium.org/556443003/diff/60001/Source/wtf/Atomics.h
File Source/wtf/Atomics.h (right):

https://codereview.chromium.org/556443003/diff/60001/Source/wtf/Atomics.h#new...
Source/wtf/Atomics.h:113: 
Did you mean adding these blank lines? (Here and below)

https://codereview.chromium.org/556443003/diff/60001/Source/wtf/Atomics.h#new...
Source/wtf/Atomics.h:237: using WTF::asanAcquireLoad;
How about 'asanUnsafe{AcquireLoad,ReleaseStore}'?

[Mads Ager (chromium), 2014-09-09 10:02:40]

https://codereview.chromium.org/556443003/diff/60001/Source/wtf/Atomics.h
File Source/wtf/Atomics.h (right):

https://codereview.chromium.org/556443003/diff/60001/Source/wtf/Atomics.h#new...
Source/wtf/Atomics.h:113: 
On 2014/09/09 09:57:30, Alexander Potapenko wrote:
> Did you mean adding these blank lines? (Here and below)

Yeah, I did to make the formatting a bit more uniform.

https://codereview.chromium.org/556443003/diff/60001/Source/wtf/Atomics.h#new...
Source/wtf/Atomics.h:237: using WTF::asanAcquireLoad;
On 2014/09/09 09:57:30, Alexander Potapenko wrote:
> How about 'asanUnsafe{AcquireLoad,ReleaseStore}'?

Good idea, will do. I like the addition of unsafe here to make people think
twice before using these!

[Mads Ager (chromium), 2014-09-09 10:08:57]

The CQ bit was checked by ager@chromium.org

[commit-bot: I haz the power, 2014-09-09 10:09:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ager@chromium.org/556443003/80001

[commit-bot: I haz the power, 2014-09-09 11:59:40]

Committed patchset #5 (id:80001) as 181638

[Nico, 2014-09-09 16:33:32]

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

[Nico, 2014-09-09 16:33:32]

This broke the asan/win build:

http://build.chromium.org/p/chromium.fyi/builders/Cr%20Win%20Clang%20%28asan%...

C:\b\build\slave\Cr_Win_Clang__asan_\build\src\third_party\WebKit\Source\wtf/Atomics.h(190)
: error C2065: 'no_sanitize_address' : undeclared identifier
C:\b\build\slave\Cr_Win_Clang__asan_\build\src\third_party\WebKit\Source\wtf/Atomics.h(190)
: error C2448: '__attribute__' : function-style initializer appears to be a
function definition
C:\b\build\slave\Cr_Win_Clang__asan_\build\src\third_party\WebKit\Source\wtf/Atomics.h(196)
: error C2065: 'no_sanitize_address' : undeclared identifier
C:\b\build\slave\Cr_Win_Clang__asan_\build\src\third_party\WebKit\Source\wtf/Atomics.h(196)
: error C2448: '__attribute__' : function-style initializer appears to be a
function definition
ninja: build stopped: subcommand failed.

[Nico, 2014-09-09 16:36:47]

…you need to do something like
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
(we try to build the file with clang, which fails due to a __try in
ThreadingWin.cpp since clang can't do SEH yet, then we fall back to cl.exe, and
it fails 'cause it doesn't understand the __attribute__ syntax.

[Nico, 2014-09-09 16:47:04]

fix: https://codereview.chromium.org/556863005/

[Mads Ager (chromium), 2014-09-09 16:57:18]

On 2014/09/09 16:47:04, Nico (hiding) wrote:
> fix: https://codereview.chromium.org/556863005/

Thanks for the fix. A half clang half cl build, yikes! ;-)