Bug 80073

Summary: WebKit2: remove NSPasteboard access for promised data from the WebProcess
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: Enrica Casucci <enrica>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, ap, dglazkov, sam, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: Unspecified   
Attachments:
Description Flags
Patch
sam: review-, webkit.review.bot: commit-queue-
Patch
none
Patch ap: review+

Description Enrica Casucci 2012-03-01 17:09:14 PST
The last step to remove access to the NSPasteboard from the WebProcess requires moving handling of the NSPromisedFileDrag to the UI process.
Comment 1 Enrica Casucci 2012-03-01 17:26:13 PST
Created attachment 129782 [details]
Patch
Comment 2 WebKit Review Bot 2012-03-01 20:02:45 PST
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
Comment 3 Alexey Proskuryakov 2012-03-02 14:13:28 PST
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?
Comment 4 Enrica Casucci 2012-03-02 14:21:10 PST
(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.
Comment 5 Alexey Proskuryakov 2012-03-02 14:39:42 PST
> > 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?
Comment 6 Sam Weinig 2012-03-04 16:35:57 PST
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 *>.
Comment 7 Enrica Casucci 2012-03-12 11:46:51 PDT
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.
Comment 8 WebKit Review Bot 2012-03-12 11:49:29 PDT
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.
Comment 9 Enrica Casucci 2012-03-12 11:55:31 PDT
Created attachment 131370 [details]
Patch

Uploading new patch with bug number in the change log.
Comment 10 Alexey Proskuryakov 2012-03-12 12:55:47 PDT
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.
Comment 11 Enrica Casucci 2012-03-12 14:28:13 PDT
(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.
Comment 12 Enrica Casucci 2012-03-12 15:48:21 PDT
http://trac.webkit.org/changeset/110494