Bug 82087 – tryReallocate could break the zero-ed memory invariant of CopiedBlocks

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 133556 [details]
Patch

[Geoffrey Garen]

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;
    }
}

[Mark Hahnenberg]

Created attachment 133586 [details]
Patch

[Mark Hahnenberg]

Created attachment 133602 [details]
Patch

[Filip Pizlo]

Comment on attachment 133602 [details]
Patch

R=me.

[WebKit Review Bot]

Comment on attachment 133602 [details]
Patch

Clearing flags on attachment: 133602

Committed r111973: <http://trac.webkit.org/changeset/111973>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Hahnenberg]

<rdar://problem/10884040>