<div style="font-family: arial, helvetica, sans-serif; font-size: 10pt">On Wed, Nov 14, 2012 at 2:48 PM, Geoffrey Garen <span dir="ltr"><<a href="mailto:ggaren@apple.com" target="_blank">ggaren@apple.com</a>></span> wrote:<br>
<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Hi Eric.<br>
<br>
Here are some problems in RenderArena that I know of:<br>
<br>
- Grows memory without bound<br>
- Duplicates the functionality of FastMalloc<br>
- 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)<br>
- Makes allocation verbose (you have to thread an arena pointer everywhere)<br>
- Makes object lifetime complicated (all objects are implicitly tied to a single owner they may outlive)<br>
- Uses C-style macros and manual initialization and destruction, instead of modern WebKit C++ style<br>
<br>
You didn't mention any of these problems in your email, so I'll assume you weren't aware of them.<br>
<br>
Considering these problems now, please don't use RenderArena in more places.<br>
<div><br>
> Slab-allocators (i.e. RenderArena) hand out memory all from a single<br>
> region, guaranteeing (among other things) that free'd objects can only<br>
> be ever overwritten by other objects from the same pool. This makes<br>
> it much harder, for example to find a Use-After-Free of a RenderBlock<br>
> and then fill that RenderBlock's memory (and vtable pointer) with<br>
> arbitrary memory (like the contents of a javascript array).<br>
> <a href="http://en.wikipedia.org/wiki/Slab_allocation" target="_blank">http://en.wikipedia.org/wiki/Slab_allocation</a><br>
<br>
</div>This is magical thinking. RenderArena is no different from FastMalloc.<br></blockquote><div><br></div><div>It's actually different enough to be interesting:</div><div>- The bucket granularity is different, which affects exploitation significantly.</div>
<div>- 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.</div><div>- free() is more expensive in fastMalloc because of various indirections to work out if we're freeing a page range or a slab slot.</div>
<div>- RenderArena has freelist poisoning. I'm not sure if freelist poisoning made it into upstream tcmalloc yet.</div><div>- There are very significant differences (from the attackers, and WebKit consumer's point of view) on page reclaim.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
(1) RenderArena recycles by object size, just like FastMalloc.<br>
<br>
(2) FastMalloc is a slab allocator, just like RenderArena.<br>
<br>
(3) RenderArena grows by calling FastMalloc.<br>
<br>
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.<br>
<br>
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.<br>
<div><br>
> Since RenderArena is generic, the current plan to move it to WTF (as<br>
> by Chris Marrin suggested back in<br>
> <a href="http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg12672.html" target="_blank">http://www.mail-archive.com/webkit-dev@lists.webkit.org/msg12672.html</a>),<br>
> clean up the code further, and investigate wider deployment (like to<br>
> the DOM tree) for the security benefit and possible perf win.<br>
> <a href="https://bugs.webkit.org/show_bug.cgi?id=101087" target="_blank">https://bugs.webkit.org/show_bug.cgi?id=101087</a><br>
<br>
</div>Having dealt with the specific technical question of RenderArena, I'd like to briefly discuss the meta-level of how the WebKit project works.<br>
<br>
Sam Weinig and I both provided review feedback saying that using RenderArena more was a bad idea (<a href="https://bugs.webkit.org/show_bug.cgi?id=101087#c9" target="_blank">https://bugs.webkit.org/show_bug.cgi?id=101087#c9</a>, <a href="https://bugs.webkit.org/show_bug.cgi?id=101087#c18" target="_blank">https://bugs.webkit.org/show_bug.cgi?id=101087#c18</a>). Nonetheless, you r+ed a patch to move in that direction, and you describe it here as the "current plan" for WebKit.<br>
<br>
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.</blockquote><div><br></div><div>We realize RenderArena tradeoffs and have not tackled them in more detail because <a href="https://bugs.webkit.org/show_bug.cgi?id=101087">https://bugs.webkit.org/show_bug.cgi?id=101087</a> is not the place to do it. <a href="https://bugs.webkit.org/show_bug.cgi?id=101087">https://bugs.webkit.org/show_bug.cgi?id=101087</a> 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- :-)</div>
<div><br></div><div><br></div><div>Cheers</div><div>Chris</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>
<br>
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.<br>
<br>
Geoff<br>
<div><div><br>
_______________________________________________<br>
webkit-dev mailing list<br>
<a href="mailto:webkit-dev@lists.webkit.org" target="_blank">webkit-dev@lists.webkit.org</a><br>
<a href="http://lists.webkit.org/mailman/listinfo/webkit-dev" target="_blank">http://lists.webkit.org/mailman/listinfo/webkit-dev</a><br>
</div></div></blockquote></div><br>
</div>