Bug 81153

Summary: WebKit2: create sandbox extensions for files that are dropped in an input control
Product: WebKit Reporter: Enrica Casucci <enrica>
Component: HTML EditingAssignee: 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 Flags
Patch
adele: review-, webkit-ews: commit-queue-
Patch2
webkit-ews: commit-queue-
Patch3
buildbot: commit-queue-
Patch4
ap: review-, gustavo: commit-queue-
Patch5 ap: review+, ap: commit-queue-

Description Enrica Casucci 2012-03-14 13:45:02 PDT
This bug track the work required to create the sandbox extensions needed to upload files to input controls.

<rdar://problem/11031207>
Comment 1 Enrica Casucci 2012-03-14 13:57:15 PDT
Created attachment 131916 [details]
Patch
Comment 2 Early Warning System Bot 2012-03-14 14:11:33 PDT
Comment on attachment 131916 [details]
Patch

Attachment 131916 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11957191
Comment 3 Build Bot 2012-03-14 14:29:17 PDT
Comment on attachment 131916 [details]
Patch

Attachment 131916 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11957198
Comment 4 Adele Peterson 2012-03-14 15:02:26 PDT
Comment on attachment 131916 [details]
Patch

r- so you can fix the EWS warnings for QT and Win bots
Comment 5 Enrica Casucci 2012-03-14 15:15:05 PDT
Created attachment 131929 [details]
Patch2

Fixes the Qt and Win builds.
Comment 6 WebKit Review Bot 2012-03-14 15:19:43 PDT
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.
Comment 7 Enrica Casucci 2012-03-14 15:22:39 PDT
Will fix the style issue before landing the patch.
Comment 8 Early Warning System Bot 2012-03-14 15:27:53 PDT
Comment on attachment 131929 [details]
Patch2

Attachment 131929 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11957247
Comment 9 Gustavo Noronha (kov) 2012-03-14 15:54:01 PDT
Comment on attachment 131929 [details]
Patch2

Attachment 131929 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11956301
Comment 10 Enrica Casucci 2012-03-14 16:09:54 PDT
Created attachment 131949 [details]
Patch3

One more attempt at fixing the QT, win and gtk builds.
Comment 11 Build Bot 2012-03-14 16:35:13 PDT
Comment on attachment 131949 [details]
Patch3

Attachment 131949 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11953492
Comment 12 Early Warning System Bot 2012-03-14 16:43:02 PDT
Comment on attachment 131949 [details]
Patch3

Attachment 131949 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11953502
Comment 13 Enrica Casucci 2012-03-14 16:55:13 PDT
Created attachment 131960 [details]
Patch4

One more try...
Comment 14 Gustavo Noronha (kov) 2012-03-14 21:10:40 PDT
Comment on attachment 131960 [details]
Patch4

Attachment 131960 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11957428
Comment 15 Anders Carlsson 2012-03-15 18:19:17 PDT
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 16 Alexey Proskuryakov 2012-03-16 10:41:48 PDT
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).
Comment 17 Enrica Casucci 2012-03-19 14:34:42 PDT
(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.
Comment 18 Enrica Casucci 2012-03-19 15:22:30 PDT
(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.
Comment 19 Enrica Casucci 2012-03-19 15:28:42 PDT
Created attachment 132686 [details]
Patch5

Fixes gtk build and addresses comments.
Comment 20 WebKit Review Bot 2012-03-19 15:31:28 PDT
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 21 Alexey Proskuryakov 2012-03-19 16:37:55 PDT
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.
Comment 23 Enrica Casucci 2012-03-23 09:35:07 PDT
I will look into it.