Bug 207279 – [iOS] Do not create sandbox reports when the UI process cannot issue extensions to diagnostics service

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

Do not create sandbox reports when the UI process cannot issue mach extensions to the diagnostics service. The majority of clients are capable of doing this.

[Per Arne Vollan]

rdar://problem/59030957

[Per Arne Vollan]

Created attachment 389838 [details]
Patch

[Brent Fulgham]

Comment on attachment 389838 [details]
Patch

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

> Source/WebKit/Shared/SandboxExtension.h:59
> +    };

You could maybe do:

enum class Flags : uint32_t {
    Default = 0,
    NoReport = SANDBOX_EXTENSION_NO_REPORT
};


Then I think that OptionSet<Flags> would map to unit32_t, and do the |-ing for you.

But you would need to add SandboxSPI.h header, which might be undesirable.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:91
> +            extensionFlags |= SANDBOX_EXTENSION_NO_REPORT;

I don't think you need this code, if you make the OptionSet<Flags> as I suggested.

> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:97
> +            return sandbox_extension_issue_file(APP_SANDBOX_READ_WRITE, path, extensionFlags);

I wonder if we should consider adding SANDBOX_EXTENSION_CANONICAL to our file paths (if we ensure we always work with fully-resolved paths).

[Per Arne Vollan]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 389838 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389838&action=review
> 
> > Source/WebKit/Shared/SandboxExtension.h:59
> > +    };
> 
> You could maybe do:
> 
> enum class Flags : uint32_t {
>     Default = 0,
>     NoReport = SANDBOX_EXTENSION_NO_REPORT
> };
> 
> 
> Then I think that OptionSet<Flags> would map to unit32_t, and do the |-ing
> for you.
> 
> But you would need to add SandboxSPI.h header, which might be undesirable.
> 

Good idea! However, I don't think this will compile on other platforms. Are you OK with keeping as is?

> > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:91
> > +            extensionFlags |= SANDBOX_EXTENSION_NO_REPORT;
> 
> I don't think you need this code, if you make the OptionSet<Flags> as I
> suggested.
> 
> > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:97
> > +            return sandbox_extension_issue_file(APP_SANDBOX_READ_WRITE, path, extensionFlags);
> 
> I wonder if we should consider adding SANDBOX_EXTENSION_CANONICAL to our
> file paths (if we ensure we always work with fully-resolved paths).

I think that would be good enhancement! Perhaps we could do this in a follow-up patch?

Thanks for reviewing!

[Brent Fulgham]

Comment on attachment 389838 [details]
Patch

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

>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:91
>>> +            extensionFlags |= SANDBOX_EXTENSION_NO_REPORT;
>> 
>> I don't think you need this code, if you make the OptionSet<Flags> as I suggested.
> 
> I think that would be good enhancement! Perhaps we could do this in a follow-up patch?
> 
> Thanks for reviewing!

Sure -- no need to do now.

[WebKit Commit Bot]

Comment on attachment 389838 [details]
Patch

Clearing flags on attachment: 389838

Committed r255874: <https://trac.webkit.org/changeset/255874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.