Fix FreeList::zapFreedMemory to fail "use-of-uninitialized-value" on MSAN

Fix FreeList::zapFreedMemory to fail "use-of-uninitialized-value" on MSAN

A CL for fallback font collection[1] broke MSAN bots[2] and was reverted.

Another try to fix[2] also fails on MSAN bots at
FreeList::zapFreeMemory[3]. This patch allows zapFreeMemory to write to
uninitialized memory.

[1] https://codereview.chromium.org/1241613006/
[2] https://codereview.chromium.org/1244973003/
[3] http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_msan_rel_ng/builds/84

BUG=514099
Committed: https://crrev.com/021efec808267f79934050714f4732961fe14c47
git-svn-id: svn://svn.chromium.org/blink/trunk@199701 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[kojii, 2015-07-27 04:13:43]

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

[kojii, 2015-07-27 04:13:44]

PTAL. Got an advice from yutak@, but I'm not familiar with these code.

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

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

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

On 2015/07/27 04:13:44, kojii wrote:
> PTAL. Got an advice from yutak@, but I'm not familiar with these code.

Thanks for taking a look at this. Hmm, actually I'm not familiar with MSan and
not sure if this is a right remedy.

glider@: Do you have any insight on this?

[Yuta Kitamura, 2015-07-27 05:46:25]

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

[Yuta Kitamura, 2015-07-27 05:46:25]

In my understanding, MSAN detects the read access for
"address[i] != reuseAllowedZapValue" as it reads
(potentially) uninitialized region of memory.

I think this patch is harmless, but if this isn't desirable,
we may want to fill the allocated region with zap values
unconditionally.

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

https://codereview.chromium.org/1259893002/diff/1/Source/platform/heap/Heap.c...
Source/platform/heap/Heap.cpp:1171: #if ENABLE(ASSERT) ||
defined(LEAK_SANITIZER) || defined(ADDRESS_SANITIZER)
Add the following?

    || defined(MEMORY_SANITIZER)

[haraken, 2015-07-27 05:47:58]

On 2015/07/27 05:46:25, Yuta Kitamura wrote:
> In my understanding, MSAN detects the read access for
> "address[i] != reuseAllowedZapValue" as it reads
> (potentially) uninitialized region of memory.
> 
> I think this patch is harmless, but if this isn't desirable,
> we may want to fill the allocated region with zap values
> unconditionally.

I'm wondering why _only_ this pace needs NO_SANITIZE_MSAN. In other words, I
wonder why we don't need NO_SANITIZE_MSAN in all places where NO_SANITIZE_ASAN
is used.

[Yuta Kitamura, 2015-07-27 05:54:21]

On 2015/07/27 05:47:58, haraken wrote:
> I'm wondering why _only_ this pace needs NO_SANITIZE_MSAN. In other words, I
> wonder why we don't need NO_SANITIZE_MSAN in all places where NO_SANITIZE_ASAN
> is used.

Yeah, that was my first impression, too, but, according to my
quick research, the issues MSAN can detect are pretty limited,
as it only checks uninitialized memory reads. So, I don't think
it is appropriate to apply NO_SANITIZE_MEMORY everywhere we have
NO_SANITIZE_ADDRESS.

[haraken, 2015-07-27 06:08:45]

On 2015/07/27 05:54:21, Yuta Kitamura wrote:
> On 2015/07/27 05:47:58, haraken wrote:
> > I'm wondering why _only_ this pace needs NO_SANITIZE_MSAN. In other words, I
> > wonder why we don't need NO_SANITIZE_MSAN in all places where
NO_SANITIZE_ASAN
> > is used.
> 
> Yeah, that was my first impression, too, but, according to my
> quick research, the issues MSAN can detect are pretty limited,
> as it only checks uninitialized memory reads. So, I don't think
> it is appropriate to apply NO_SANITIZE_MEMORY everywhere we have
> NO_SANITIZE_ADDRESS.

Thanks, that makes sense to me.

[sof, 2015-07-27 06:26:42]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-07-27 06:26:42]

Why does this only fail with the CL reverted?

[kojii, 2015-07-27 06:26:45]

On 2015/07/27 05:46:25, Yuta Kitamura wrote:
> 
>
https://codereview.chromium.org/1259893002/diff/1/Source/platform/heap/Heap.cpp
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1259893002/diff/1/Source/platform/heap/Heap.c...
> Source/platform/heap/Heap.cpp:1171: #if ENABLE(ASSERT) ||
> defined(LEAK_SANITIZER) || defined(ADDRESS_SANITIZER)
> Add the following?
> 
>     || defined(MEMORY_SANITIZER)

|zapFreedMemory| is called only from |SET_MEMORY_INACCESSIBLE|:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
so if we add that, we should add to here too.

But |SET_MEMORY_INACCESSIBLE| calls |__asan_poison_memory_region| right after
|zapFreeMemory|. I'm not sure whether to call it or not if MEMORY_SANITIZER &&
!ASSERT && !LEAK_SANITIZER && !ADDRESS_SANITIZER. Do you think we should?

> I think this patch is harmless, but if this isn't desirable,

