Bug 144096 - [UNIX] Do not allow copies of IPC::Attachment
Summary: [UNIX] Do not allow copies of IPC::Attachment
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 145967
Blocks:
  Show dependency treegraph
 
Reported: 2015-04-23 05:16 PDT by Carlos Garcia Campos
Modified: 2015-06-15 08:46 PDT (History)
2 users (show)

See Also:


Attachments
Patch (12.39 KB, patch)
2015-04-23 05:27 PDT, Carlos Garcia Campos
darin: review+
Details | Formatted Diff | Diff
Patch for landing (11.83 KB, patch)
2015-04-23 08:40 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2015-04-23 05:16:08 PDT
See discussion in bug #144046. It ensures that the file descriptor ownership is always correctly transfered. This way we can remove the dispose() method to explicitly close the file descriptor and always close it in the Attachment destructor (unless explicitly transferred to IPC::Connection or SharedMemory). It simplifies the code and ensure we don't leak file descriptors.
Comment 1 Carlos Garcia Campos 2015-04-23 05:27:05 PDT
Created attachment 251427 [details]
Patch
Comment 2 Darin Adler 2015-04-23 08:05:33 PDT
Comment on attachment 251427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251427&action=review

This is OK, but could be improved.

> Source/WebKit2/Platform/IPC/ArgumentDecoder.cpp:217
> +    attachment = WTF::move(m_attachments.last());
>      m_attachments.removeLast();

Should instead be:

    attachment = m_attachments.takeLast();

> Source/WebKit2/Platform/IPC/Attachment.cpp:59
> +    encoder.addAttachment(Attachment(*this));
> +#if USE(UNIX_DOMAIN_SOCKETS)
> +    // The encoder takes the onwership of our file descriptor.
> +    m_fileDescriptor = -1;
> +#endif

Should instead be:

    encoding.addAttachment(WTF::move(*this));

> Source/WebKit2/Platform/IPC/Attachment.h:85
> +    Attachment(const Attachment&) = default;
> +    Attachment& operator=(Attachment&) = default;

Why default? Why not delete? I think this is only needed because of the incorrect code in Attachment::encode.
Comment 3 Darin Adler 2015-04-23 08:12:22 PDT
Comment on attachment 251427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=251427&action=review

>> Source/WebKit2/Platform/IPC/Attachment.cpp:59
>> +#endif
> 
> Should instead be:
> 
>     encoding.addAttachment(WTF::move(*this));

Might require const_cast. It’s a design problem, I think, that encoding modifies the attachment, but the encode function is marked const.
Comment 4 Carlos Garcia Campos 2015-04-23 08:25:56 PDT
Yes, I didn't know how to do the move(this), but this:

encoder.addAttachment(WTF::move(*const_cast<Attachment*>(this)));

made the trick :-) Thanks!
Comment 5 Carlos Garcia Campos 2015-04-23 08:26:52 PDT
I don't think we need to use = delete, because that's the default when the move constructor and assignment operation are defined.
Comment 6 Carlos Garcia Campos 2015-04-23 08:34:36 PDT
And we don't need to make the file descriptor mutable either
Comment 7 Carlos Garcia Campos 2015-04-23 08:40:13 PDT
Created attachment 251437 [details]
Patch for landing
Comment 8 Carlos Garcia Campos 2015-04-23 09:54:22 PDT
Committed r183189: <http://trac.webkit.org/changeset/183189>