Bug 182457 – Gigacage: enable only for WebContent process and token executables

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[JF Bastien]

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.

[JF Bastien]

<rdar://problem/35875011>

[JF Bastien]

Created attachment 333027 [details]
patch

[EWS Watchlist]

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.

[Keith Miller]

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...

[JF Bastien]

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.

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 333084 [details]
patch

Clearing flags on attachment: 333084

Committed r228108: <https://trac.webkit.org/changeset/228108>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

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