Bug 192670 – [macOS] Block coreservicesd in sandbox.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Per Arne Vollan]

We should block CoreServices in newer versions of macOS.

[Per Arne Vollan]

Created attachment 357240 [details]
Patch

[Per Arne Vollan]

This patch stops blocking CoreServices on Mojave, since this caused a performance regression.

[Geoffrey Garen]

Comment on attachment 357240 [details]
Patch

This looks like the right way to restore coreservicesd sandboxing. But we probably shouldn't do that until we fix the _CSCheckFix performance issue.

[Geoffrey Garen]

...Also we need to make sure that denying access to coreservicesd does not prevent us from setting our process name in Activity Monitor.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #4)
> ...Also we need to make sure that denying access to coreservicesd does not
> prevent us from setting our process name in Activity Monitor.

That's a good point. I believe this should work, since we were able to set the url of the WebContent process in Activity Monitor also before _RegisterApplication was moved to be called before entering the sandbox.

[Per Arne Vollan]

Created attachment 359277 [details]
Patch

[Per Arne Vollan]

Created attachment 359280 [details]
Patch

[EWS Watchlist]

Attachment 359280 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:104:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:218:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Geoffrey Garen]

Comment on attachment 359280 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359280&action=review

r=me

> Source/WebKit/Shared/ChildProcess.cpp:77
> +    initializeProcessName(parameters);

Can you manually verify that WebContent process names still show up in Activity Monitor correctly? They should appear either as "http://example.com" for one page per process, "http://example.com, ..." for multiple pages per process, or "Safari WebContent (Prewarmed)" for the prewarmed PSON process.

[Per Arne Vollan]

(In reply to Geoffrey Garen from comment #9)
> Comment on attachment 359280 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359280&action=review
> 
> r=me
> 
> > Source/WebKit/Shared/ChildProcess.cpp:77
> > +    initializeProcessName(parameters);
> 
> Can you manually verify that WebContent process names still show up in
> Activity Monitor correctly? They should appear either as
> "http://example.com" for one page per process, "http://example.com, ..." for
> multiple pages per process, or "Safari WebContent (Prewarmed)" for the
> prewarmed PSON process.

Yes, manually verified that this still works :)

Thanks for reviewing!

[Aakash Jain]

> Created attachment 359280 [details]
This patch seems to cause following API Tests issues:

Failure: TestWebKitAPI.ProcessSwap.SessionStorage
Timeout: TestWebKitAPI.WKNavigation.FailureToStartWebProcessRecovery
Crash:
TestWebKitAPI.WebKit.OverrideLayoutSizeIsRestoredAfterChangingDuringProcessRelaunch
TestWebKitAPI.WebKit.OverrideLayoutSizeIsRestoredAfterProcessRelaunch
TestWebKitAPI.WebKit.DoAfterNextPresentationUpdateAfterCrash
TestWebKitAPI.WebKit.InteractionDeadlockAfterCrash
TestWebKitAPI.WKNavigation.FailureToStartWebProcessAfterCrashRecovery
TestWebKitAPI.WebKit.DoAfterNextStablePresentationUpdateAfterCrash


Build: https://ews-build.webkit-uat.org/#/builders/20/builds/176

We are doing test-runs for upcoming 'EWS for API tests'. If this is a false positive, please let us know.

[Aakash Jain]

on macOS, this patch seems to cause following API Tests issues:

Timeout: TestWebKitAPI.WKNavigation.FailureToStartWebProcessRecovery
Crash:
TestWebKitAPI.WebKit.ResizeWindowAfterCrash
TestWebKitAPI.WebKit.ReloadPageAfterCrash
TestWebKitAPI.WebKit.EnumerateDevices
TestWebKitAPI.WKNavigation.FailureToStartWebProcessAfterCrashRecovery
TestWebKitAPI.WebKit.LoadPageAfterCrash


Build: https://ews-build.webkit-uat.org/#/builders/19/builds/311

We are doing test-runs for upcoming 'EWS for API tests'. If this is a false positive, please let us know.

[Brent Fulgham]

<rdar://problem/47106766>

[Per Arne Vollan]

(In reply to Aakash Jain from comment #12)
> on macOS, this patch seems to cause following API Tests issues:
> 
> Timeout: TestWebKitAPI.WKNavigation.FailureToStartWebProcessRecovery
> Crash:
> TestWebKitAPI.WebKit.ResizeWindowAfterCrash
> TestWebKitAPI.WebKit.ReloadPageAfterCrash
> TestWebKitAPI.WebKit.EnumerateDevices
> TestWebKitAPI.WKNavigation.FailureToStartWebProcessAfterCrashRecovery
> TestWebKitAPI.WebKit.LoadPageAfterCrash
> 
> 
> Build: https://ews-build.webkit-uat.org/#/builders/19/builds/311
> 
> We are doing test-runs for upcoming 'EWS for API tests'. If this is a false
> positive, please let us know.

I don't see these failures on my local build.

[Per Arne Vollan]

Created attachment 361411 [details]
Patch

[EWS Watchlist]

Attachment 361411 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:104:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
ERROR: Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:218:  Misplaced OS version check. Please use a named macro in wtf/Platform.h, wtf/FeatureDefines.h, or an appropriate internal file.  [build/version_check] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Aakash Jain]

> I don't see these failures on my local build.
It might be false positive then.

[Alexey Proskuryakov]

Comment on attachment 361411 [details]
Patch

Please address the style checker error.

[Per Arne Vollan]

Created attachment 361431 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #18)
> Comment on attachment 361411 [details]
> Patch
> 
> Please address the style checker error.

Thanks Alexey! Uploaded new patch to address the style error.

[Alexey Proskuryakov]

Comment on attachment 361431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361431&action=review

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:644
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500

Can't we use HAVE(CSCHECKFIXDISABLE) here?

[Per Arne Vollan]

Created attachment 361433 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #21)
> Comment on attachment 361431 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361431&action=review
> 
> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:644
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> 
> Can't we use HAVE(CSCHECKFIXDISABLE) here?

I don't think we can use these macros in the sandbox .in file.

[Alexey Proskuryakov]

Comment on attachment 361431 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=361431&action=review

>>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:644
>>> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
>> 
>> Can't we use HAVE(CSCHECKFIXDISABLE) here?
> 
> I don't think we can use these macros in the sandbox .in file.

sb.in files are preprocessed with wtf/Platform.h included. Why do HAVE macros not work?

[Per Arne Vollan]

Created attachment 361453 [details]
Patch

[Per Arne Vollan]

(In reply to Alexey Proskuryakov from comment #24)
> Comment on attachment 361431 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361431&action=review
> 
> >>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:644
> >>> +#if __MAC_OS_X_VERSION_MIN_REQUIRED < 101500
> >> 
> >> Can't we use HAVE(CSCHECKFIXDISABLE) here?
> > 
> > I don't think we can use these macros in the sandbox .in file.
> 
> sb.in files are preprocessed with wtf/Platform.h included. Why do HAVE
> macros not work?

You are absolutely right :) Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 361453 [details]
Patch

Clearing flags on attachment: 361453

Committed r241169: <https://trac.webkit.org/changeset/241169>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.