There are two flaky results in the MSAN bot for PS1 (though it's green):
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...
==30774==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x7f75c784231f in blink::FreeList::addToFreeList(unsigned char*, unsigned
long) third_party/WebKit/Source/platform/heap/Heap.cpp:1127:13

I suppose there are more work to be done for Heap.cpp to work nicely with MSAN,
they are just not hit yet. Maybe also adding |defined(MEMORY_SANITIZER)| to
|addToFreeList()| is yet another "harmless but not the best" fix?

I can add these and run bots so that reviewers can pick whichever looks better.

[kojii, 2015-07-27 06:52:45]

On 2015/07/27 06:26:42, sof wrote:
> Why does this only fail with the CL reverted?

That I don't know...our best guess when talking with yutak@ is that something in
the CL caused sweeper to collect a block. But the CL does not rely on Oilpan to
collect memory, so I don't have answers. I'm interested in the answer too, but
don't have clue in this area.

[kojii, 2015-07-27 08:16:56]

Looks like it does not help the flakiness in addToFreeList, I'll leave it from
this CL.

[Alexander Potapenko, 2015-07-27 09:39:20]

Looks like you're fine, any other insight needed?

https://codereview.chromium.org/1259893002/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1259893002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.h:81: #if ENABLE(ASSERT) || defined(LEAK_SANITIZER) ||
defined(ADDRESS_SANITIZER) || defined(MEMORY_SANITIZER)
Note that ASAN_{POISON,UNPOISON}_MEMORY_REGION makes little sense in MSan builds
(unless you define it to poison/unpoison MSan shadow, in which case it needs to
be renamed).

[Alexander Potapenko, 2015-07-27 09:45:00]

https://codereview.chromium.org/1259893002/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1259893002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1177: // See the comment in addToFreeList().
NO_SANITIZE_MEMORY will remove checks for uninitialized reads in line 1178, so
there'll be no MSan reports in zapFreeMemory().
It will however preserve the code that marks address[i] as initialized in line
1179, so that random reads of that memory after it has been freed will not be
detected. Is that really what you want?

[kojii, 2015-07-27 11:08:56]

On 2015/07/27 09:45:00, Alexander Potapenko wrote:
>
https://codereview.chromium.org/1259893002/diff/40001/Source/platform/heap/He...
> File Source/platform/heap/Heap.cpp (right):
> 
>
https://codereview.chromium.org/1259893002/diff/40001/Source/platform/heap/He...
> Source/platform/heap/Heap.cpp:1177: // See the comment in addToFreeList().
> NO_SANITIZE_MEMORY will remove checks for uninitialized reads in line 1178, so
> there'll be no MSan reports in zapFreeMemory().
> It will however preserve the code that marks address[i] as initialized in line
> 1179, so that random reads of that memory after it has been freed will not be
> detected. Is that really what you want?

Hmm...that side-effect doesn't sound ideal. What else could we do better? Does
it make sense to no-op SET_MEMORY_INACCESSIBLE for MSAN builds? i.e.; change

  #if ENABLE(ASSERT) || defined(LEAK_SANITIZER) || defined(ADDRESS_SANITIZER)
to:
  #if (ENABLE(ASSERT) || defined(LEAK_SANITIZER) || defined(ADDRESS_SANITIZER))
&& !defined(MEMORY_SANITIZER)
?

[Alexander Potapenko, 2015-07-27 11:32:48]

First, I assume you need to zap the freed memory despite ASan and MSan are able
to catch accesses to it, just for the case some random system library tries to
dereference that memory and crashes. Otherwise you don't need to call
zapFreedMemory() from SET_MEMORY_INACCESSIBLE, because ASAN_POISON_MEMORY_REGION
already means "report an error whenever someone touches that memory".

In order to make MSan behave similarly, we need to call __msan_poison for the
freed memory (again, probably after zapping it).

[kojii, 2015-07-28 00:21:13]

On 2015/07/27 11:32:48, Alexander Potapenko wrote:
> First, I assume you need to zap the freed memory despite ASan and MSan are
able
> to catch accesses to it, just for the case some random system library tries to
> dereference that memory and crashes. Otherwise you don't need to call
> zapFreedMemory() from SET_MEMORY_INACCESSIBLE, because
ASAN_POISON_MEMORY_REGION
> already means "report an error whenever someone touches that memory".
> 
> In order to make MSan behave similarly, we need to call __msan_poison for the
> freed memory (again, probably after zapping it).

PS4 did what glider@ said, if I'm not mistaken, and now I see 267 failures of
MemorySanitizer: use-of-uninitialized-value in FreeList::addToFreeList. That's
probably right, addToFreeList has several lines for ASAN, and from what I
understand, it needs to be modified for MSAN too.

May I take PS3 and file a separate bug for addToFreeList, or should I continue
investigating addToFreeList? I have mild preference to the former as this MSAN
failure is blocking a layout crasher, but I'd be happy to follow advices.

[sof, 2015-07-28 05:41:13]

haraken@: what benefit does freed-mem zapping bring if *San is enabled? (i.e.,
would it be sufficient to only use it for "simple" debug builds.)

[haraken, 2015-07-28 06:02:06]

On 2015/07/28 05:41:13, sof wrote:
> haraken@: what benefit does freed-mem zapping bring if *San is enabled? (i.e.,
> would it be sufficient to only use it for "simple" debug builds.)

The zapping is necessary to delay reusing freed regions. This is meaningful in
ASan to detect use-after-frees.

(I'm OK with just disabling it in MSan.)

[sof, 2015-07-28 06:07:11]

On 2015/07/28 06:02:06, haraken wrote:
> On 2015/07/28 05:41:13, sof wrote:
> > haraken@: what benefit does freed-mem zapping bring if *San is enabled?
(i.e.,
> > would it be sufficient to only use it for "simple" debug builds.)
> 
> The zapping is necessary to delay reusing freed regions. This is meaningful in
> ASan to detect use-after-frees.
> 
> (I'm OK with just disabling it in MSan.)

But isn't the free-mem poisoning that we already do with ASan enabled more
stringent than that already?

[haraken, 2015-07-28 06:11:18]

On 2015/07/28 06:07:11, sof wrote:
> On 2015/07/28 06:02:06, haraken wrote:
> > On 2015/07/28 05:41:13, sof wrote:
> > > haraken@: what benefit does freed-mem zapping bring if *San is enabled?
> (i.e.,
> > > would it be sufficient to only use it for "simple" debug builds.)
> > 
> > The zapping is necessary to delay reusing freed regions. This is meaningful
in
> > ASan to detect use-after-frees.
> > 
> > (I'm OK with just disabling it in MSan.)
> 
> But isn't the free-mem poisoning that we already do with ASan enabled more
> stringent than that already?

The reuse delay is a more stringent verification :)

If we don't have the reuse delay, the freed region is reused in the next GC
cycle. The freed region is poisoned only for one GC cycle. If we have the reuse
delay, the freed region is not reused in the next GC cycle. The freed region is
poisoned for a couple of GC cycles.

[sof, 2015-07-28 06:18:36]

On 2015/07/28 06:11:18, haraken wrote:
> On 2015/07/28 06:07:11, sof wrote:
> > On 2015/07/28 06:02:06, haraken wrote:
> > > On 2015/07/28 05:41:13, sof wrote:
> > > > haraken@: what benefit does freed-mem zapping bring if *San is enabled?
> > (i.e.,
> > > > would it be sufficient to only use it for "simple" debug builds.)
> > > 
> > > The zapping is necessary to delay reusing freed regions. This is
meaningful
> in
> > > ASan to detect use-after-frees.
> > > 
> > > (I'm OK with just disabling it in MSan.)
> > 
> > But isn't the free-mem poisoning that we already do with ASan enabled more
> > stringent than that already?
> 
> The reuse delay is a more stringent verification :)
> 
> If we don't have the reuse delay, the freed region is reused in the next GC
> cycle. The freed region is poisoned only for one GC cycle. If we have the
reuse
> delay, the freed region is not reused in the next GC cycle. The freed region
is
> poisoned for a couple of GC cycles.

Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after these zapped
regions have been touched. (=> ASan will flag any attempts to access them
first.)

[haraken, 2015-07-28 06:33:26]

On 2015/07/28 06:18:36, sof wrote:
> On 2015/07/28 06:11:18, haraken wrote:
> > On 2015/07/28 06:07:11, sof wrote:
> > > On 2015/07/28 06:02:06, haraken wrote:
> > > > On 2015/07/28 05:41:13, sof wrote:
> > > > > haraken@: what benefit does freed-mem zapping bring if *San is
enabled?
> > > (i.e.,
> > > > > would it be sufficient to only use it for "simple" debug builds.)
> > > > 
> > > > The zapping is necessary to delay reusing freed regions. This is
> meaningful
> > in
> > > > ASan to detect use-after-frees.
> > > > 
> > > > (I'm OK with just disabling it in MSan.)
> > > 
> > > But isn't the free-mem poisoning that we already do with ASan enabled more
> > > stringent than that already?
> > 
> > The reuse delay is a more stringent verification :)
> > 
> > If we don't have the reuse delay, the freed region is reused in the next GC
> > cycle. The freed region is poisoned only for one GC cycle. If we have the
> reuse
> > delay, the freed region is not reused in the next GC cycle. The freed region
> is
> > poisoned for a couple of GC cycles.
> 
> Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after these zapped
> regions have been touched. (=> ASan will flag any attempts to access them
> first.)

Without zapping the region with reuseForbiddenZapValue, how can we know when the
freed memory is OK to reuse? Zapping doesn't matter, the zapped values matter.

[sof, 2015-07-28 06:39:20]

On 2015/07/28 06:33:26, haraken wrote:
> On 2015/07/28 06:18:36, sof wrote:
> > On 2015/07/28 06:11:18, haraken wrote:
> > > On 2015/07/28 06:07:11, sof wrote:
> > > > On 2015/07/28 06:02:06, haraken wrote:
> > > > > On 2015/07/28 05:41:13, sof wrote:
> > > > > > haraken@: what benefit does freed-mem zapping bring if *San is
> enabled?
> > > > (i.e.,
> > > > > > would it be sufficient to only use it for "simple" debug builds.)
> > > > > 
> > > > > The zapping is necessary to delay reusing freed regions. This is
> > meaningful
> > > in
> > > > > ASan to detect use-after-frees.
> > > > > 
> > > > > (I'm OK with just disabling it in MSan.)
> > > > 
> > > > But isn't the free-mem poisoning that we already do with ASan enabled
more
> > > > stringent than that already?
> > > 
> > > The reuse delay is a more stringent verification :)
> > > 
> > > If we don't have the reuse delay, the freed region is reused in the next
GC
> > > cycle. The freed region is poisoned only for one GC cycle. If we have the
> > reuse
> > > delay, the freed region is not reused in the next GC cycle. The freed
region
> > is
> > > poisoned for a couple of GC cycles.
> > 
> > Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after these
zapped
> > regions have been touched. (=> ASan will flag any attempts to access them
> > first.)
> 
> Without zapping the region with reuseForbiddenZapValue, how can we know when
the
> freed memory is OK to reuse? Zapping doesn't matter, the zapped values matter.

..but if you call ASAN_POISON_MEMORY_REGION() just after always, how are those
zapped values observable - wouldn't ASan complain either way?

[haraken, 2015-07-28 06:48:58]

On 2015/07/28 06:39:20, sof wrote:
> On 2015/07/28 06:33:26, haraken wrote:
> > On 2015/07/28 06:18:36, sof wrote:
> > > On 2015/07/28 06:11:18, haraken wrote:
> > > > On 2015/07/28 06:07:11, sof wrote:
> > > > > On 2015/07/28 06:02:06, haraken wrote:
> > > > > > On 2015/07/28 05:41:13, sof wrote:
> > > > > > > haraken@: what benefit does freed-mem zapping bring if *San is
> > enabled?
> > > > > (i.e.,
> > > > > > > would it be sufficient to only use it for "simple" debug builds.)
> > > > > > 
> > > > > > The zapping is necessary to delay reusing freed regions. This is
> > > meaningful
> > > > in
> > > > > > ASan to detect use-after-frees.
> > > > > > 
> > > > > > (I'm OK with just disabling it in MSan.)
> > > > > 
> > > > > But isn't the free-mem poisoning that we already do with ASan enabled
> more
> > > > > stringent than that already?
> > > > 
> > > > The reuse delay is a more stringent verification :)
> > > > 
> > > > If we don't have the reuse delay, the freed region is reused in the next
> GC
> > > > cycle. The freed region is poisoned only for one GC cycle. If we have
the
> > > reuse
> > > > delay, the freed region is not reused in the next GC cycle. The freed
> region
> > > is
> > > > poisoned for a couple of GC cycles.
> > > 
> > > Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after these
> zapped
> > > regions have been touched. (=> ASan will flag any attempts to access them
> > > first.)
> > 
> > Without zapping the region with reuseForbiddenZapValue, how can we know when
> the
> > freed memory is OK to reuse? Zapping doesn't matter, the zapped values
matter.
> 
> ..but if you call ASAN_POISON_MEMORY_REGION() just after always, how are those
> zapped values observable - wouldn't ASan complain either way?

Maybe I'm misunderstanding how ASan works. Does ASAN_POISON_MEMORY_REGION()
overwrite the values in the region? I was assuming that
ASAN_POISON_MEMORY_REGION() just registers the address range as poisoned without
touching the values in the region.

[sof, 2015-07-28 06:51:22]

On 2015/07/28 06:48:58, haraken wrote:
> On 2015/07/28 06:39:20, sof wrote:
> > On 2015/07/28 06:33:26, haraken wrote:
> > > On 2015/07/28 06:18:36, sof wrote:
> > > > On 2015/07/28 06:11:18, haraken wrote:
> > > > > On 2015/07/28 06:07:11, sof wrote:
> > > > > > On 2015/07/28 06:02:06, haraken wrote:
> > > > > > > On 2015/07/28 05:41:13, sof wrote:
> > > > > > > > haraken@: what benefit does freed-mem zapping bring if *San is
> > > enabled?
> > > > > > (i.e.,
> > > > > > > > would it be sufficient to only use it for "simple" debug
builds.)
> > > > > > > 
> > > > > > > The zapping is necessary to delay reusing freed regions. This is
> > > > meaningful
> > > > > in
> > > > > > > ASan to detect use-after-frees.
> > > > > > > 
> > > > > > > (I'm OK with just disabling it in MSan.)
> > > > > > 
> > > > > > But isn't the free-mem poisoning that we already do with ASan
enabled
> > more
> > > > > > stringent than that already?
> > > > > 
> > > > > The reuse delay is a more stringent verification :)
> > > > > 
> > > > > If we don't have the reuse delay, the freed region is reused in the
next
> > GC
> > > > > cycle. The freed region is poisoned only for one GC cycle. If we have
> the
> > > > reuse
> > > > > delay, the freed region is not reused in the next GC cycle. The freed
> > region
> > > > is
> > > > > poisoned for a couple of GC cycles.
> > > > 
> > > > Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after these
> > zapped
> > > > regions have been touched. (=> ASan will flag any attempts to access
them
> > > > first.)
> > > 
> > > Without zapping the region with reuseForbiddenZapValue, how can we know
when
> > the
> > > freed memory is OK to reuse? Zapping doesn't matter, the zapped values
> matter.
> > 
> > ..but if you call ASAN_POISON_MEMORY_REGION() just after always, how are
those
> > zapped values observable - wouldn't ASan complain either way?
> 
> Maybe I'm misunderstanding how ASan works. Does ASAN_POISON_MEMORY_REGION()
> overwrite the values in the region? I was assuming that
> ASAN_POISON_MEMORY_REGION() just registers the address range as poisoned
without
> touching the values in the region.

The poisoning is shadowed and not overwritten. But does that matter here? ASan
will catch accesses to either kind of (Oilpan) zapped free memory.

[haraken, 2015-07-28 06:56:35]

On 2015/07/28 06:51:22, sof wrote:
> On 2015/07/28 06:48:58, haraken wrote:
> > On 2015/07/28 06:39:20, sof wrote:
> > > On 2015/07/28 06:33:26, haraken wrote:
> > > > On 2015/07/28 06:18:36, sof wrote:
> > > > > On 2015/07/28 06:11:18, haraken wrote:
> > > > > > On 2015/07/28 06:07:11, sof wrote:
> > > > > > > On 2015/07/28 06:02:06, haraken wrote:
> > > > > > > > On 2015/07/28 05:41:13, sof wrote:
> > > > > > > > > haraken@: what benefit does freed-mem zapping bring if *San is
> > > > enabled?
> > > > > > > (i.e.,
> > > > > > > > > would it be sufficient to only use it for "simple" debug
> builds.)
> > > > > > > > 
> > > > > > > > The zapping is necessary to delay reusing freed regions. This is
> > > > > meaningful
> > > > > > in
> > > > > > > > ASan to detect use-after-frees.
> > > > > > > > 
> > > > > > > > (I'm OK with just disabling it in MSan.)
> > > > > > > 
> > > > > > > But isn't the free-mem poisoning that we already do with ASan
> enabled
> > > more
> > > > > > > stringent than that already?
> > > > > > 
> > > > > > The reuse delay is a more stringent verification :)
> > > > > > 
> > > > > > If we don't have the reuse delay, the freed region is reused in the
> next
> > > GC
> > > > > > cycle. The freed region is poisoned only for one GC cycle. If we
have
> > the
> > > > > reuse
> > > > > > delay, the freed region is not reused in the next GC cycle. The
freed
> > > region
> > > > > is
> > > > > > poisoned for a couple of GC cycles.
> > > > > 
> > > > > Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after
these
> > > zapped
> > > > > regions have been touched. (=> ASan will flag any attempts to access
> them
> > > > > first.)
> > > > 
> > > > Without zapping the region with reuseForbiddenZapValue, how can we know
> when
> > > the
> > > > freed memory is OK to reuse? Zapping doesn't matter, the zapped values
> > matter.
> > > 
> > > ..but if you call ASAN_POISON_MEMORY_REGION() just after always, how are
> those
> > > zapped values observable - wouldn't ASan complain either way?
> > 
> > Maybe I'm misunderstanding how ASan works. Does ASAN_POISON_MEMORY_REGION()
> > overwrite the values in the region? I was assuming that
> > ASAN_POISON_MEMORY_REGION() just registers the address range as poisoned
> without
> > touching the values in the region.
> 
> The poisoning is shadowed and not overwritten. But does that matter here? ASan
> will catch accesses to either kind of (Oilpan) zapped free memory.

Yes, but without distinguishing the zapped values (reuseForbiddenZapValue or
reuseAllowedZapValue), we can't delay reusing the free list, can we?

What we want to verify here is:

1) Lazy sweeping adds the region A to the free list. The region A is poisoned.
It is possible that someone X keeps a pointer to the region A.
2) outOfLineAllocate finds the region A. The region A is unpoisoned. A new
object is allocated on the region A.
3) X accesses the new object. This is not detected as use-after-free.

