On Wed, Nov 14, 2012 at 2:48 PM, Geoffrey Garen <[email protected]> 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/[email protected]/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 > [email protected] > http://lists.webkit.org/mailman/listinfo/webkit-dev >
_______________________________________________ webkit-dev mailing list [email protected] http://lists.webkit.org/mailman/listinfo/webkit-dev
