Closed
Bug 663386
Opened 14 years ago
Closed 6 years ago
blacklist FixedAlloc-managed memory detection is fragile/wrong
Categories
(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P4)
Tamarin Graveyard
Garbage Collection (mmGC)
Tracking
(Not tracked)
RESOLVED
WONTFIX
Q1 12 - Brannan
People
(Reporter: pnkfelix, Unassigned)
References
Details
(Whiteboard: has-patch, loose-end)
Attachments
(1 file)
2.24 KB,
patch
|
treilly
:
review+
lhansen
:
superreview+
|
Details | Diff | Splinter Review |
The blacklist code makes use of GC::findGCGraphBeginning.
This code, in a (presumably rare) corner case, tries to determine whether the given address addr belongs to a small object, and thus managed by FixedAlloc (and thus the function should use FixedAlloc::FindBeginning to determine the object's start), or if the addr belongs to a large object and thus not managed by FixedAlloc.
The logic it uses is this:
GCHeap::HeapBlock *b = heap->AddrToBlock(addr);
if(b) {
wasDeletedGCRoot = true;
if(b->inUse()) {
if(b->size == 1) {
return FixedAlloc::FindBeginning(addr);
} else {
return GetUserPointer(b->baseAddr);
}
}
}
The problem is that this will not make the correct choice for middle-sized objects where size > FixedMalloc::kLargestAlloc but size <= GCHeap::kBlockSize. kLargestAlloc is either 2016 or 2032 (for 64- and 32-bit systems, respectively). kBlockSize is 4096.
(I would love to claim that I discovered this by reviewing the code, but I do not have such eagle eyes. Rather, I ran into the issue while working on Bug 663177: it turns out that sizeof(avmshell::ShellCoreImpl) == 2576.)
Anyway, Tommy suggested that I lift the blacklist's findGCGraphBeginning logic to FixedMalloc as part of the work on Bug 663177; I'll work on fixing this bug at the same time.
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → fklockii
Reporter | ||
Comment 1•14 years ago
|
||
Reporter | ||
Updated•14 years ago
|
Whiteboard: has-patch
Reporter | ||
Comment 2•14 years ago
|
||
Comment on attachment 539323 [details] [diff] [review]
use FixedMalloc::FindBeginning to do the fallback lookup
The "friend class GCRoot" declaration here actually belongs with attachment 539548 [details] [diff] [review] (on Bug 664137, the GCRoot beginning derivation bug). I put it here because this patch came earlier in my patch queue, but assuming I end up pushing the GCRoot fix first, then obviously the friend declaration will go there as well.
I thought when I first started on this code that I would be able to get rid of the loop over m_roots, but that's not true, because to support wasDeletedGCRoot one still needs to traverse the roots. That's okay though, its not a big deal if the MMGC_HEAP_GRAPH/blacklist code has some extra computational overhead.
Attachment #539323 -
Flags: superreview?(lhansen)
Attachment #539323 -
Flags: review?(treilly)
Reporter | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Updated•14 years ago
|
Attachment #539323 -
Flags: review?(treilly) → review+
Updated•14 years ago
|
Attachment #539323 -
Flags: superreview?(lhansen) → superreview+
Looks ready to land? Can this land during i9?
Flags: flashplayer-qrb+
Flags: flashplayer-injection-
Flags: flashplayer-bug+
Priority: -- → P4
Whiteboard: has-patch → has-patch, loose-end
Target Milestone: --- → Q1 12 - Brannan
Reporter | ||
Comment 4•13 years ago
|
||
I do not trust FindBeginning 100% in debug mode, due to bug 735121. So let us fix that first.
Reporter | ||
Updated•12 years ago
|
Assignee: fklockii → nobody
Comment on attachment 539323 [details] [diff] [review]
use FixedMalloc::FindBeginning to do the fallback lookup
># HG changeset patch
># Parent 601ba227d5cdaa8b5e1ce6de1219e47ff63b0401
>
>diff --git a/MMgc/FixedMalloc.h b/MMgc/FixedMalloc.h
>--- a/MMgc/FixedMalloc.h
>+++ b/MMgc/FixedMalloc.h
>@@ -55,16 +55,18 @@ namespace MMgc
> * The "owner" of the FixedMalloc is GCHeap, the block manager.
> *
> * FixedMalloc uses size classes; each size class is handled by a FixedAllocSafe
> * instance. Large objects are handled specially. All objects are headerless.
> */
> class FixedMalloc
> {
> friend class GCHeap;
>+ friend class GC;
>+ friend class GCRoot;
> friend class avmplus::ST_mmgc_bugzilla_663508::ST_mmgc_bugzilla_663508;
>
> public:
> /**
> * Obtain a FixedMalloc instance.
> *
> * @return the FixedMalloc singleton.
> */
>diff --git a/MMgc/GC.cpp b/MMgc/GC.cpp
>--- a/MMgc/GC.cpp
>+++ b/MMgc/GC.cpp
>@@ -3828,36 +3828,33 @@ namespace MMgc
> {
> /* These are all the possibilities
> 1) GC small object
> 2) GC large object
> 3) GCRoot
> 4) OS stack
> */
> if(!IsPointerToGCPage(addr)) {
>+ const void* begin =
>+ FixedMalloc::GetFixedMalloc()->FindBeginning(addr);
> GCRoot *r = m_roots;
> while(r) {
>- if(addr >= r->Get() && addr < r->End())
>+ if(addr >= r->Get() && addr < r->End()) {
>+ GCAssert(r->Get() == begin);
> return r->Get();
>+ }
> r = r->next;
> }
>
> // could be a deleted GCRoot
> GCHeap::HeapBlock *b = heap->InteriorAddrToBlock(addr);
> if(b) {
> wasDeletedGCRoot = true;
>- if(b->inUse()) {
>- if(b->size == 1) {
>- return FixedAlloc::FindBeginning(addr);
>- } else {
>- return GetUserPointer(b->baseAddr);
>- }
>- }
> }
>- return NULL;
>+ return begin;
> }
> return FindBeginningGuarded(addr, true); // will return NULL for OS stack
> }
>
> void GC::DumpBackPointerChain(const void *p)
> {
> dumpBackPointerChain(p, markerGraph);
> }
Comment 7•6 years ago
|
||
No assignee, updating the status.
Comment 8•6 years ago
|
||
No assignee, updating the status.
Comment 9•6 years ago
|
||
Tamarin is a dead project now. Mass WONTFIX.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → WONTFIX
Comment 10•6 years ago
|
||
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in
before you can comment on or make changes to this bug.
Description
•