[webkit-dev] RenderArena: Teaching an old dog new tricks

[Chris Evans]

On Wed, Nov 14, 2012 at 2:48 PM, Geoffrey Garen <ggaren at apple.com> wrote:

> Hi Eric.
>
> Here are some problems in RenderArena that I know of:
>
> - Grows memory without bound
> - Duplicates the functionality of FastMalloc
> - Makes allocation error-prone (allocation in the wrong arena is sometimes
> a leak, sometimes a use-after-free, and sometimes a heizenbug of the two)
> - Makes allocation verbose (you have to thread an arena pointer everywhere)
> - Makes object lifetime complicated (all objects are implicitly tied to a
> single owner they may outlive)
> - Uses C-style macros and manual initialization and destruction, instead
> of modern WebKit C++ style
>
> You didn't mention any of these problems in your email, so I'll assume you
> weren't aware of them.
>
> Considering these problems now, please don't use RenderArena in more
> places.
>
> > Slab-allocators (i.e. RenderArena) hand out memory all from a single
> > region, guaranteeing (among other things) that free'd objects can only
> > be ever overwritten by other objects from the same pool.  This makes
> > it much harder, for example to find a Use-After-Free of a RenderBlock
> > and then fill that RenderBlock's memory (and vtable pointer) with
> > arbitrary memory (like the contents of a javascript array).
> > http://en.wikipedia.org/wiki/Slab_allocation
>
> This is magical thinking. RenderArena is no different from FastMalloc.
>

It's actually different enough to be interesting:
- The bucket granularity is different, which affects exploitation
significantly.
- The packing is perfect, which might even lead to better memory usage
under some patterns, although, yes, it's true that there are possible
pathological conditions as noted by Maciej.
- free() is more expensive in fastMalloc because of various indirections to
work out if we're freeing a page range or a slab slot.
- RenderArena has freelist poisoning. I'm not sure if freelist poisoning
made it into upstream tcmalloc yet.
- There are very significant differences (from the attackers, and WebKit
consumer's point of view) on page reclaim.


> (1) RenderArena recycles by object size, just like FastMalloc.
>
> (2) FastMalloc is a slab allocator, just like RenderArena.
>
> (3) RenderArena grows by calling FastMalloc.
>
> Isolating object types from each other -- and specifically isolating
> objects of arbitrary size and contents like arrays -- is an interesting
> idea. RenderArena is neither necessary nor sufficient for implementing this
> feature.
>
> The only reason RenderArena seems isolated from other object types is
> social, not technical: we actively discourage using RenderArena, so few
> object types currently use it.
>
> > Since RenderArena is generic, the current plan to move it to WTF (as
> > by Chris Marrin suggested back in
> > http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg12672.html),
> > clean up the code further, and investigate wider deployment (like to
> > the DOM tree) for the security benefit and possible perf win.
> > https://bugs.webkit.org/show_bug.cgi?id=101087
>
> Having dealt with the specific technical question of RenderArena, I'd like
> to briefly discuss the meta-level of how the WebKit project works.
>
> Sam Weinig and I both provided review feedback saying that using
> RenderArena more was a bad idea (
> https://bugs.webkit.org/show_bug.cgi?id=101087#c9,
> https://bugs.webkit.org/show_bug.cgi?id=101087#c18). Nonetheless, you
> r+ed a patch to move in that direction, and you describe it here as the
> "current plan" for WebKit.
>
> I'm a little disappointed that, as individual contributors, Chris Neklar
> and Chris Evans didn't realize or understand the problems listed above, and
> didn't tackle them.


We realize RenderArena tradeoffs and have not tackled them in more detail
because https://bugs.webkit.org/show_bug.cgi?id=101087 is not the place to
do it. https://bugs.webkit.org/show_bug.cgi?id=101087 is a simple clean-up
changeset. Eric started this thread in order to start discussing some of
the trade-offs with a DOM slab -- something for which we haven't yet
uploaded any patches for anyone to r+ or r- :-)


Cheers
Chris


> However, the mistake is understandable: Chris and Chris are new to WebKit.
> The WebKit project has a mechanism for resolving mistakes like this: patch
> review.
>
> Your job as a reviewer is to understand the zeitgeist of the project, to
> use good judgement, and to r- patches that make mistakes like this. A bad
> patch is only a small nuisance. But the small nuisance turns into a major
> problem when you, as a reviewer, take a bad patch, mark it r+, and declare
> it the current direction of the project, despite the objections of two
> other reviewers who are senior members of the project.
>
> Geoff
>
> _______________________________________________
> webkit-dev mailing list
> webkit-dev at lists.webkit.org
> http://lists.webkit.org/mailman/listinfo/webkit-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20121114/c5846ab0/attachment.html>