Bug 209042 – reportZappedCellAndCrash should handle PreciseAllocation in IsoSubspace

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

[Yusuke Suzuki]

Let's extend.

[Yusuke Suzuki]

Created attachment 393567 [details]
Patch

[Yusuke Suzuki]

To collect more cell info in ASan bots.

[Mark Lam]

Comment on attachment 393567 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSCell.cpp:387
> +            variousState |= static_cast<uint64_t>(!isFreeListed) << 1;

Is this correct?  Does !!isFreeListed mean isAllocated?

[Mark Lam]

(In reply to Mark Lam from comment #3)
> Comment on attachment 393567 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=393567&action=review
> 
> > Source/JavaScriptCore/runtime/JSCell.cpp:387
> > +            variousState |= static_cast<uint64_t>(!isFreeListed) << 1;
> 
> Is this correct?  Does !!isFreeListed mean isAllocated?

typo: I meant !isFreeListed

[Mark Lam]

Comment on attachment 393567 [details]
Patch

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

r=me

>>> Source/JavaScriptCore/runtime/JSCell.cpp:387
>>> +            variousState |= static_cast<uint64_t>(!isFreeListed) << 1;
>> 
>> Is this correct?  Does !!isFreeListed mean isAllocated?
> 
> typo: I meant !isFreeListed

Nevermind, I've read thru the PreciseAllocation code and this patch some more.  This is correct because foundPreciseAllocation is only true if this cell is found in the lower tier of this IsoSubspace, and it is only allocated if it's not on the free list.

> Source/JavaScriptCore/runtime/JSCell.cpp:390
> +                variousState |= static_cast<uint64_t>(foundPreciseAllocation->isEmpty()) << 2;
> +                variousState |= static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4;

The isEmpty and isNewlyAllocated flags previously meant that the cell pointer was found in a MarkedBlock of that state.  For PreciseAllocations, I think knowing whether it isFreeListed or isAllocated is sufficient.  I'm not quite sure what isEmpty and isNewlyAllocated would mean in this case.  I suggest just leaving these unset.

Instead, can you also set the cellIsProperlyAligned flag ( << 5) in variousState based on whether (foundPreciseAllocation->cell() == cell)?  If the cell pointer is found in a PreciseAllocation but does not equal it's expected cell() pointer, then we have a corrupted pointer or someone unscrupulous is making bad pointers.

Can you also set the needsDestruction flag in variousState based on whether the SubSpace CellAttributes says it NeedsDestruction or not?

[Yusuke Suzuki]

Comment on attachment 393567 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSCell.cpp:390
>> +                variousState |= static_cast<uint64_t>(foundPreciseAllocation->isNewlyAllocated()) << 4;
> 
> The isEmpty and isNewlyAllocated flags previously meant that the cell pointer was found in a MarkedBlock of that state.  For PreciseAllocations, I think knowing whether it isFreeListed or isAllocated is sufficient.  I'm not quite sure what isEmpty and isNewlyAllocated would mean in this case.  I suggest just leaving these unset.
> 
> Instead, can you also set the cellIsProperlyAligned flag ( << 5) in variousState based on whether (foundPreciseAllocation->cell() == cell)?  If the cell pointer is found in a PreciseAllocation but does not equal it's expected cell() pointer, then we have a corrupted pointer or someone unscrupulous is making bad pointers.
> 
> Can you also set the needsDestruction flag in variousState based on whether the SubSpace CellAttributes says it NeedsDestruction or not?

I think isNewlyAllocated (In Eden or not) is nice to have. And isEmpty is also nice too (WeakSet is claered etc.). I'm querying this only when `!isFreeListed` since (1) isNewlyAllocated is meaningless if it is not in free list and (2) if this cell is not in freelist, we are not guaranteeing that WeakSet (isEmpty is accessing this) is not broken.
Adding cellIsProperlyAligned sounds nice. Fixed. Not sure whether NeedsDestruction is important since this can be super easily detected based on IsoSubspace, but maybe it would be ok. Added.

[Yusuke Suzuki]

Committed r258479: <https://trac.webkit.org/changeset/258479>

[Radar WebKit Bug Importer]

<rdar://problem/60466867>