Bug 212634 – REGRESSION(r261387): Introduced sandbox violations

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

The change set r261387 introduced some new mach lookup sandbox violations, when attempting to close Launch Services connections that were not open.

[Per Arne Vollan]

rdar://problem/63676658

[Per Arne Vollan]

Created attachment 400819 [details]
Patch

[Darin Adler]

Comment on attachment 400819 [details]
Patch

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

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:181
> +        auto connection = [objc_getClass("_LSDReadService") XPCConnectionToService];
> +        [connection invalidate];

How do we know this is the only connection we need to close? Seems very magical.

Also seems unnecessary to use a local variable here.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:186
>          ASSERT(String(uti.get()) = String(adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, CFSTR("text/html"), 0)).get()));

This assertion is using assignment ("="), not an equality check ("==").

[Per Arne Vollan]

Created attachment 400845 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #3)
> Comment on attachment 400819 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400819&action=review
> 
> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:181
> > +        auto connection = [objc_getClass("_LSDReadService") XPCConnectionToService];
> > +        [connection invalidate];
> 
> How do we know this is the only connection we need to close? Seems very
> magical.
> 
> Also seems unnecessary to use a local variable here.
> 

This is the only Launch Services connection type which is allowed (when holding the extension), so there cannot be any other Launch Services connections open.

> > Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:186
> >          ASSERT(String(uti.get()) = String(adoptCF(UTTypeCreatePreferredIdentifierForTag(kUTTagClassMIMEType, CFSTR("text/html"), 0)).get()));
> 
> This assertion is using assignment ("="), not an equality check ("==").

Ah, good catch!

Thanks for reviewing!

[EWS]

Committed r262435: <https://trac.webkit.org/changeset/262435>

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

[Simon Fraser (smfr)]

Comment on attachment 400845 [details]
Patch

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

> Source/WebCore/PAL/pal/spi/cocoa/LaunchServicesSPI.h:93
> +@interface _LSDReadService : _LSDService
> +@end

Let's not use non-SPI stuff in future.