Bug 215869 – [macOS] Stop logging sandbox violations of com.apple.CoreDisplay.Notification

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

[Per Arne Vollan]

On macOS, stop logging mach-lookup sandbox violations of com.apple.CoreDisplay.Notification for performance reasons.

[Per Arne Vollan]

Created attachment 407342 [details]
Patch

[Per Arne Vollan]

<rdar://problem/67831223>

[Per Arne Vollan]

Created attachment 407353 [details]
Patch

[Darin Adler]

Comment on attachment 407353 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        On macOS, stop logging mach-lookup sandbox violations of com.apple.CoreDisplay.Notification for performance reasons.

This makes it sound like it’s something already denied, and this patch simply changes it to no-log.

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:771
> +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 110000
> +(deny mach-lookup (with no-log)
> +    (global-name "com.apple.CoreDisplay.Notification))
> +#endif

But this is a new deny rule, not changing an existing one to no-log mode.

What am I missing?

[Per Arne Vollan]

(In reply to Darin Adler from comment #4)
> Comment on attachment 407353 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407353&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        On macOS, stop logging mach-lookup sandbox violations of com.apple.CoreDisplay.Notification for performance reasons.
> 
> This makes it sound like it’s something already denied, and this patch
> simply changes it to no-log.
> 

Yes, that is correct.

> > Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:771
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 110000
> > +(deny mach-lookup (with no-log)
> > +    (global-name "com.apple.CoreDisplay.Notification))
> > +#endif
> 
> But this is a new deny rule, not changing an existing one to no-log mode.
> 
> What am I missing?

Yes, you are right. There is no previous rule for this service, so the default behavior is in effect, where the service is being denied, but with logging enabled.

This change is only changing the logging behavior.

Thanks for reviewing!

[Darin Adler]

Comment on attachment 407353 [details]
Patch

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

>>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:771
>>> +#endif
>> 
>> But this is a new deny rule, not changing an existing one to no-log mode.
>> 
>> What am I missing?
> 
> Yes, you are right. There is no previous rule for this service, so the default behavior is in effect, where the service is being denied, but with logging enabled.
> 
> This change is only changing the logging behavior.
> 
> Thanks for reviewing!

Oh, didn’t realize that "deny" was default for everything. I guess that should have been obvious.

[Per Arne Vollan]

(In reply to Darin Adler from comment #6)
> Comment on attachment 407353 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407353&action=review
> 
> >>> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:771
> >>> +#endif
> >> 
> >> But this is a new deny rule, not changing an existing one to no-log mode.
> >> 
> >> What am I missing?
> > 
> > Yes, you are right. There is no previous rule for this service, so the default behavior is in effect, where the service is being denied, but with logging enabled.
> > 
> > This change is only changing the logging behavior.
> > 
> > Thanks for reviewing!
> 
> Oh, didn’t realize that "deny" was default for everything. I guess that
> should have been obvious.

Actually, in some cases, "allow" is the default, but not in this case :)

Thanks for reviewing!

[EWS]

Committed r266216: <https://trac.webkit.org/changeset/266216>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407353 [details].

[Chris Dumez]

Comment on attachment 407353 [details]
Patch

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

> Source/WebKit/WebProcess/com.apple.WebProcess.sb.in:770
> +    (global-name "com.apple.CoreDisplay.Notification))

Missing double quote here :(

https://trac.webkit.org/changeset/266238/webkit