Bug 81153 – WebKit2: create sandbox extensions for files that are dropped in an input control

• Status: NEW
• Component: HTML Editing

[Enrica Casucci]

This bug track the work required to create the sandbox extensions needed to upload files to input controls.

<rdar://problem/11031207>

[Enrica Casucci]

Created attachment 131916 [details]
Patch

[Early Warning System Bot]

Comment on attachment 131916 [details]
Patch

Attachment 131916 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11957191

[Build Bot]

Comment on attachment 131916 [details]
Patch

Attachment 131916 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11957198

[Adele Peterson]

Comment on attachment 131916 [details]
Patch

r- so you can fix the EWS warnings for QT and Win bots

[Enrica Casucci]

Created attachment 131929 [details]
Patch2

Fixes the Qt and Win builds.

[WebKit Review Bot]

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.

[Enrica Casucci]

Will fix the style issue before landing the patch.

[Early Warning System Bot]

Comment on attachment 131929 [details]
Patch2

Attachment 131929 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11957247

[Gustavo Noronha (kov)]

Comment on attachment 131929 [details]
Patch2

Attachment 131929 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11956301

[Enrica Casucci]

Created attachment 131949 [details]
Patch3

One more attempt at fixing the QT, win and gtk builds.

[Build Bot]

Comment on attachment 131949 [details]
Patch3

Attachment 131949 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/11953492

[Early Warning System Bot]

Comment on attachment 131949 [details]
Patch3

Attachment 131949 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/11953502

[Enrica Casucci]

Created attachment 131960 [details]
Patch4

One more try...

[Gustavo Noronha (kov)]

Comment on attachment 131960 [details]
Patch4

Attachment 131960 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11957428

[Anders Carlsson]

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.

[Alexey Proskuryakov]

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).

[Enrica Casucci]

(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.

[Enrica Casucci]

(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.

[Enrica Casucci]

Created attachment 132686 [details]
Patch5

Fixes gtk build and addresses comments.

[WebKit Review Bot]

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

[Alexey Proskuryakov]

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.

[Ryosuke Niwa]

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

[Enrica Casucci]

I will look into it.