Oilpan: Unpoison orphaned large objects before zapping (issue 1154733002 by haraken@chromium.org)

[har...@chromium.org]

Reviewers: oilpan-reviews, keishi,

Message:
PTAL


Description:
Oilpan: Unpoison orphaned large objects before zapping

To zap orphaned large objects, we need to unpoison the area.
Otherwise ASan detects the error, although it seems there has been
no orphaned large objects so far (by accident).

We're already doing that for orphaned normal pages.

BUG=420515

Please review this at https://codereview.chromium.org/1154733002/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+9, -0 lines):
M Source/platform/heap/Heap.cpp


Index: Source/platform/heap/Heap.cpp
diff --git a/Source/platform/heap/Heap.cpp b/Source/platform/heap/Heap.cpp
index
3fef547f1660fe8dc9a2fddf735d751d6e8f191d..2230a2dbd9867866b9eeb302dfd7c2845d9c6abb
100644
--- a/Source/platform/heap/Heap.cpp
+++ b/Source/platform/heap/Heap.cpp
@@ -544,6 +544,7 @@ void NormalPageHeap::freePage(NormalPage* page)
Heap::decreaseAllocatedSpace(page->size());

if (page->terminating()) {
+ ASSERT(ThreadState::current()->isTerminating());
// The thread is shutting down and this page is being removed as a
part
// of the thread local GC. In that case the object could be
traced in
// the next global GC if there is a dangling pointer from a live
thread
@@ -553,7 +554,9 @@ void NormalPageHeap::freePage(NormalPage* page)
// crashes instead of causing use-after-frees. After the next
global
// GC, the orphaned pages are removed.
Heap::orphanedPagePool()->addOrphanedPage(heapIndex(), page);
+ ASSERT(!page->terminating());
} else {
+ ASSERT(!ThreadState::current()->isTerminating());
PageMemory* memory = page->storage();
page->~NormalPage();
Heap::freePagePool()->addFreePage(heapIndex(), memory);
@@ -926,6 +929,7 @@ void
LargeObjectHeap::freeLargeObjectPage(LargeObjectPage* object)
// crashes instead of causing use-after-frees. After the next
global
// GC, the orphaned pages are removed.
Heap::orphanedPagePool()->addOrphanedPage(heapIndex(), object);
+ ASSERT(!object->terminating());
} else {
ASSERT(!ThreadState::current()->isTerminating());
PageMemory* memory = object->storage();
@@ -1487,6 +1491,11 @@ void LargeObjectPage::markOrphaned()
{
// Zap the payload with a recognizable value to detect any incorrect
// cross thread pointer usage.
+#if defined(ADDRESS_SANITIZER)
+ // This needs to zap poisoned memory as well.
+ // Force unpoison memory before memset.
+ ASAN_UNPOISON_MEMORY_REGION(payload(), payloadSize());
+#endif
memset(payload(), orphanedZapValue, payloadSize());
BasePage::markOrphaned();
}

[sigb...@opera.com]

https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (left):

https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp#oldcode1491
Source/platform/heap/Heap.cpp:1491: BasePage::markOrphaned();
Why isn't it re-poisoned afterwards?

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

https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp#newcode1497
Source/platform/heap/Heap.cpp:1497:
ASAN_UNPOISON_MEMORY_REGION(payload(), payloadSize());
If you pass (payload,size) as args to BasePage::markOrphaned(), you can
common up some code.

https://codereview.chromium.org/1154733002/

[har...@chromium.org]

https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp
> File Source/platform/heap/Heap.cpp (left):


https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp#oldcode1491
> Source/platform/heap/Heap.cpp:1491: BasePage::markOrphaned();
> Why isn't it re-poisoned afterwards?

Nice catch. done.

https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp
> File Source/platform/heap/Heap.cpp (right):


https://codereview.chromium.org/1154733002/diff/20001/Source/platform/heap/Heap.cpp#newcode1497
> Source/platform/heap/Heap.cpp:1497: ASAN_UNPOISON_MEMORY_REGION(payload(),
> payloadSize());
> If you pass (payload,size) as args to BasePage::markOrphaned(), you can
> common
> up some code.

Done.


https://codereview.chromium.org/1154733002/

[sigb...@opera.com]

lgtm



https://codereview.chromium.org/1154733002/

[commi...@chromium.org]

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


https://codereview.chromium.org/1154733002/

[commi...@chromium.org]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195782

https://codereview.chromium.org/1154733002/

[sigb...@opera.com]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/1146393004/ by sigb...@opera.com.

The reason for reverting is: Broke ASAN compilation,


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

https://codereview.chromium.org/1154733002/