Changeset 183189 in webkit

Timestamp:

Apr 23, 2015 9:53:05 AM (9 years ago)

Author:

Carlos Garcia Campos

Message:

[UNIX] Do not allow copies of IPC::Attachment
​https://bugs.webkit.org/show_bug.cgi?id=144096

Reviewed by Darin Adler.

It ensures that the file descriptor ownership is always correctly
transferred. 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.

• Platform/IPC/ArgumentDecoder.cpp:

(IPC::ArgumentDecoder::~ArgumentDecoder): Remove the code to
explicitly dispose attachments.
(IPC::ArgumentDecoder::removeAttachment): Use WTF::move().

• Platform/IPC/ArgumentEncoder.cpp:

(IPC::ArgumentEncoder::~ArgumentEncoder): Remove the code to
explicitly dispose attachments.
(IPC::ArgumentEncoder::addAttachment): Use WTF::move().
(IPC::ArgumentEncoder::releaseAttachments): Simplify by using WTF::move().

• Platform/IPC/ArgumentEncoder.h:
• Platform/IPC/Attachment.cpp:

(IPC::Attachment::encode): Move a copy of the attachment, and
reset the file descriptor, since the ownership is passed to the encoder.

• Platform/IPC/Attachment.h: Make copy constructor and assignment

private to not allow public copies. The only copy allowed is done
by Attachment::encode(). Make m_fileDescriptor mutable so that we
can reset it in Attachment::encode() after passing the ownership
to the encoder.

• Platform/IPC/unix/AttachmentUnix.cpp:

(IPC::Attachment::~Attachment): Close the file descriptor if it
hasn't been released explicitly.
(IPC::Attachment::dispose): Deleted.

• Platform/IPC/unix/ConnectionUnix.cpp:

(IPC::Connection::processMessage): Do not use AttachmentResourceGuard.
(IPC::Connection::sendOutgoingMessage): Ditto.
(IPC::AttachmentResourceGuard::AttachmentResourceGuard): Deleted.
(IPC::AttachmentResourceGuard::~AttachmentResourceGuard): Deleted.

• Platform/unix/SharedMemoryUnix.cpp:

(WebKit::SharedMemory::Handle::~Handle): Do not call clear().
(WebKit::SharedMemory::Handle::clear): Reset the attachment.

• UIProcess/WebInspectorProxy.cpp:

(WebKit::WebInspectorProxy::createInspectorPage): Use WTF::move().

• WebProcess/Plugins/PluginProcessConnectionManager.cpp:

(WebKit::PluginProcessConnectionManager::getPluginProcessConnection):
Call releaseFileDescriptor() instead of fileDescritpro() since the
ownership is passed to the connection.

Location:

trunk/Source/WebKit2

Files:

• ChangeLog (modified) (1 diff)
• Platform/IPC/ArgumentDecoder.cpp (modified) (2 diffs)
• Platform/IPC/ArgumentEncoder.cpp (modified) (2 diffs)
• Platform/IPC/ArgumentEncoder.h (modified) (1 diff)
• Platform/IPC/Attachment.cpp (modified) (1 diff)
• Platform/IPC/Attachment.h (modified) (2 diffs)
• Platform/IPC/unix/AttachmentUnix.cpp (modified) (1 diff)
• Platform/IPC/unix/ConnectionUnix.cpp (modified) (3 diffs)
• Platform/unix/SharedMemoryUnix.cpp (modified) (1 diff)
• UIProcess/WebInspectorProxy.cpp (modified) (1 diff)
• WebProcess/Plugins/PluginProcessConnectionManager.cpp (modified) (1 diff)

trunk/Source/WebKit2/ChangeLog

