Bug 144096 – [UNIX] Do not allow copies of IPC::Attachment

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 251427 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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.

[Carlos Garcia Campos]

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!

[Carlos Garcia Campos]

I don't think we need to use = delete, because that's the default when the move constructor and assignment operation are defined.

[Carlos Garcia Campos]

And we don't need to make the file descriptor mutable either

[Carlos Garcia Campos]

Created attachment 251437 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r183189: <http://trac.webkit.org/changeset/183189>