This was happening in some crashes (around ImageLoader?) when we enabled lazy
sweeping.

[sof, 2015-07-28 07:02:33]

On 2015/07/28 06:56:35, haraken wrote:
> On 2015/07/28 06:51:22, sof wrote:
> > On 2015/07/28 06:48:58, haraken wrote:
> > > On 2015/07/28 06:39:20, sof wrote:
> > > > On 2015/07/28 06:33:26, haraken wrote:
> > > > > On 2015/07/28 06:18:36, sof wrote:
> > > > > > On 2015/07/28 06:11:18, haraken wrote:
> > > > > > > On 2015/07/28 06:07:11, sof wrote:
> > > > > > > > On 2015/07/28 06:02:06, haraken wrote:
> > > > > > > > > On 2015/07/28 05:41:13, sof wrote:
> > > > > > > > > > haraken@: what benefit does freed-mem zapping bring if *San
is
> > > > > enabled?
> > > > > > > > (i.e.,
> > > > > > > > > > would it be sufficient to only use it for "simple" debug
> > builds.)
> > > > > > > > > 
> > > > > > > > > The zapping is necessary to delay reusing freed regions. This
is
> > > > > > meaningful
> > > > > > > in
> > > > > > > > > ASan to detect use-after-frees.
> > > > > > > > > 
> > > > > > > > > (I'm OK with just disabling it in MSan.)
> > > > > > > > 
> > > > > > > > But isn't the free-mem poisoning that we already do with ASan
> > enabled
> > > > more
> > > > > > > > stringent than that already?
> > > > > > > 
> > > > > > > The reuse delay is a more stringent verification :)
> > > > > > > 
> > > > > > > If we don't have the reuse delay, the freed region is reused in
the
> > next
> > > > GC
> > > > > > > cycle. The freed region is poisoned only for one GC cycle. If we
> have
> > > the
> > > > > > reuse
> > > > > > > delay, the freed region is not reused in the next GC cycle. The
> freed
> > > > region
> > > > > > is
> > > > > > > poisoned for a couple of GC cycles.
> > > > > > 
> > > > > > Hmm, I see nothing but calls to ASAN_POISON_MEMORY_REGION() after
> these
> > > > zapped
> > > > > > regions have been touched. (=> ASan will flag any attempts to access
> > them
> > > > > > first.)
> > > > > 
> > > > > Without zapping the region with reuseForbiddenZapValue, how can we
know
> > when
> > > > the
> > > > > freed memory is OK to reuse? Zapping doesn't matter, the zapped values
> > > matter.
> > > > 
> > > > ..but if you call ASAN_POISON_MEMORY_REGION() just after always, how are
> > those
> > > > zapped values observable - wouldn't ASan complain either way?
> > > 
> > > Maybe I'm misunderstanding how ASan works. Does
ASAN_POISON_MEMORY_REGION()
> > > overwrite the values in the region? I was assuming that
> > > ASAN_POISON_MEMORY_REGION() just registers the address range as poisoned
> > without
> > > touching the values in the region.
> > 
> > The poisoning is shadowed and not overwritten. But does that matter here?
ASan
> > will catch accesses to either kind of (Oilpan) zapped free memory.
> 
> Yes, but without distinguishing the zapped values (reuseForbiddenZapValue or
> reuseAllowedZapValue), we can't delay reusing the free list, can we?
> 
> What we want to verify here is:
> 
> 1) Lazy sweeping adds the region A to the free list. The region A is poisoned.
> It is possible that someone X keeps a pointer to the region A.
> 2) outOfLineAllocate finds the region A. The region A is unpoisoned. A new
> object is allocated on the region A.
> 3) X accesses the new object. This is not detected as use-after-free.
> 
> This was happening in some crashes (around ImageLoader?) when we enabled lazy
> sweeping.

