Bug 182217 - MarkedBlock should have a footer instead of a header
Summary: MarkedBlock should have a footer instead of a header
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords: InRadar
Depends on:
Blocks: 181636
  Show dependency tree / graph
 
Reported: 2018-01-27 10:59 PST by Filip Pizlo
Modified: 2018-01-27 18:26 PST (History)
9 users (show)

See Also:

Attachments
it's a start (18.37 KB, patch)
2018-01-27 11:00 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
maybe this is right (28.75 KB, patch)
2018-01-27 11:24 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (48.69 KB, patch)
2018-01-27 13:16 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (46.59 KB, patch)
2018-01-27 14:09 PST, Filip Pizlo 		no flags Details | Formatted Diff | Diff
the patch (47.24 KB, patch)
2018-01-27 14:20 PST, Filip Pizlo 		jfbastien: review+
Details | Formatted Diff | Diff
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2018-01-27 10:59:34 PST 
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.
Comment 1 Filip Pizlo 2018-01-27 11:00:30 PST 
Created attachment 332475 [details]
it's a start
Comment 2 Filip Pizlo 2018-01-27 11:24:37 PST 
Created attachment 332476 [details]
maybe this is right
Comment 3 Filip Pizlo 2018-01-27 13:16:52 PST 
Created attachment 332482 [details]
the patch
Comment 4 Filip Pizlo 2018-01-27 14:09:41 PST 
Created attachment 332483 [details]
the patch
Comment 5 Filip Pizlo 2018-01-27 14:20:51 PST 
Created attachment 332484 [details]
the patch

Now I like the changelog.
Comment 6 JF Bastien 2018-01-27 15:13:24 PST 
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?
Comment 7 Filip Pizlo 2018-01-27 17:39:20 PST 
(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.
Comment 8 Radar WebKit Bug Importer 2018-01-27 17:40:00 PST 
<rdar://problem/36952146>
Comment 9 Filip Pizlo 2018-01-27 18:26:00 PST 
Landed in https://trac.webkit.org/changeset/227717/webkit