[UNIX] Simplify the file descriptor handling in SharedMemory

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 251329 [details]
Patch

[Darin Adler]

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!

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Committed r183176: <http://trac.webkit.org/changeset/183176>

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

See https://bugs.webkit.org/show_bug.cgi?id=144096