Bug 80073 – WebKit2: remove NSPasteboard access for promised data from the WebProcess

• Component: HTML Editing
• Status: RESOLVED FIXED

[Enrica Casucci]

The last step to remove access to the NSPasteboard from the WebProcess requires moving handling of the NSPromisedFileDrag to the UI process.

[Enrica Casucci]

Created attachment 129782 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129782 [details]
Patch

Attachment 129782 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11776758

New failing tests:
compositing/reflections/nested-reflection-transformed.html

[Alexey Proskuryakov]

Comment on attachment 129782 [details]
Patch

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

There is a number of names with WTF prefix in the patch (e.g. WRF::PassRefPtr). Please remove it.

WebCore:: is also unnecessary in most or all implementation files (e.g. in PageImpl.mm). Please remove it, too.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:76
> +#import <WebKit/WebNSFileManagerExtras.h>
> +#import <WebKit/WebKitNSStringExtras.h>

Can we really include WebKit files from WebKit2???

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2599
> +    RetainPtr<NSPasteboard> pasteboard = [NSPasteboard pasteboardWithName:pasteboardName];

Why do we need to retain the pasteboard?

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2643
> +        NSData *data = _data->_promisedImage->data()->createNSData();

This looks like a leak.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2644
> +        wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:data] autorelease];

Why autorelease? We prefer explicit RetainPtrs.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2648
> +    if (wrapper == nil) {

WebKit style is to not compare to 0 or nil (just do "if (!wrapper)").

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2656
> +    NSString *path = [[dropDestination path] stringByAppendingPathComponent:[wrapper preferredFilename]];
> +    path = [[NSFileManager defaultManager] _webkit_pathWithUniqueFilenameForPath:path];
> +    if (![wrapper writeToFile:path atomically:NO updateFilenames:YES])

Where does the path come from? What parts of it can be influenced by a rogue WebProcess?

[Enrica Casucci]

(In reply to comment #3)
> (From update of attachment 129782 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129782&action=review
> 
> There is a number of names with WTF prefix in the patch (e.g. WRF::PassRefPtr). Please remove it.
Sure.
> 
> WebCore:: is also unnecessary in most or all implementation files (e.g. in PageImpl.mm). Please remove it, too.
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:76
> > +#import <WebKit/WebNSFileManagerExtras.h>
> > +#import <WebKit/WebKitNSStringExtras.h>
> 
> Can we really include WebKit files from WebKit2???
We do it in some other places too.

> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2599
> > +    RetainPtr<NSPasteboard> pasteboard = [NSPasteboard pasteboardWithName:pasteboardName];
> 
> Why do we need to retain the pasteboard?
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2643
> > +        NSData *data = _data->_promisedImage->data()->createNSData();
> 
> This looks like a leak.
Let me check.
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2644
> > +        wrapper = [[[NSFileWrapper alloc] initRegularFileWithContents:data] autorelease];
> 
> Why autorelease? We prefer explicit RetainPtrs.
Copied from existing code.
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2648
> > +    if (wrapper == nil) {
I'll fix this (I copied from existing code without thinking)
> 
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2656
> > +    NSString *path = [[dropDestination path] stringByAppendingPathComponent:[wrapper preferredFilename]];
> > +    path = [[NSFileManager defaultManager] _webkit_pathWithUniqueFilenameForPath:path];
> > +    if (![wrapper writeToFile:path atomically:NO updateFilenames:YES])
> 
> Where does the path come from? What parts of it can be influenced by a rogue WebProcess?
This is a path that we build starting from the image name that we retrieve from the image resource.
The image resource only provides the file name, the full path is built in the UI process.

thanks for the review. I'll double check the leak and, if confirmed, will post a new patch.

[Alexey Proskuryakov]

> > Can we really include WebKit files from WebKit2???
> We do it in some other places too.

I'm still not convinced that it's OK. What does Sam say?

> This is a path that we build starting from the image name that we retrieve from the image resource.
> The image resource only provides the file name, the full path is built in the UI process.

Can the image name provided by WebProcess have ".." components in it?

[Sam Weinig]

Comment on attachment 129782 [details]
Patch

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

> Source/WebKit2/UIProcess/API/mac/PageClientImpl.mm:272
> +void PageClientImpl::setPromisedData(const String& pasteboardName, PassRefPtr<WebCore::SharedBuffer> imageBuffer, const String& filename, const String& extension, const String& title, const String& url, const String& visibleUrl, WTF::PassRefPtr<WebCore::SharedBuffer> archiveBuffer)
> +{
> +    RefPtr<WebCore::Image> image = BitmapImage::create();

The WebCore::'s should not be necessary here.  We also usually make sure that all the letters in URL are the same case.

>>> Source/WebKit2/UIProcess/API/mac/WKView.mm:76
>>> +#import <WebKit/WebKitNSStringExtras.h>
>> 
>> Can we really include WebKit files from WebKit2???
> 
> We do it in some other places too.

We really should not be doing this.  We should move those things down to WebCore if we need them.

> Source/WebKit2/UIProcess/mac/WebPageProxyMac.mm:301
> +    RefPtr<SharedMemory> sharedMemoryImage = SharedMemory::create(imageHandle, SharedMemory::ReadOnly);
> +    RefPtr<SharedBuffer> imageBuffer = WebCore::SharedBuffer::create(static_cast<unsigned char *>(sharedMemoryImage->data()), imageSize);
> +    RefPtr<SharedBuffer> archiveBuffer;
> +    
> +    if (!archiveHandle.isNull()) {
> +        RefPtr<SharedMemory> sharedMemoryArchive = SharedMemory::create(archiveHandle, SharedMemory::ReadOnly);;
> +        archiveBuffer = WebCore::SharedBuffer::create(static_cast<unsigned char *>(sharedMemoryArchive->data()), archiveSize);
> +    }
> +    m_pageClient->setPromisedData(pasteboardName, imageBuffer, filename, extension, title, url, visibleURL, archiveBuffer);

The WebCore:: should not be necessary.  And the * is on the wrong side for static_cast<unsigned char *>.

[Enrica Casucci]

Created attachment 131368 [details]
Patch

This new patch addresses all the comments by Alexey and Sam.
The function that sanitizes the file name after receiving it from the WebProcess is filenameByFixingIllegalCharacters and is implemented in WebCoreNSStringExtras.mm.

[WebKit Review Bot]

Attachment 131368 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Enrica Casucci]

Created attachment 131370 [details]
Patch

Uploading new patch with bug number in the change log.

[Alexey Proskuryakov]

Comment on attachment 131370 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        WebKit2: remove the last NSPasteboard access from the WebProcess

Is the patch title still correct? We are again getting file paths in WebProcess.

> Source/WebKit2/UIProcess/API/mac/WKView.mm:2691
> +        NSData *data = [_data->_promisedImage->data()->createNSData() autorelease];

This should use RetainPtr, not autorelease.

[Enrica Casucci]

(In reply to comment #10)
> (From update of attachment 131370 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131370&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        WebKit2: remove the last NSPasteboard access from the WebProcess
> 
> Is the patch title still correct? We are again getting file paths in WebProcess.

No, it is not. I'll change it.

> > Source/WebKit2/UIProcess/API/mac/WKView.mm:2691
> > +        NSData *data = [_data->_promisedImage->data()->createNSData() autorelease];
> 
> This should use RetainPtr, not autorelease.
Will do.

Thanks for the review.

[Enrica Casucci]

http://trac.webkit.org/changeset/110494