blacklist FixedAlloc-managed memory detection is fragile/wrong

ASSIGNED
Unassigned

Status

Tamarin
Garbage Collection (mmGC)
P4
normal
ASSIGNED
7 years ago
4 years ago

People

(Reporter: pnkfelix, Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
Q1 12 - Brannan
Dependency tree / graph
Bug Flags:
flashplayer-injection -
flashplayer-qrb +
flashplayer-bug +

Details

(Whiteboard: has-patch, loose-end)

Attachments

(1 attachment)

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

7 years ago
Assignee: nobody → fklockii
(Reporter)

Updated

7 years ago
Depends on: 663508
Created attachment 539323 [details] [diff] [review]
use FixedMalloc::FindBeginning to do the fallback lookup
(Reporter)

Updated

6 years ago
Whiteboard: has-patch
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

6 years ago
Status: NEW → ASSIGNED

Updated

6 years ago
Attachment #539323 - Flags: review?(treilly) → review+

Updated

6 years ago
Attachment #539323 - Flags: superreview?(lhansen) → superreview+

Comment 3

6 years ago
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
I do not trust FindBeginning 100% in debug mode, due to bug 735121.  So let us fix that first.
(Reporter)

Updated

6 years ago
Depends on: 735121
(Reporter)

Updated

5 years ago
Blocks: 657675
(Reporter)

Updated

5 years ago
Assignee: fklockii → nobody

Comment 5

4 years ago
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);
>     }
You need to log in before you can comment on or make changes to this bug.