It seems like RenderArena is not actually specific to rendering and could be called something like RecycledArena. Seems like a better home would be platform so that other classes could use this type of arena. I have a patch for this that I will upload shortly. It seems like the only renderer specific code at the moment is the hardcoded arena name but this should probably just be an argument to the constructor.
Created attachment 172118 [details] Patch
Attachment 172118 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/platform/RecycledArena.cpp:43: Missing spaces around / [whitespace/operators] [3] Total errors found: 1 in 188 files If any of these errors are false positives, please file a bug against check-webkit-style.
I'm not a huge fan of the name. :) I suspect neither you or I have particularly strong opinions on the subject. These feel a lot like malloc zones. Slab is also a common name for these things I'm told. I would also note (and this is slightly unrelated to this bug), that if we're going to make RenderArena more widely used (instead of killing it -- which has long been the plan), then we need to make our smart pointers smarter about it! Right now you cant OwnPtr a RenderObject because we need to use the arena during destruction. However, RenderObjects almost always know how to get to their Arena (and certainly could be taught how to get there always) and thus we could (and should!) make a smarter OwnPtr to handle arena'd objects.
I should also note that this code originally comes from mozilla. It's possible that we should make some effort to sync up with their version at some point. Or at least pull any good bits. :)
Comment on attachment 172118 [details] Patch Attachment 172118 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14670951
Eric, Yeah I'm not sure how I feel about the name. The reasoning was that all this really adds on top of Arenas are freelists for easy reuse. I could see something like ReusableArena but "Recycled" was already mentioned in comments and variable names. Making OwnPtrs play nice does seem like a good idea. On another note I believe cevans was playing with some benchmarks using this for other object types and getting interesting results but I'll let him comment.
Created attachment 172141 [details] Patch
Attachment 172141 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/CMakeLists.txt', u'Source/W..." exit_code: 1 Source/WebCore/platform/RecycledArena.cpp:43: Missing spaces around / [whitespace/operators] [3] Total errors found: 1 in 191 files If any of these errors are false positives, please file a bug against check-webkit-style.
I would rather not generalize this. Our long term plan is to stop using the RenderArena, so changes to use it in more places is probably not a great idea. If you have specific other uses where this type of allocator would be useful that would be interesting, but we should have those use cases established before moving forward.
(In reply to comment #9) > I would rather not generalize this. Our long term plan is to stop using the RenderArena, so changes to use it in more places is probably not a great idea. > > If you have specific other uses where this type of allocator would be useful that would be interesting, but we should have those use cases established before moving forward. cevans would like to use it for allocating DOM Nodes. He has a prototype patch. I agree we should probably wait to see how that bears fruit before generalizing.
I think generalizing this is the right thing to do. It's also independent of future usages or de-usages. It's the right thing to do because if you look at the RenderArena implementation, it has nothing to do specifically with rendering. It's simply layering additional allocator functionality on top of platform/Arena.h. In terms of stopping using RenderArena, I'd be grateful for anyone looking to do that to at least make it a platform-specific choice / define. There are some really nice security properties we get from having it, so we'd keep it for the Chromium port. In terms of possible additional usages of RecycledArena, we're looking at security hardening measures that would have stopped Pinkie Pie's recent SVGUseElement exploit.
Some initial performance notes on tests for DOM node slabbing. It's reasonably promising. Most tests are performance neutral because creating tons of DOM nodes in a tight loop isn't a super common thing. In terms of tests that did show a difference: Parser/html5-full-render.html: +1% DOM/CreateNodes.html: +2% Parser/innerHTML-setter.html: +1% Interestingly, this is in a Chromium build where everything is allocated with tcmalloc (already a fast allocator). I've done some spelunking on how a Mac Safari build looks to work, and most Node subclasses are _not_ tagged with WTF_MAKE_FAST_ALLOCATED which would suggest a slower non-tcmalloc allocation and further suggest a possible larger performance gain in that configuration. I'm working an analysis of the security gain this would have given us against Pinkie Pie's SVGUseElement attack, it also seems promising but I'm not ready to get properly excited just yet :)
Mac WebCore uses fastMalloc (an old fork of tcmalloc), it just turns it on a different way.
@eseidel: I just want to make sure I understand the Mac WebCore situation fully. My analysis from reading of the code is that: - operator new by default will use the system malloc() - Some WebCore classes, though, are annoted with the WTF_MAKE_FAST_ALLOCATED macro, which overrides operator new and calls fastMalloc() directly Is that correct?
> Is that correct? I think apple-mac always uses fastMalloc. Some other ports use the WTF_MAKE_FAST_ALLOCATED macro to have more fine-grained control over which objects use fastMalloc.
A brief analysis of the defense a DOM slab would have provided against Pinkie Pie's exploit has been completed. It's also quite promising. Alignments differ between 32-bit and 64-bit. On 32-bit there are significantly limited exploitation opportunities, particularly is looks like all the SVGUseElement vtables would be valid. 64-bit is more interesting; difference in vtable vs. data overlap could have been achieved with SVGUseElement vs. SVGAltGlyphElement. However, it seems like it would not be directly possible to either leak the value of the vtable ptr (ASLR bypass) or place a plausible pointer value on top of the vtable ptr. Anyway -- without getting too far into the weeds of the somewhat fringe science of exploitation -- I'm happy to conclude this is a significant defense.
Comment on attachment 172141 [details] Patch This seems fine to me. RenderArena is not dead yet, and I don't believe it's harmful to move it to WTF. I think that if we're going to keep it around we should make it more our own. This is one step towards doing that. Thank you.
This is completely insane. We don't want to have two malloc implementations, just because doing so is a 1% speedup. We can get a 1% speedup by optimizing our one malloc implementation.
Comment on attachment 172141 [details] Patch Marking r-. Please don't do this.
Hello Geoffrey, Thanks for dropping in to assist with the review. I'm not sure that declaring a patch from a fellow WebKit team member "completely insane" is entirely kind; but that strictly aside, there seems to be some confusion as to the scope of this changeset. Eric correctly summarizes the scope in https://bugs.webkit.org/show_bug.cgi?id=101087#c17 -- it's strictly a cleanup changeset which is moving something non-render-specific into a generic location. It's probably my fault for muddying the waters by putting performance numbers here. This changeset is of course performance neutral. (For the curious: the numbers relate to another change we will consider via a separate WebKit bug, and the goal of the numbers is _not_ to try and improve performance, but instead to prove that a possible security measure does not harm performance.)
> (For the curious: the numbers relate to another change we will consider via a separate WebKit bug Please don't use RenderArena in that change.
Thanks Geoff. I'll make sure to cc: you on any bug that starts to discuss concrete changes in the area of DOM slabbing.
I am puzzled by the claimed security benefits as well. RenderArena is not a true slab allocator, it allocates based on size classes, just like malloc. Both recycle freed memory. And neither limits specific classes to specific types, rather than size classes. What is it about RenderArena that makes it better security-wise? Is it just the fact that fewer different classes are allocated via RenderArena? Or is there some more fundamental difference? Your comment #16 does not really clarify, at least to me.
@Maciej: I'm not sure if the bug here or the webkit-dev thread is the best place to answer the question; but I'll simply answer here since the question was asked here. It is indeed significant that only a small number of classes are allocated within RenderArena. For example, when there's a RenderObject which has been freed but is still in use, it's not possible for the attacker to allocate (e.g.) a raw ArrayBuffer contents on top of the freed slot. So the attacker cannot place arbitrary bytes on top of where the vtable pointer is expected to be. We've made sure that the first sizeof(void*) bytes either point to a valid vtable pointer, or are a poisoned freelist pointer, or are an invalid pointer in the case that one of the (few) non-virtual classes is overlaid. It's essentially about limiting the attacker's possibilities when the inevitable rendering use-after-frees occur. There are some other non vtable related possibilities that get limited too.
(In reply to comment #24) > @Maciej: I'm not sure if the bug here or the webkit-dev thread is the best place to answer the question; but I'll simply answer here since the question was asked here. > Yeah, sorry to fragment the conversation. I'll reply to this on the webkit-dev thread.