Bug 210937 – [iOS] Fix sandbox violation when uploading a file

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Per Arne Vollan]

On iOS, the file chooser needs access to frontboard and icon services in the WebContent process. Create and consume extensions for these services when choosing files. When done, the extensions should be revoked.

[Per Arne Vollan]

Created attachment 397484 [details]
Patch

[Per Arne Vollan]

Created attachment 397486 [details]
Patch

[Per Arne Vollan]

Created attachment 397497 [details]
Patch

[Per Arne Vollan]

Created attachment 397511 [details]
Patch

[Darin Adler]

Comment on attachment 397511 [details]
Patch

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

> Source/WebKit/Shared/ios/WebIconUtilities.mm:95
> +    if (![interactionController.icons count])
> +        return nullptr;

How does this change relate to the rest? No comment in the change log to explain it. Definitely not obvious.

Do we have to carefully call this before the extension is revoked?

Also, UIImage * return value would normally be a nil, not *.

Also, looks like there is a mistake in thumbnailSizedImageForImage, which should return a RetainPtr<UIImage> or should retain/autorelease the result of UIGraphicsGetImageFromCurrentImageContext. Given that name it doesn’t sound safe to keep the pointer around before deallocating the image context.

[Per Arne Vollan]

Created attachment 397732 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #5)
> Comment on attachment 397511 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397511&action=review
> 
> > Source/WebKit/Shared/ios/WebIconUtilities.mm:95
> > +    if (![interactionController.icons count])
> > +        return nullptr;
> 
> How does this change relate to the rest? No comment in the change log to
> explain it. Definitely not obvious.
> 

When running the test associated with this patch in the iOS simulator, I encountered an out-of-bounds array exception, which was fixed by the change above. I added a comment about this in the change log.

> Do we have to carefully call this before the extension is revoked?
> 
> Also, UIImage * return value would normally be a nil, not *.
> 

Fixed.

> Also, looks like there is a mistake in thumbnailSizedImageForImage, which
> should return a RetainPtr<UIImage> or should retain/autorelease the result
> of UIGraphicsGetImageFromCurrentImageContext. Given that name it doesn’t
> sound safe to keep the pointer around before deallocating the image context.

I changed the return type to RetainPtr<UIImage> in the most recent patch. This lead to a few other changes. I did not put the bug back into the Review state, but it would perhaps be good if you could have another quick look to make sure the changes look good to you.

Thanks for reviewing!

[Per Arne Vollan]

Created attachment 397738 [details]
Patch

[Darin Adler]

Comment on attachment 397738 [details]
Patch

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

> Source/WebKit/Shared/ios/WebIconUtilities.mm:86
>      return resultImage;

Need to change the resultImage local variable to a RetainPtr. If UIGraphicsEndImageContext released then image, then this return statement is too late.

[Per Arne Vollan]

Created attachment 397749 [details]
Patch

[Per Arne Vollan]

(In reply to Darin Adler from comment #9)
> Comment on attachment 397738 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397738&action=review
> 
> > Source/WebKit/Shared/ios/WebIconUtilities.mm:86
> >      return resultImage;
> 
> Need to change the resultImage local variable to a RetainPtr. If
> UIGraphicsEndImageContext released then image, then this return statement is
> too late.

Good catch, thanks for reviewing!

[EWS]

Committed r260820: <https://trac.webkit.org/changeset/260820>

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

[Radar WebKit Bug Importer]

<rdar://problem/62513703>