If this catches that scenario with some build configuration, great (but was
there really such a delay in that particular case?)

I just don't get it what this provides if you additionally have ASan poisoning
of all freed regions, but will put the matter to the side for now.

[kojii, 2015-07-29 06:42:15]

PTAL. Reverted to PS3. This CL:
1. is likely a "harmless though not ideal".
2. glider@ pointed out in comment #16 that MSAN may not be able to detect reads
of freed memory.

Turned out that, IIUC, 2 is rather not a simple work that can involve fixing
other parts of Heap.cpp and that I'd like to separate it. I'm happy to file a
bug if my understanding is correct.

I actually don't have a clear understanding of how bad 2 is, as it looks to me
that it's something ASAN can detect, but I admit I have little understanding of
the differences between the two, so I might be wrong.

If we were not happy with this CL and would like larger changes, I can seek for
how the Layout crasher fix can avoid hitting this issue. I have another way to
fix it, though it's less performant and uses more memory.

Thoughts from experts here appreciated.

[Alexander Potapenko, 2015-07-29 08:51:05]

Can you please elaborate which problem this CL fixes?
The description says "Fix FreeList::zapFreedMemory to fail
"use-of-uninitialized-value" on MSAN", but as far as I understand it's this CL
that enables use of zapFreedMemory() under MSan.