@@ +1 @@
+2015-04-23  Carlos Garcia Campos  <cgarcia@igalia.com>
+
+        [UNIX] Do not allow copies of IPC::Attachment
+        https://bugs.webkit.org/show_bug.cgi?id=144096
+
+        Reviewed by Darin Adler.
+
+        It ensures that the file descriptor ownership is always correctly
+        transferred. 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.
+
+        * Platform/IPC/ArgumentDecoder.cpp:
+        (IPC::ArgumentDecoder::~ArgumentDecoder): Remove the code to
+        explicitly dispose attachments.
+        (IPC::ArgumentDecoder::removeAttachment): Use WTF::move().
+        * Platform/IPC/ArgumentEncoder.cpp:
+        (IPC::ArgumentEncoder::~ArgumentEncoder): Remove the code to
+        explicitly dispose attachments.
+        (IPC::ArgumentEncoder::addAttachment): Use WTF::move().
+        (IPC::ArgumentEncoder::releaseAttachments): Simplify by using WTF::move().
+        * Platform/IPC/ArgumentEncoder.h:
+        * Platform/IPC/Attachment.cpp:
+        (IPC::Attachment::encode): Move a copy of the attachment, and
+        reset the file descriptor, since the ownership is passed to the encoder.
+        * Platform/IPC/Attachment.h: Make copy constructor and assignment
+        private to not allow public copies. The only copy allowed is done
+        by Attachment::encode(). Make m_fileDescriptor mutable so that we
+        can reset it in Attachment::encode() after passing the ownership
+        to the encoder.
+        * Platform/IPC/unix/AttachmentUnix.cpp:
+        (IPC::Attachment::~Attachment): Close the file descriptor if it
+        hasn't been released explicitly.
+        (IPC::Attachment::dispose): Deleted.
+        * Platform/IPC/unix/ConnectionUnix.cpp:
+        (IPC::Connection::processMessage): Do not use AttachmentResourceGuard.
+        (IPC::Connection::sendOutgoingMessage): Ditto.
+        (IPC::AttachmentResourceGuard::AttachmentResourceGuard): Deleted.
+        (IPC::AttachmentResourceGuard::~AttachmentResourceGuard): Deleted.
+        * Platform/unix/SharedMemoryUnix.cpp:
+        (WebKit::SharedMemory::Handle::~Handle): Do not call clear().
+        (WebKit::SharedMemory::Handle::clear): Reset the attachment.
+        * UIProcess/WebInspectorProxy.cpp:
+        (WebKit::WebInspectorProxy::createInspectorPage): Use WTF::move().
+        * WebProcess/Plugins/PluginProcessConnectionManager.cpp:
+        (WebKit::PluginProcessConnectionManager::getPluginProcessConnection):
+        Call releaseFileDescriptor() instead of fileDescritpro() since the
+        ownership is passed to the connection.
+
 2015-04-23  Alexey Proskuryakov  <ap@apple.com>
 

trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.cpp

@@ -48 +48 @@
     ASSERT(m_buffer);
     free(m_buffer);
-#if !USE(UNIX_DOMAIN_SOCKETS)
     // FIXME: We need to dispose of the mach ports in cases of failure.
-#else
-    Vector<Attachment>::iterator end = m_attachments.end();
-    for (Vector<Attachment>::iterator it = m_attachments.begin(); it != end; ++it)
-        it->dispose();
-#endif
 }
 
@@ -220 +214 @@
         return false;
 
-    attachment = m_attachments.last();
-    m_attachments.removeLast();
+    attachment = m_attachments.takeLast();
     return true;
 }

trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.cpp

@@ -71 +71 @@
     if (m_buffer != m_inlineBuffer)
         freeBuffer(m_buffer, m_bufferCapacity);
-
-#if !USE(UNIX_DOMAIN_SOCKETS)
     // FIXME: We need to dispose of the attachments in cases of failure.
-#else
-    for (size_t i = 0; i < m_attachments.size(); ++i)
-        m_attachments[i].dispose();
-#endif
 }
 
@@ -192 +186 @@
 }
 
-void ArgumentEncoder::addAttachment(const Attachment& attachment)
+void ArgumentEncoder::addAttachment(Attachment&& attachment)
 {
-    m_attachments.append(attachment);
+    m_attachments.append(WTF::move(attachment));
 }
 
 Vector<Attachment> ArgumentEncoder::releaseAttachments()
 {
-    Vector<Attachment> newList;
-    newList.swap(m_attachments);
-    return newList;
+    return WTF::move(m_attachments);
 }
 

trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h

@@ -66 +66 @@
     size_t bufferSize() const { return m_bufferSize; }
 
-    void addAttachment(const Attachment&);
+    void addAttachment(Attachment&&);
     Vector<Attachment> releaseAttachments();
     void reserve(size_t);

trunk/Source/WebKit2/Platform/IPC/Attachment.cpp

