Bug 148500 - MarkedBlock::allocateBlock will have the wrong allocation size when (sizeof(MarkedBlock) + bytes) is divisible by WTF::pageSize()
Summary: MarkedBlock::allocateBlock will have the wrong allocation size when (sizeof(M...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Saam Barati
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-08-26 17:02 PDT by Saam Barati
Modified: 2015-08-26 22:58 PDT (History)
9 users (show)

See Also:


Attachments
patch (4.51 KB, patch)
2015-08-26 19:12 PDT, Saam Barati
fpizlo: review-
Details | Formatted Diff | Diff
patch (4.91 KB, patch)
2015-08-26 22:29 PDT, Saam Barati
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Saam Barati 2015-08-26 17:02:58 PDT
Consider the following scenario:
- On OS X, WTF::pageSize() is 4*1024 bytes.
- JSEnvironmentRecord::allocationSizeForScopeSize(6621) == 53000
- sizeof(MarkedBlock) == 248
- (248 + 53000) is a multiple of 4*1024.
- (248 + 53000)/(4*1024) == 13

We will allocate a chunk of memory of size 53248 bytes that looks like this:
0            248       256                       53248       53256
[Marked Block | 8 bytes |  payload     ......      ]  8 bytes  |
                        ^                                      ^
                   Our Environment record starts here.         ^
                                                               ^
                                                         Our last JSValue in the environment record will go from byte 53248 to 53256. But, we don't own this memory.
Comment 1 Saam Barati 2015-08-26 19:12:31 PDT
Created attachment 260019 [details]
patch
Comment 2 Filip Pizlo 2015-08-26 19:39:35 PDT
Comment on attachment 260019 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260019&action=review

I think there is an even lower-risk fix.

> Source/JavaScriptCore/ChangeLog:24
> +        All that we care about is that we allocate space for MarkedBlock
> +        on an atomSize boundary that is larger than sizeof(MarkedBlock).

I think that we care about first rounding up sizeof(MarkedBlock).  The old code was assuming that sizeof(MarkedBlock)+bytes is the amount of memory that is needed for the marked block, when in reality, it's roundUp<atomSize>(sizeof(MarkedBlock))+bytes.  Hence, we would sometimes forget to allocate the 8 bytes for the gap between the MarkedBlock header and the payload.

On the other hand, this sentence makes it seem like the bug was that we were rounding up to something other than atomSize.

> Source/JavaScriptCore/heap/MarkedAllocator.cpp:178
> -    size_t minAllocationSize = WTF::roundUpToMultipleOf(WTF::pageSize(), sizeof(MarkedBlock) + bytes);
> +    size_t minAllocationSize = WTF::roundUpToMultipleOf<MarkedBlock::atomSize>(sizeof(MarkedBlock)) + WTF::roundUpToMultipleOf<MarkedBlock::atomSize>(bytes);

I think that this contains an unnecessary behavior change: marked block allocation size will no longer be a multiple of system page size.

You could fix the bug more directly by saying:

size_t minAllocationSize = WTF::roundUpToMultipleOf(WTF::pageSize(), WTF::roundUpToMultipleOf<MarkedBlock::atomSize>(sizeof(MarkedBlock)) + bytes);

This way, you'd be preserving the page size alignment behavior.
Comment 3 Mark Lam 2015-08-26 19:49:43 PDT
Comment on attachment 260019 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260019&action=review

>> Source/JavaScriptCore/ChangeLog:24
>> +        on an atomSize boundary that is larger than sizeof(MarkedBlock).
> 
> I think that we care about first rounding up sizeof(MarkedBlock).  The old code was assuming that sizeof(MarkedBlock)+bytes is the amount of memory that is needed for the marked block, when in reality, it's roundUp<atomSize>(sizeof(MarkedBlock))+bytes.  Hence, we would sometimes forget to allocate the 8 bytes for the gap between the MarkedBlock header and the payload.
> 
> On the other hand, this sentence makes it seem like the bug was that we were rounding up to something other than atomSize.

Oh, I get it now.  When I was reading this earlier, I didn’t find the ChangeLog adequate to shed light on what the issue is (although it was there in the illustration).  I suggest you state clearly that the bug is that we need to start our JSValues (and hence the JSEnvironment) on an atomSize boundary, but the computation of minAllocationSize did not account for the needed padding between the MarkedBlock header and where the JSEnvironment needs to start.

And FWIW, I agree with Fil’s proposed alternate fix.
Comment 4 Saam Barati 2015-08-26 22:29:54 PDT
Created attachment 260033 [details]
patch
Comment 5 Mark Lam 2015-08-26 22:34:23 PDT
Comment on attachment 260033 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260033&action=review

r=me

> Source/JavaScriptCore/ChangeLog:3
> +        MarkedBlock::allocateBlock may have the wrong allocation size when (sizeof(MarkedBlock) + bytes) is divisible by WTF::pageSize()

Did you mean "will" instead of "may" here?
Comment 6 Saam Barati 2015-08-26 22:37:03 PDT
(In reply to comment #5)
> Comment on attachment 260033 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260033&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/ChangeLog:3
> > +        MarkedBlock::allocateBlock may have the wrong allocation size when (sizeof(MarkedBlock) + bytes) is divisible by WTF::pageSize()
> 
> Did you mean "will" instead of "may" here?

Yes.
Comment 7 Saam Barati 2015-08-26 22:58:17 PDT
landed in:
http://trac.webkit.org/changeset/189012