Bug 144046 - [UNIX] Simplify the file descriptor handling in SharedMemory
Summary: [UNIX] Simplify the file descriptor handling in SharedMemory
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:
Blocks:
 
Reported: 2015-04-22 09:30 PDT by Carlos Garcia Campos
Modified: 2015-04-23 05:27 PDT (History)
1 user (show)

See Also:


Attachments
Patch (10.16 KB, patch)
2015-04-22 09:41 PDT, Carlos Garcia Campos
darin: review+
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-22 09:30:40 PDT
There are some weird things in SharedMemoryUnix when handling the ownership of the file descriptor. It could be simplified and clarified using IPC::Attachment instead of fd + size
Comment 1 Carlos Garcia Campos 2015-04-22 09:41:02 PDT
Created attachment 251329 [details]
Patch
Comment 2 Darin Adler 2015-04-22 10:44:38 PDT
Comment on attachment 251329 [details]
Patch

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

Looks OK.

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

I think you want = delete here, not = default. It doesn’t seem OK to just copy a file descriptor!
Comment 3 Carlos Garcia Campos 2015-04-23 00:07:58 PDT
(In reply to comment #2)
> Comment on attachment 251329 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251329&action=review
> 
> Looks OK.

Thanks for the review.

> > Source/WebKit2/Platform/IPC/Attachment.h:59
> > +    Attachment(const Attachment&) = default;
> > +    Attachment& operator=(Attachment&) = default;
> 
> I think you want = delete here, not = default. It doesn’t seem OK to just
> copy a file descriptor!

Yes, I first tried to make the class non copyable, and release the fd in the destructor to get rid of the dispose() method and the explicit attachment dispose made by the ArgumentEncoder, ArgumentDecoder and ConnectionUnix. But it was not possible, or I didn't find the way. Encoding/decoding attachments is adding them to the encoder/decoder class that take the ownership of the file descriptors until they are passed to the platform specific send/receive IPC methods. The thing is that Attachment::encode() is const and does encoder.addAttachment(*this); Even if we change ArgumentEncoder::addAttachment to receive an Attachment&& we can't WTF::move(*this) in the const encode method. I'm not a C++ expert, so maybe there's a way. In the end I forgot about that part and left the explicit dispose, but avoiding the manual fd = -1 in several places and renaming the confusing adoptFrom, releaseTo methods.
Comment 4 Carlos Garcia Campos 2015-04-23 00:30:04 PDT
Committed r183176: <http://trac.webkit.org/changeset/183176>
Comment 5 Carlos Garcia Campos 2015-04-23 05:11:38 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 251329 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=251329&action=review
> > 
> > Looks OK.
> 
> Thanks for the review.
> 
> > > Source/WebKit2/Platform/IPC/Attachment.h:59
> > > +    Attachment(const Attachment&) = default;
> > > +    Attachment& operator=(Attachment&) = default;
> > 
> > I think you want = delete here, not = default. It doesn’t seem OK to just
> > copy a file descriptor!
> 
> Yes, I first tried to make the class non copyable, and release the fd in the
> destructor to get rid of the dispose() method and the explicit attachment
> dispose made by the ArgumentEncoder, ArgumentDecoder and ConnectionUnix. But
> it was not possible, or I didn't find the way. Encoding/decoding attachments
> is adding them to the encoder/decoder class that take the ownership of the
> file descriptors until they are passed to the platform specific send/receive
> IPC methods. The thing is that Attachment::encode() is const and does
> encoder.addAttachment(*this); Even if we change
> ArgumentEncoder::addAttachment to receive an Attachment&& we can't
> WTF::move(*this) in the const encode method. I'm not a C++ expert, so maybe
> there's a way. In the end I forgot about that part and left the explicit
> dispose, but avoiding the manual fd = -1 in several places and renaming the
> confusing adoptFrom, releaseTo methods.

Zan suggested to make the copy constructor and assignment private and use it only in ::encode() doing encoder.addAttachment(Attachment(*this)); That way we can ensure that any public assignment is a move. We also need to make the file descriptor member mutable and explicitly set to -1 in ::encode() since the ownership is transfered to the encoder. It's tricky, but it works. I'll submit a new bug.
Comment 6 Carlos Garcia Campos 2015-04-23 05:27:25 PDT
See https://bugs.webkit.org/show_bug.cgi?id=144096