@@ -53 +53 @@
 void Attachment::encode(ArgumentEncoder& encoder) const
 {
-    encoder.addAttachment(*this);
+    encoder.addAttachment(WTF::move(*const_cast<Attachment*>(this)));
 }
 

trunk/Source/WebKit2/Platform/IPC/Attachment.h

@@ -56 +56 @@
     Attachment(Attachment&&);
     Attachment& operator=(Attachment&&);
-    Attachment(const Attachment&) = default;
-    Attachment& operator=(Attachment&) = default;
     Attachment(int fileDescriptor, size_t);
     Attachment(int fileDescriptor);
+    ~Attachment();
 #endif
 
@@ -76 +75 @@
     int releaseFileDescriptor() { int temp = m_fileDescriptor; m_fileDescriptor = -1; return temp; }
     int fileDescriptor() const { return m_fileDescriptor; }
-
-    void dispose();
 #endif
 

trunk/Source/WebKit2/Platform/IPC/unix/AttachmentUnix.cpp

@@ -68 +68 @@
 }
 
-void Attachment::dispose()
+Attachment::~Attachment()
 {
-    if (m_fileDescriptor == -1)
-        return;
-
-    closeWithRetry(m_fileDescriptor);
-    m_fileDescriptor = -1;
+    if (m_fileDescriptor != -1)
+        closeWithRetry(m_fileDescriptor);
 }
 

trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp

@@ -156 +156 @@
 }
 
-template<class T, class iterator>
-class AttachmentResourceGuard {
-public:
-    AttachmentResourceGuard(T& attachments)
-        : m_attachments(attachments)
-    {
-    }
-    ~AttachmentResourceGuard()
-    {
-        iterator end = m_attachments.end();
-        for (iterator i = m_attachments.begin(); i != end; ++i)
-            i->dispose();
-    }
-private:
-    T& m_attachments;
-};
-
 bool Connection::processMessage()
 {
@@ -215 +198 @@
 
     Vector<Attachment> attachments(attachmentCount);
-    AttachmentResourceGuard<Vector<Attachment>, Vector<Attachment>::iterator> attachementDisposer(attachments);
     RefPtr<WebKit::SharedMemory> oolMessageBody;
 
@@ -424 +406 @@
 
     Vector<Attachment> attachments = encoder->releaseAttachments();
-    AttachmentResourceGuard<Vector<Attachment>, Vector<Attachment>::iterator> attachementDisposer(attachments);
-
     if (attachments.size() > (attachmentMaxAmount - 1)) {
         ASSERT_NOT_REACHED();

trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp

@@ -53 +53 @@
 SharedMemory::Handle::~Handle()
 {
-    clear();
 }
 
 void SharedMemory::Handle::clear()
 {
-    m_attachment.dispose();
+    m_attachment = IPC::Attachment();
 }
 

trunk/Source/WebKit2/UIProcess/WebInspectorProxy.cpp

@@ -487 +487 @@
 
     m_underTest = underTest;
-    m_connectionIdentifier = connectionIdentifier;
+    m_connectionIdentifier = WTF::move(connectionIdentifier);
 
     m_inspectorPage->process().send(Messages::WebInspectorUI::EstablishConnection(m_connectionIdentifier, m_inspectedPage->pageID(), m_underTest), m_inspectorPage->pageID());

trunk/Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp

@@ -77 +77 @@
 #if OS(DARWIN)
     IPC::Connection::Identifier connectionIdentifier(encodedConnectionIdentifier.port());
+#elif USE(UNIX_DOMAIN_SOCKETS)
+    IPC::Connection::Identifier connectionIdentifier = encodedConnectionIdentifier.releaseFileDescriptor();
+#endif
     if (IPC::Connection::identifierIsNull(connectionIdentifier))
-        return 0;
-#elif USE(UNIX_DOMAIN_SOCKETS)
-    IPC::Connection::Identifier connectionIdentifier = encodedConnectionIdentifier.fileDescriptor();
-    if (connectionIdentifier == -1)
-        return 0;
-#endif
+        return nullptr;
 
     RefPtr<PluginProcessConnection> pluginProcessConnection = PluginProcessConnection::create(this, pluginProcessToken, connectionIdentifier, supportsAsynchronousInitialization);