Summary: | WebKit2: create sandbox extensions for files that are dropped in an input control | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Enrica Casucci <enrica> | ||||||||||||
Component: | HTML Editing | Assignee: | Enrica Casucci <enrica> | ||||||||||||
Status: | NEW --- | ||||||||||||||
Severity: | Normal | CC: | andersca, cgarcia, gustavo, menard, rniwa, sam, webkit.review.bot, xan.lopez, zoltan | ||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Enrica Casucci
2012-03-14 13:45:02 PDT
Created attachment 131916 [details]
Patch
Comment on attachment 131916 [details] Patch Attachment 131916 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11957191 Comment on attachment 131916 [details] Patch Attachment 131916 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11957198 Comment on attachment 131916 [details]
Patch
r- so you can fix the EWS warnings for QT and Win bots
Created attachment 131929 [details]
Patch2
Fixes the Qt and Win builds.
Attachment 131929 [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/Shared/SandboxExtension.h:77: The parameter name "size" adds no information, so it should be removed. [readability/parameter_name] [5]
Total errors found: 1 in 17 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Will fix the style issue before landing the patch. Comment on attachment 131929 [details] Patch2 Attachment 131929 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11957247 Comment on attachment 131929 [details] Patch2 Attachment 131929 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11956301 Created attachment 131949 [details]
Patch3
One more attempt at fixing the QT, win and gtk builds.
Comment on attachment 131949 [details] Patch3 Attachment 131949 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/11953492 Comment on attachment 131949 [details] Patch3 Attachment 131949 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/11953502 Created attachment 131960 [details]
Patch4
One more try...
Comment on attachment 131960 [details] Patch4 Attachment 131960 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11957428 Comment on attachment 131960 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=131960&action=review The gtk EWS bot is still red, but patch looks good otherwise. > Source/WebCore/ChangeLog:11 > + an input type=file element. The extension are created at the time the files Typo, "extensions". > Source/WebKit2/Shared/SandboxExtension.h:86 > + Handle* m_data; > + size_t m_size; This should be a Vector. > Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:73 > + if (action != DragDestinationActionUpload) This would be more clear with an early return + comment. Comment on attachment 131960 [details] Patch4 View in context: https://bugs.webkit.org/attachment.cgi?id=131960&action=review Looks good to me, too. Seems worth posting a patch with comments addressed for final pass. > Source/WebCore/page/DragActions.h:39 > + DragDestinationActionUpload = 8, It's quite confusing that this is a bit flag, but it's only used exclusively. I'm not sure what to suggest to improve the design, but at least a comment seems necessary here. > Source/WebCore/page/DragActions.h:40 > DragDestinationActionAny = UINT_MAX What are the checks that match action to DragDestinationActionAny? Is it OK that we now have an additional action that's matching DragDestinationActionAny? > Source/WebKit2/ChangeLog:9 > + Now the the pasteboard access is performed only in the UI process, it is Typo "the the". > Source/WebKit2/ChangeLog:10 > + necessary to create sanbox extensions for each file that is dropped into Typo: "sanbox". > Source/WebKit2/Shared/SandboxExtension.h:115 > +inline const SandboxExtension::Handle& SandboxExtension::HandleArray::operator[](size_t) const { return *m_data; } > +inline SandboxExtension::Handle& SandboxExtension::HandleArray::operator[](size_t) { return *m_data; } This dereferences null, which is a bug. > Source/WebKit2/Shared/mac/SandboxExtensionMac.mm:145 > + for (size_t i = 0; i < size; i++) > + decoder->decode(handles[i]); Shouldn't this also check decoding result? > Source/WebKit2/UIProcess/WebPageProxy.cpp:866 > + SandboxExtension::HandleArray sandboxExtensionHandleArray; I like to use more descriptive names in such cases. Maybe "emptyArray"? Ditto below. > Source/WebKit2/UIProcess/API/mac/WKView.mm:1697 > + handles.reserve([files count]); > + for (unsigned int i = 0; i < [files count]; i++) { s/unsigned int/unsigned/. The name "reserve" is incorrect here. Reserving space in a container doesn't change its size, so handles[i] would still be a mistake after this reserve(). > Source/WebKit2/UIProcess/API/mac/WKView.mm:1700 > + if (![[NSFileManager defaultManager] fileExistsAtPath:file]) > + continue; Does this return YES for directories too? It's legitimate to drop directories (especially bundles). (In reply to comment #15) > (From update of attachment 131960 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131960&action=review > > The gtk EWS bot is still red, but patch looks good otherwise. > Uploading a new patch that should fix that too. I missed it last time. > > Source/WebCore/ChangeLog:11 > > + an input type=file element. The extension are created at the time the files > > Typo, "extensions". > Done. > > Source/WebKit2/Shared/SandboxExtension.h:86 > > + Handle* m_data; > > + size_t m_size; > > This should be a Vector. It cannot be a vector because Handle is non copyable. > > > Source/WebKit/mac/WebCoreSupport/WebDragClient.mm:73 > > + if (action != DragDestinationActionUpload) > > This would be more clear with an early return + comment. Done. (In reply to comment #16) > (From update of attachment 131960 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=131960&action=review > > Looks good to me, too. Seems worth posting a patch with comments addressed for final pass. > > > Source/WebCore/page/DragActions.h:39 > > + DragDestinationActionUpload = 8, > > It's quite confusing that this is a bit flag, but it's only used exclusively. I'm not sure what to suggest to improve the design, but at least a comment seems necessary here. > It was a bit flag already, I've only added one value. > > Source/WebCore/page/DragActions.h:40 > > DragDestinationActionAny = UINT_MAX > > What are the checks that match action to DragDestinationActionAny? Is it OK that we now have an additional action that's matching DragDestinationActionAny? Sorry, but I don't get this. DragDestinationAny was already UINT_MAX and has not changed. Could you explain? > > > Source/WebKit2/ChangeLog:9 > > + Now the the pasteboard access is performed only in the UI process, it is > > Typo "the the". > > > Source/WebKit2/ChangeLog:10 > > + necessary to create sanbox extensions for each file that is dropped into > > Typo: "sanbox". > All done. > > Source/WebKit2/Shared/SandboxExtension.h:115 > > +inline const SandboxExtension::Handle& SandboxExtension::HandleArray::operator[](size_t) const { return *m_data; } > > +inline SandboxExtension::Handle& SandboxExtension::HandleArray::operator[](size_t) { return *m_data; } > > This dereferences null, which is a bug. Fixed. > > > Source/WebKit2/Shared/mac/SandboxExtensionMac.mm:145 > > + for (size_t i = 0; i < size; i++) > > + decoder->decode(handles[i]); > > Shouldn't this also check decoding result? > Yes, done. > > Source/WebKit2/UIProcess/WebPageProxy.cpp:866 > > + SandboxExtension::HandleArray sandboxExtensionHandleArray; > > I like to use more descriptive names in such cases. Maybe "emptyArray"? Ditto below. > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:1697 > > + handles.reserve([files count]); > > + for (unsigned int i = 0; i < [files count]; i++) { > > s/unsigned int/unsigned/. > ok. > The name "reserve" is incorrect here. Reserving space in a container doesn't change its size, so handles[i] would still be a mistake after this reserve(). Ok, I'll change it to resize. > > > Source/WebKit2/UIProcess/API/mac/WKView.mm:1700 > > + if (![[NSFileManager defaultManager] fileExistsAtPath:file]) > > + continue; > > Does this return YES for directories too? It's legitimate to drop directories (especially bundles). Yes. Created attachment 132686 [details]
Patch5
Fixes gtk build and addresses comments.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Comment on attachment 132686 [details] Patch5 View in context: https://bugs.webkit.org/attachment.cgi?id=132686&action=review > Sorry, but I don't get this. DragDestinationAny was already UINT_MAX and has not changed. Could you explain? Had a look at this with Enrica. All these values are actually exclusive. The reason this is a bitmap is that clients provide allowed actions in the form of a single value. Eventually, it may be better to decouple WebKit1 API from WebCore logic here, and have more sensible DragAction.h. Enrica is going to improve comments when landing. > Source/WebKit2/Shared/mac/SandboxExtensionMac.mm:111 > +void SandboxExtension::HandleArray::resize(size_t size) > +{ > + if (!size) > + return; > + > + if (m_data) > + delete[] m_data; > + > + m_data = new SandboxExtension::Handle[size]; > + m_size = size; > +} This is not a proper resize - a resize doesn't delete original data. Please rename. It seems like this patch broke editing/pasteboard/dataTransfer-setData-getData.html on Snow Leopard: http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r111825%20(38164)/results.html http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=editing%2Fpasteboard%2FdataTransfer-setData-getData.html I will look into it. |