Bug 82087 - tryReallocate could break the zero-ed memory invariant of CopiedBlocks
Summary: tryReallocate could break the zero-ed memory invariant of CopiedBlocks
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-23 13:58 PDT by Mark Hahnenberg
Modified: 2012-03-24 10:25 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.69 KB, patch)
2012-03-23 14:10 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (1.88 KB, patch)
2012-03-23 16:32 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (2.64 KB, patch)
2012-03-23 18:03 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>