Bug 182217 – MarkedBlock should have a footer instead of a header

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

[Filip Pizlo]

Patch forthcoming.

This helps us create a distancing constraint for positive offsets: any out-of-bounds read or write that has offset smaller than K, where K is MarkedBlock footer size, will end up hitting another object in the same block or the footer.

[Filip Pizlo]

Created attachment 332475 [details]
it's a start

[Filip Pizlo]

Created attachment 332476 [details]
maybe this is right

[Filip Pizlo]

Created attachment 332482 [details]
the patch

[Filip Pizlo]

Created attachment 332483 [details]
the patch

[Filip Pizlo]

Created attachment 332484 [details]
the patch

Now I like the changelog.

[JF Bastien]

Comment on attachment 332484 [details]
the patch

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

r=me

> Source/JavaScriptCore/heap/MarkedBlock.cpp:92
> +        dataLog(RawPointer(this), ": Allocated.\n");

Drop logging or add verbose flag.

> Source/JavaScriptCore/heap/MarkedBlock.h:269
> +        CountingLock m_lock;

This is 4 bytes, I'd put it next to HeapVersion (also 4) instead of before the 2x 2 byte mark counts.

> Source/JavaScriptCore/heap/MarkedBlock.h:615
>      return true;

Seems like all uses of isAtom() should be succeeded by a size mask (when accessing the atoms) to Spectre-protect the accesses?

[Filip Pizlo]

(In reply to JF Bastien from comment #6)
> Comment on attachment 332484 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=332484&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/heap/MarkedBlock.cpp:92
> > +        dataLog(RawPointer(this), ": Allocated.\n");
> 
> Drop logging or add verbose flag.

That code was already there.  The patch makes it seem like it was added.  I'm just moving it.

> 
> > Source/JavaScriptCore/heap/MarkedBlock.h:269
> > +        CountingLock m_lock;
> 
> This is 4 bytes, I'd put it next to HeapVersion (also 4) instead of before
> the 2x 2 byte mark counts.

But the goal is more padding! ;-)

> 
> > Source/JavaScriptCore/heap/MarkedBlock.h:615
> >      return true;
> 
> Seems like all uses of isAtom() should be succeeded by a size mask (when
> accessing the atoms) to Spectre-protect the accesses?

I don't think so, because the index is not under user control.

[Radar WebKit Bug Importer]

<rdar://problem/36952146>

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/227717/webkit