663386 - blacklist FixedAlloc-managed memory detection is fragile/wrong

Status: RESOLVED WONTFIX

(Tamarin Graveyard :: Garbage Collection (mmGC), defect, P4)

Description

[Felix S. Klock II [:pnkfelix, :fklock]]

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.

Comment 1

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: use FixedMalloc::FindBeginning to do the fallback lookup

Comment 2

[Felix S. Klock II [:pnkfelix, :fklock]]

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.

Comment 3

[Dan Smith]

Looks ready to land?  Can this land during i9?

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

I do not trust FindBeginning 100% in debug mode, due to bug 735121.  So let us fix that first.

Comment 5

[huge514]

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 6

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 8

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Tamarin is a dead project now. Mass WONTFIX.

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Tamarin isn't maintained anymore. WONTFIX remaining bugs.