Bug 220595 – [GPUP][iOS] Create sandbox extensions for cache and temp directory

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

[Per Arne Vollan]

Create sandbox extensions for GPU process access to cache and temp directory.

[Per Arne Vollan]

<rdar://problem/72450307>

[Per Arne Vollan]

Created attachment 417544 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 417544 [details]
Patch

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

> Source/WebKit/GPUProcess/GPUProcessCreationParameters.cpp:57
> +#if PLATFORM(IOS_FAMILY)
> +    encoder << containerCachesDirectoryExtensionHandle;
> +    encoder << containerTemporaryDirectoryExtensionHandle;
> +#endif

Why is this iOS only?

[Chris Dumez]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 417544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417544&action=review
> 
> > Source/WebKit/GPUProcess/GPUProcessCreationParameters.cpp:57
> > +#if PLATFORM(IOS_FAMILY)
> > +    encoder << containerCachesDirectoryExtensionHandle;
> > +    encoder << containerTemporaryDirectoryExtensionHandle;
> > +#endif
> 
> Why is this iOS only?

When GPUProcess is disabled and we pass those to the WebProcess, the code is for IOS_FAMILY only too, so at least it is consistent.

The patch does not build on iOS EWS though.

[Per Arne Vollan]

Created attachment 417547 [details]
Patch

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 417544 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=417544&action=review
> 
> > Source/WebKit/GPUProcess/GPUProcessCreationParameters.cpp:57
> > +#if PLATFORM(IOS_FAMILY)
> > +    encoder << containerCachesDirectoryExtensionHandle;
> > +    encoder << containerTemporaryDirectoryExtensionHandle;
> > +#endif
> 
> Why is this iOS only?

This works a little different on macOS, where confstr is used in the UI process to determine the cache and temp directory. The cache and temp directories are then passed as sandbox parameters, and sandbox rules will allow access to these directories.

Thanks for reviewing!

[Per Arne Vollan]

(In reply to Chris Dumez from comment #4)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 417544 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=417544&action=review
> > 
> > > Source/WebKit/GPUProcess/GPUProcessCreationParameters.cpp:57
> > > +#if PLATFORM(IOS_FAMILY)
> > > +    encoder << containerCachesDirectoryExtensionHandle;
> > > +    encoder << containerTemporaryDirectoryExtensionHandle;
> > > +#endif
> > 
> > Why is this iOS only?
> 
> When GPUProcess is disabled and we pass those to the WebProcess, the code is
> for IOS_FAMILY only too, so at least it is consistent.
> 
> The patch does not build on iOS EWS though.

Should be fixed in latest patch.

Thanks for reviewing!

[Simon Fraser (smfr)]

(In reply to Per Arne Vollan from comment #6)
> (In reply to Simon Fraser (smfr) from comment #3)
> > Comment on attachment 417544 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=417544&action=review
> > 
> > > Source/WebKit/GPUProcess/GPUProcessCreationParameters.cpp:57
> > > +#if PLATFORM(IOS_FAMILY)
> > > +    encoder << containerCachesDirectoryExtensionHandle;
> > > +    encoder << containerTemporaryDirectoryExtensionHandle;
> > > +#endif
> > 
> > Why is this iOS only?
> 
> This works a little different on macOS, where confstr is used in the UI
> process to determine the cache and temp directory. The cache and temp
> directories are then passed as sandbox parameters, and sandbox rules will
> allow access to these directories.

It would be nicer to use a HAVE_FOO or USE_FOO macro then. We should avoid sprinkling platform #ifdefs around.

[Per Arne Vollan]

(In reply to Simon Fraser (smfr) from comment #8)
> (In reply to Per Arne Vollan from comment #6)
> > (In reply to Simon Fraser (smfr) from comment #3)
> > > Comment on attachment 417544 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=417544&action=review
> > > 
> > > > Source/WebKit/GPUProcess/GPUProcessCreationParameters.cpp:57
> > > > +#if PLATFORM(IOS_FAMILY)
> > > > +    encoder << containerCachesDirectoryExtensionHandle;
> > > > +    encoder << containerTemporaryDirectoryExtensionHandle;
> > > > +#endif
> > > 
> > > Why is this iOS only?
> > 
> > This works a little different on macOS, where confstr is used in the UI
> > process to determine the cache and temp directory. The cache and temp
> > directories are then passed as sandbox parameters, and sandbox rules will
> > allow access to these directories.
> 
> It would be nicer to use a HAVE_FOO or USE_FOO macro then. We should avoid
> sprinkling platform #ifdefs around.

Sounds good, I will use a USE macro!

[Per Arne Vollan]

Created attachment 417564 [details]
Patch

[Per Arne Vollan]

Created attachment 417612 [details]
Patch

[Per Arne Vollan]

Created attachment 417614 [details]
Patch

[EWS]

Committed r271482: <https://trac.webkit.org/changeset/271482>

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