[Alexander Potapenko, 2015-07-29 08:51:07]

Can you please elaborate which problem this CL fixes?
The description says "Fix FreeList::zapFreedMemory to fail
"use-of-uninitialized-value" on MSAN", but as far as I understand it's this CL
that enables use of zapFreedMemory() under MSan.

[kojii, 2015-07-29 08:57:23]

On 2015/07/29 08:51:07, Alexander Potapenko wrote:
> Can you please elaborate which problem this CL fixes?
> The description says "Fix FreeList::zapFreedMemory to fail
> "use-of-uninitialized-value" on MSAN", but as far as I understand it's this CL
> that enables use of zapFreedMemory() under MSan.

This CL fixes a layout crasher:
https://codereview.chromium.org/1244973003/

You can see PS4 fails MSAN bot:
MemorySanitizer: use-of-uninitialized-value
    #0 0x7f782cc84a6e in blink::FreeList::zapFreedMemory

and PS5 adds "__attribute__((no_sanitize_memory))" to zapFreeMemory and passes
MSAN bot.

[Alexander Potapenko, 2015-07-29 09:14:14]

On 2015/07/29 08:57:23, kojii wrote:
> On 2015/07/29 08:51:07, Alexander Potapenko wrote:
> > Can you please elaborate which problem this CL fixes?
> > The description says "Fix FreeList::zapFreedMemory to fail
> > "use-of-uninitialized-value" on MSAN", but as far as I understand it's this
CL
> > that enables use of zapFreedMemory() under MSan.
> 
> This CL fixes a layout crasher:
> https://codereview.chromium.org/1244973003/
> 
> You can see PS4 fails MSAN bot:
> MemorySanitizer: use-of-uninitialized-value
>     #0 0x7f782cc84a6e in blink::FreeList::zapFreedMemory
> 
> and PS5 adds "__attribute__((no_sanitize_memory))" to zapFreeMemory and passes
> MSAN bot.

