Summary: | [UNIX] Do not allow copies of IPC::Attachment | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | WebKit2 | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | darin, zan | ||||||
Priority: | P2 | Keywords: | Gtk | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Bug Depends on: | 145967 | ||||||||
Bug Blocks: | |||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2015-04-23 05:16:08 PDT
Created attachment 251427 [details]
Patch
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 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. 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! I don't think we need to use = delete, because that's the default when the move constructor and assignment operation are defined. And we don't need to make the file descriptor mutable either Created attachment 251437 [details]
Patch for landing
Committed r183189: <http://trac.webkit.org/changeset/183189> |