Bug 148500 – MarkedBlock::allocateBlock will have the wrong allocation size when (sizeof(MarkedBlock) + bytes) is divisible by WTF::pageSize()

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

[Saam Barati]

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.

[Saam Barati]

Created attachment 260019 [details]
patch

[Filip Pizlo]

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.

[Mark Lam]

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.

[Saam Barati]

Created attachment 260033 [details]
patch

[Mark Lam]

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?

[Saam Barati]

(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.

[Saam Barati]

landed in:
http://trac.webkit.org/changeset/189012