Sorry, I must be missing something.
This CL (https://codereview.chromium.org/1259893002) adds "if
defined(MEMORY_SANITIZER)" to the definition of SET_MEMORY_INACCESSIBLE(). Prior
to this CL there were no calls to zapFreeMemory() under MSan, correct?

[kojii, 2015-07-29 09:36:45]

On 2015/07/29 09:14:14, Alexander Potapenko wrote:
> On 2015/07/29 08:57:23, kojii wrote:
> > On 2015/07/29 08:51:07, Alexander Potapenko wrote:
> > > Can you please elaborate which problem this CL fixes?
> > > The description says "Fix FreeList::zapFreedMemory to fail
> > > "use-of-uninitialized-value" on MSAN", but as far as I understand it's
this
> CL
> > > that enables use of zapFreedMemory() under MSan.
> > 
> > This CL fixes a layout crasher:
> > https://codereview.chromium.org/1244973003/
> > 
> > You can see PS4 fails MSAN bot:
> > MemorySanitizer: use-of-uninitialized-value
> >     #0 0x7f782cc84a6e in blink::FreeList::zapFreedMemory
> > 
> > and PS5 adds "__attribute__((no_sanitize_memory))" to zapFreeMemory and
passes
> > MSAN bot.
> 
> Sorry, I must be missing something.
> This CL (https://codereview.chromium.org/1259893002) adds "if
> defined(MEMORY_SANITIZER)" to the definition of SET_MEMORY_INACCESSIBLE().
Prior
> to this CL there were no calls to zapFreeMemory() under MSan, correct?

I have to admit that I skipped really looking into it, but zapFreedMemory is
ifdef'ed for ASSERT and LEAK_SANITIZER too, I suspect MEMORY_SANITIZER implies
either of these and thus called.

[Alexander Potapenko, 2015-07-29 10:05:10]

On 2015/07/29 09:36:45, kojii wrote:
> On 2015/07/29 09:14:14, Alexander Potapenko wrote:
> > On 2015/07/29 08:57:23, kojii wrote:
> > > On 2015/07/29 08:51:07, Alexander Potapenko wrote:
> > > > Can you please elaborate which problem this CL fixes?
> > > > The description says "Fix FreeList::zapFreedMemory to fail
> > > > "use-of-uninitialized-value" on MSAN", but as far as I understand it's
> this
> > CL
> > > > that enables use of zapFreedMemory() under MSan.
> > > 
> > > This CL fixes a layout crasher:
> > > https://codereview.chromium.org/1244973003/
> > > 
> > > You can see PS4 fails MSAN bot:
> > > MemorySanitizer: use-of-uninitialized-value
> > >     #0 0x7f782cc84a6e in blink::FreeList::zapFreedMemory
> > > 
> > > and PS5 adds "__attribute__((no_sanitize_memory))" to zapFreeMemory and
> passes
> > > MSAN bot.
> > 
> > Sorry, I must be missing something.
> > This CL (https://codereview.chromium.org/1259893002) adds "if
> > defined(MEMORY_SANITIZER)" to the definition of SET_MEMORY_INACCESSIBLE().
> Prior
> > to this CL there were no calls to zapFreeMemory() under MSan, correct?
> 
> I have to admit that I skipped really looking into it, but zapFreedMemory is
> ifdef'ed for ASSERT and LEAK_SANITIZER too, I suspect MEMORY_SANITIZER implies
> either of these and thus called.
I think MSan should not imply LSan, so this is probably because of ASSERT.
The quick workaround would be to just disable zapFreedMemory() under MSan at
all.
But since we ultimately want the reuse of freed memory to be delayed in MSan
builds, we can go with PS4 without __msan_[un]poison for now.
PS5 is almost good, but it's conceptually wrong to have
ASAN_POISON_MEMORY_REGION under "if defined(MEMORY_SANITIZER)" - as it turns
out, it confuses people not familiar with ASan/MSan features.
defined(MEMORY_SANITIZER)

[kojii, 2015-07-29 10:30:53]

On 2015/07/29 10:05:10, Alexander Potapenko wrote:
> > 
> > I have to admit that I skipped really looking into it, but zapFreedMemory is
> > ifdef'ed for ASSERT and LEAK_SANITIZER too, I suspect MEMORY_SANITIZER
implies
> > either of these and thus called.
> I think MSan should not imply LSan, so this is probably because of ASSERT.
> The quick workaround would be to just disable zapFreedMemory() under MSan at
> all.
> But since we ultimately want the reuse of freed memory to be delayed in MSan
> builds, we can go with PS4 without __msan_[un]poison for now.
> PS5 is almost good, but it's conceptually wrong to have
> ASAN_POISON_MEMORY_REGION under "if defined(MEMORY_SANITIZER)" - as it turns
> out, it confuses people not familiar with ASan/MSan features.
> defined(MEMORY_SANITIZER)

Thank you for your prompt and kind reply. I was looking into gyp and build logs
but it looks I can't find which defines are implied in MSAN builds from these.

Trying your advice now.

[kojii, 2015-07-29 13:36:17]

PTAL. All bots passed, including ASAN and MSAN.

Hope I understand what you meant correctly.

[Alexander Potapenko, 2015-07-29 13:39:42]

LGTM (please add a TODO before submitting)

https://codereview.chromium.org/1259893002/diff/100001/Source/platform/heap/H...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1259893002/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.h:83: FreeList::zapFreedMemory(address, size);
Please add a TODO here indicating that we actually do need MSan poisoning, but
it'll be added later.

https://codereview.chromium.org/1259893002/diff/100001/Source/wtf/AddressSani...
File Source/wtf/AddressSanitizer.h (right):

https://codereview.chromium.org/1259893002/diff/100001/Source/wtf/AddressSani...
Source/wtf/AddressSanitizer.h:6: #define WTF_AddressSanitizer_h
This file will need to be renamed, because it's no more specific to
AddressSanitizer.

[kojii, 2015-07-29 14:30:06]

Added TODO comments.

Thank you so much for this long and deep support.

On behalf of the Layout team. We were almost giving up landing the CL, glider@,
haraken@, yutak@ you all helped us so much!

https://codereview.chromium.org/1259893002/diff/100001/Source/platform/heap/H...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1259893002/diff/100001/Source/platform/heap/H...
Source/platform/heap/Heap.h:83: FreeList::zapFreedMemory(address, size);
On 2015/07/29 13:39:42, Alexander Potapenko wrote:
> Please add a TODO here indicating that we actually do need MSan poisoning, but
> it'll be added later.

Done.

https://codereview.chromium.org/1259893002/diff/100001/Source/wtf/AddressSani...
File Source/wtf/AddressSanitizer.h (right):

https://codereview.chromium.org/1259893002/diff/100001/Source/wtf/AddressSani...
Source/wtf/AddressSanitizer.h:6: #define WTF_AddressSanitizer_h
On 2015/07/29 13:39:42, Alexander Potapenko wrote:
> This file will need to be renamed, because it's no more specific to
> AddressSanitizer.

Done.

[haraken, 2015-07-29 14:40:50]

Thanks for driving this effort! platform/heap/ LGTM.

TODO => TODO(someone's name)

[kojii, 2015-07-29 16:35:16]

The CQ bit was checked by kojii@chromium.org

[kojii, 2015-07-29 16:35:16]

The patchset sent to the CQ was uploaded after l-g-t-m from glider@chromium.org,
haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1259893002/#ps140001
(title: "Changed TODO with name")

[commit-bot: I haz the power, 2015-07-29 16:35:35]

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

[commit-bot: I haz the power, 2015-07-29 16:40:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-07-29 16:40:28]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/3...)

[kojii, 2015-07-29 16:46:01]

kojii@chromium.org changed reviewers:
+ tkent@chromium.org

[kojii, 2015-07-29 16:46:02]

tkent@, could you PTAL for Source/wtf/AddressSanitizer.h?

[tkent, 2015-07-29 22:28:15]

https://codereview.chromium.org/1259893002/diff/140001/Source/platform/heap/H...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1259893002/diff/140001/Source/platform/heap/H...
Source/platform/heap/Heap.h:82: // TODO(tkent): We actually need
__msan_poison/unpoison here, but it'll be
Why TODO(tkent)?

[kojii, 2015-07-30 02:08:35]

On 2015/07/29 22:28:15, tkent wrote:
>
https://codereview.chromium.org/1259893002/diff/140001/Source/platform/heap/H...
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/1259893002/diff/140001/Source/platform/heap/H...
> Source/platform/heap/Heap.h:82: // TODO(tkent): We actually need
> __msan_poison/unpoison here, but it'll be
> Why TODO(tkent)?

Fixed. Learned TODO name is who wrote it, not who signed up to do it in future,
thank you.

[tkent, 2015-07-30 02:26:06]

The CQ bit was checked by tkent@chromium.org

[tkent, 2015-07-30 02:26:07]

lgtm

[tkent, 2015-07-30 02:26:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, glider@chromium.org
Link to the patchset: https://codereview.chromium.org/1259893002/#ps160001
(title: "Change TODO name")

[commit-bot: I haz the power, 2015-07-30 02:26:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1259893002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1259893002/160001

[commit-bot: I haz the power, 2015-07-30 03:22:54]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199701

[kojii, 2015-07-30 04:27:58]

A revert of this CL (patchset #9 id:160001) has been created in
https://codereview.chromium.org/1257303003/ by kojii@chromium.org.

The reason for reverting is: Strange, it passed MSAN try bot, but not the MSAN
bots on waterfall...reverting..

[kojii, 2015-07-30 04:30:57]

FAILED: /b/build/goma/gomacc
../../third_party/llvm-build/Release+Asserts/bin/clang++ -MMD -MF
obj/third_party/WebKit/Source/platform/heap/blink_platform.Heap.o.d
-DV8_DEPRECATION_WARNINGS -D_FILE_OFFSET_BITS=64 -DNO_TCMALLOC -DCHROMIUM_BUILD
-DCR_CLANG_REVISION=242792-1 -DUI_COMPOSITOR_IMAGE_TRANSPORT -DUSE_AURA=1
-DUSE_ASH=1 -DUSE_PANGO=1 -DUSE_CAIRO=1 -DUSE_DEFAULT_RENDER_THEME=1
-DUSE_LIBJPEG_TURBO=1 -DUSE_X11=1 -DUSE_CLIPBOARD_AURAX11=1
-DENABLE_ONE_CLICK_SIGNIN -DENABLE_PRE_SYNC_BACKUP -DENABLE_REMOTING=1
-DENABLE_WEBRTC=1 -DENABLE_MEDIA_ROUTER=1 -DENABLE_PEPPER_CDMS
-DENABLE_CONFIGURATION_POLICY -DENABLE_NOTIFICATIONS -DENABLE_HIDPI=1
-DENABLE_TOPCHROME_MD=1 -DUSE_UDEV -DDONT_EMBED_BUILD_METADATA
-DENABLE_TASK_MANAGER=1 -DENABLE_EXTENSIONS=1 -DENABLE_PLUGINS=1
-DENABLE_SESSION_SERVICE=1 -DENABLE_THEMES=1 -DENABLE_AUTOFILL_DIALOG=1
-DENABLE_BACKGROUND=1 -DENABLE_GOOGLE_NOW=1 -DCLD_VERSION=2 -DENABLE_PRINTING=1
-DENABLE_BASIC_PRINTING=1 -DENABLE_PRINT_PREVIEW=1 -DENABLE_SPELLCHECK=1
-DENABLE_CAPTIVE_PORTAL_DETECTION=1 -DENABLE_APP_LIST=1 -DENABLE_SETTINGS_APP=1
-DENABLE_SUPERVISED_USERS=1 -DENABLE_MDNS=1 -DENABLE_SERVICE_DISCOVERY=1
-DV8_USE_EXTERNAL_STARTUP_DATA -DFULL_SAFE_BROWSING -DSAFE_BROWSING_CSD
-DSAFE_BROWSING_DB_LOCAL -DSAFE_BROWSING_SERVICE
-DBLINK_PLATFORM_IMPLEMENTATION=1 -DINSIDE_BLINK
-DENABLE_LAYOUT_UNIT_IN_INLINE_BOXES=0
-DWTF_USE_CONCATENATED_IMPULSE_RESPONSES=1 -DENABLE_INPUT_MULTIPLE_FIELDS_UI=1
-DENABLE_WEB_AUDIO=1 -DWTF_USE_WEBAUDIO_FFMPEG=1
-DWTF_USE_DEFAULT_RENDER_THEME=1 -DENABLE_LAZY_SWEEPING=1 -DENABLE_IDLE_GC=1
-DSK_SUPPORT_GPU=1 -DSK_IGNORE_LINEONLY_AA_CONVEX_PATH_OPTS
-DSK_LEGACY_SKPOINT3_CTORS -DU_USING_ICU_NAMESPACE=0 -DU_ENABLE_DYLOAD=0
-DU_STATIC_IMPLEMENTATION -DCHROME_PNG_WRITE_SUPPORT -DPNG_USER_CONFIG
-DUSE_LIBPCI=1 -DUSE_OPENSSL=1 -DUSE_GLIB=1 -DUSE_NSS_CERTS=1
-D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -DMEMORY_TOOL_REPLACES_ALLOCATOR
-DMEMORY_SANITIZER_INITIAL_SIZE -DMEMORY_SANITIZER -DNDEBUG -DNVALGRIND
-DDYNAMIC_ANNOTATIONS_ENABLED=0 -Igen -I../../third_party/angle/include
-Igen/blink -I../../third_party/ffmpeg -I../../third_party/WebKit/Source -I../..
-I../../skia/config -I../../third_party/khronos -I../../gpu
-I../../third_party/WebKit -I../../third_party/skia/include/core
-I../../third_party/skia/include/effects -I../../third_party/skia/include/pdf
-I../../third_party/skia/include/gpu -I../../third_party/skia/include/lazy
-I../../third_party/skia/include/pathops -I../../third_party/skia/include/pipe
-I../../third_party/skia/include/ports -I../../third_party/skia/include/utils
-I../../skia/ext -I../../third_party/icu/source/i18n
-I../../third_party/icu/source/common -I../../third_party/libpng
-I../../third_party/zlib -I../../third_party/libwebp
-I../../third_party/ots/include -I../../third_party/qcms/src -I../../v8/include
-I../../third_party/iccjpeg -I../../third_party/libjpeg_turbo
-I../../third_party/harfbuzz-ng/src
-I../../third_party/ffmpeg/chromium/config/Chromium/linux-noasm/x64
-I../../third_party/ffmpeg '-I../../buildtools/third_party/libc++/trunk/include'
'-I../../buildtools/third_party/libc++abi/trunk/include' -fstack-protector
--param=ssp-buffer-size=4 -Werror -pthread -fno-strict-aliasing -Wall
-Wno-unused-parameter -Wno-missing-field-initializers -fvisibility=hidden -pipe
-fPIC -Wno-reserved-user-defined-literal -Xclang -load -Xclang
/b/build/slave/WebKit_Linux_MSAN/build/src/third_party/llvm-build/Release+Asserts/lib/libFindBadConstructs.so
-Xclang -add-plugin -Xclang find-bad-constructs -Xclang
-plugin-arg-find-bad-constructs -Xclang check-templates -fcolor-diagnostics
-B/b/build/slave/WebKit_Linux_MSAN/build/src/third_party/binutils/Linux_x64/Release/bin
-Wheader-hygiene -Wno-char-subscripts -Wno-unneeded-internal-declaration
-Wno-covered-switch-default -Wstring-conversion -Wno-c++11-narrowing
-Wno-deprecated-register -Wno-inconsistent-missing-override
-Wno-shift-negative-value -Wglobal-constructors -Xclang -load -Xclang
/b/build/slave/WebKit_Linux_MSAN/build/src/third_party/llvm-build/Release+Asserts/lib/libBlinkGCPlugin.so
-Xclang -add-plugin -Xclang blink-gc-plugin -m64 -march=x86-64
-fno-omit-frame-pointer -gline-tables-only -fsanitize=memory
-fsanitize-memory-track-origins=2
-fsanitize-blacklist=../../tools/msan/blacklist.txt -O2 -fno-ident
-fdata-sections -ffunction-sections -funwind-tables -fno-exceptions -fno-rtti
-fno-threadsafe-statics -fvisibility-inlines-hidden -Wsign-compare -std=gnu++11
-nostdinc++  -c ../../third_party/WebKit/Source/platform/heap/Heap.cpp -o
obj/third_party/WebKit/Source/platform/heap/blink_platform.Heap.o
../../third_party/WebKit/Source/platform/heap/Heap.cpp:713:17: error: no member
named 'zapFreedMemory' in 'blink::FreeList'
                SET_MEMORY_INACCESSIBLE(headerAddress,
sizeof(HeapObjectHeader));
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../third_party/WebKit/Source/platform/heap/Heap.h:85:15: note: expanded from
macro 'SET_MEMORY_INACCESSIBLE'
    FreeList::zapFreedMemory(address, size);
    ~~~~~~~~~~^

[kojii, 2015-07-30 04:36:31]

Patchset #10 (id:180001) has been deleted

[Alexander Potapenko, 2015-07-30 08:53:49]

> Fixed. Learned TODO name is who wrote it, not who signed up to do it in
future,
> thank you.

Per C++ Style Guide
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments):

> TODOs should include the string TODO in all caps, followed by the name, e-mail
address,
> or other identifier of the person with the best context about the problem
referenced by
> the TODO. The main purpose is to have a consistent TODO that can be searched
to find out
> how to get more details upon request. A TODO is not a commitment that the
person
> referenced will fix the problem. Thus when you create a TODO, it is almost
always your
> name that is given.

So this isn't necessarily the person who wrote it, but could be the person who
signed up to do it in future, if he/she has enough context.

[kojii, 2015-07-30 12:54:50]

On 2015/07/30 08:53:49, Alexander Potapenko wrote:
> > Fixed. Learned TODO name is who wrote it, not who signed up to do it in
> future,
> > thank you.
> 
> Per C++ Style Guide
>
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#TODO_Comments):
> 
> > TODOs should include the string TODO in all caps, followed by the name,
e-mail
> address,
> > or other identifier of the person with the best context about the problem
> referenced by
> > the TODO. The main purpose is to have a consistent TODO that can be searched
> to find out
> > how to get more details upon request. A TODO is not a commitment that the
> person
> > referenced will fix the problem. Thus when you create a TODO, it is almost
> always your
> > name that is given.
> 
> So this isn't necessarily the person who wrote it, but could be the person who
> signed up to do it in future, if he/she has enough context.

Thank you, so my name works better. I don't have a confidence to fix this
properly with my current knowledge on ASAN/MSAN/memory-related code in Blink,
but I'm happy to be referred (though maybe I just have to forward them to you
;-)

The re-land passed. Thank you so much again for the support. With this landed, I
confirmed that the layout crasher fix passes the MSAN try-bots, I can try to
land that too.

(/me wonder, you all are working for 24 hours?)

[commit-bot: I haz the power, 2015-09-23 11:55:01]

Patchset 9 (id:??) landed as
https://crrev.com/021efec808267f79934050714f4732961fe14c47