Bug 82087

Summary: tryReallocate could break the zero-ed memory invariant of CopiedBlocks
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Mark Hahnenberg 2012-03-23 13:58:23 PDT
When we call tryReallocate, it resets the offset in the CopiedAllocator if the things we're trying to reallocate was the last thing we allocated out of the block. It then tries to allocate the new larger size out of the CopiedAllocator as if it had never done the old allocation in the first place. This prevents us having to copy if we hit this case.

If the new size is too big, however, we will just go on and allocate a new block by calling tryAllocate. This leaves the old dirty allocation in the block without resetting our current offset. tryAllocate, however, could cause a GC to occur. 

If the block that we just abandoned with dirty memory is pinned during the GC, we will keep it around and put it back into to-space after the collection. In fact, it could become the new "head" allocation block since pinned blocks are pushed onto the front of the new to-space after copying has completed (see CopiedSpace::doneCopying()). If this happens, we will use this dirty space for any new allocations out of the block. However, these allocations are supposed to be guaranteed that their memory is clear when they receive it. Thus, we could end up with bogus dirty memory at the tail end of whatever space we're allocating, which, if another GC runs, will cause us to crash by trying to access stale/possibly dead objects.

The fix for this is simply to reset the offset in the CopiedAllocator back to where it started if we fail to allocate from the current block during our reallocate optimization. This way, if the block is pinned and makes it to the head of our new to-space after a collection, we will start off allocating from a known clean offset.
Comment 1 Mark Hahnenberg 2012-03-23 14:10:42 PDT
Created attachment 133556 [details]
Patch
Comment 2 Geoffrey Garen 2012-03-23 15:30:53 PDT
Comment on attachment 133556 [details]
Patch

I think you can simplify this code, like so:

if (m_allocator.wasLastAllocation(oldPtr, oldSize)) {
    size_t delta = newSize - oldSize;
    if (m_allocator.fitsInCurrentBlock(delta)) {
        m_allocator.allocate(delta))
        return oldPtr;
    }
}
Comment 3 Mark Hahnenberg 2012-03-23 16:32:24 PDT
Created attachment 133586 [details]
Patch
Comment 4 Mark Hahnenberg 2012-03-23 18:03:16 PDT
Created attachment 133602 [details]
Patch
Comment 5 Filip Pizlo 2012-03-23 18:03:45 PDT
Comment on attachment 133602 [details]
Patch

R=me.
Comment 6 WebKit Review Bot 2012-03-23 20:12:04 PDT
Comment on attachment 133602 [details]
Patch

Clearing flags on attachment: 133602

Committed r111973: <http://trac.webkit.org/changeset/111973>
Comment 7 WebKit Review Bot 2012-03-23 20:12:08 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Mark Hahnenberg 2012-03-24 10:25:49 PDT
<rdar://problem/10884040>