Gigacage is a solid security improvement, but it's probably best to roll it out incrementally to the most valuable targets first and progressively try out more and more over time rather than outright enabling it everywhere. We've gotten some reports that it has some side-effects that weren't expected, so for now let's enable it for the WebContent process, JSC, and other executables we know, and then later we'll enable more gigacage uses.
<rdar://problem/35875011>
Created attachment 333027 [details] patch
Attachment 333027 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/ProcessCheck.mm:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 333027 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333027&action=review r=me with comment. > Source/bmalloc/bmalloc/ProcessCheck.h:34 > +bool gigacageEnabledForProcess() { return false; } Don't you need to mark this inline or forward declare the function for this to build? > Source/bmalloc/bmalloc/ProcessCheck.h:39 > +bool gigacageEnabledForProcess() { return true; } ditto, idk how this builds...
Created attachment 333084 [details] patch > > Source/bmalloc/bmalloc/ProcessCheck.h:34 > > +bool gigacageEnabledForProcess() { return false; } > > Don't you need to mark this inline or forward declare the function for this > to build? > > > Source/bmalloc/bmalloc/ProcessCheck.h:39 > > +bool gigacageEnabledForProcess() { return true; } > > ditto, idk how this builds... Oops yeah, included in just one .cpp file so it Just Works, but it's also Just Wrong.
Attachment 333084 [details] did not pass style-queue: ERROR: Source/bmalloc/bmalloc/ProcessCheck.mm:26: Found header this file implements before WebCore config.h. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 333084 [details] patch Clearing flags on attachment: 333084 Committed r228108: <https://trac.webkit.org/changeset/228108>
All reviewed patches have been landed. Closing bug.
Comment on attachment 333084 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=333084&action=review > Source/bmalloc/bmalloc/ProcessCheck.mm:36 > + static NSString *appName = [[NSBundle mainBundle] bundleIdentifier]; Style Nit: I’d just wrap this entire thing in a do_once and get rid of these static variables > Source/bmalloc/bmalloc/ProcessCheck.mm:45 > + static bool isOptInBinary = [processName isEqualToString:@"jsc"] Is it worth opting in minibrowser since we use that as a proxy for the full browser often. Also, what about DumpRenderTree and WebkitTestRunner? We should definitely opt those in