Summary: | WebKit2: remove NSPasteboard access for promised data from the WebProcess | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||||
Component: | HTML Editing | Assignee: | 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
Enrica Casucci
2012-03-01 17:09:14 PST
Created attachment 129782 [details]
Patch
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 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? (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. > > 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 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 *>. 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.
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.
Created attachment 131370 [details]
Patch
Uploading new patch with bug number in the change log.
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. (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. |