[webkit-changes] [183189] trunk/Source/WebKit2

Thu, 23 Apr 2015 09:54:05 -0700

Title: [183189] trunk/Source/WebKit2
Revision
183189
Author
[email protected]
Date
2015-04-23 09:53:05 -0700 (Thu, 23 Apr 2015)

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

Modified Paths

Diff

Modified: trunk/Source/WebKit2/ChangeLog (183188 => 183189)


--- trunk/Source/WebKit2/ChangeLog	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/ChangeLog	2015-04-23 16:53:05 UTC (rev 183189)
@@ -1,3 +1,54 @@
+2015-04-23  Carlos Garcia Campos  <[email protected]>
+
+        [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  <[email protected]>
 
         Build fix.

Modified: trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.cpp (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/ArgumentDecoder.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -47,13 +47,7 @@
 {
     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
 }
 
 static inline uint8_t* roundUpToAlignment(uint8_t* ptr, unsigned alignment)
@@ -219,8 +213,7 @@
     if (m_attachments.isEmpty())
         return false;
 
-    attachment = m_attachments.last();
-    m_attachments.removeLast();
+    attachment = m_attachments.takeLast();
     return true;
 }

Modified: trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.cpp (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -70,13 +70,7 @@
 {
     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
 }
 
 static inline size_t roundUpToAlignment(size_t value, unsigned alignment)
@@ -191,16 +185,14 @@
     copyValueToBuffer(n, buffer);
 }
 
-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);
 }
 
 } // namespace IPC

Modified: trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/ArgumentEncoder.h	2015-04-23 16:53:05 UTC (rev 183189)
@@ -65,7 +65,7 @@
     uint8_t* buffer() const { return m_buffer; }
     size_t bufferSize() const { return m_bufferSize; }
 
-    void addAttachment(const Attachment&);
+    void addAttachment(Attachment&&);
     Vector<Attachment> releaseAttachments();
     void reserve(size_t);

Modified: trunk/Source/WebKit2/Platform/IPC/Attachment.cpp (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/Attachment.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/Attachment.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -52,7 +52,7 @@
 
 void Attachment::encode(ArgumentEncoder& encoder) const
 {
-    encoder.addAttachment(*this);
+    encoder.addAttachment(WTF::move(*const_cast<Attachment*>(this)));
 }
 
 bool Attachment::decode(ArgumentDecoder& decoder, Attachment& attachment)

Modified: trunk/Source/WebKit2/Platform/IPC/Attachment.h (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/Attachment.h	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/Attachment.h	2015-04-23 16:53:05 UTC (rev 183189)
@@ -55,10 +55,9 @@
 #elif USE(UNIX_DOMAIN_SOCKETS)
     Attachment(Attachment&&);
     Attachment& operator=(Attachment&&);
-    Attachment(const Attachment&) = default;
-    Attachment& operator=(Attachment&) = default;
     Attachment(int fileDescriptor, size_t);
     Attachment(int fileDescriptor);
+    ~Attachment();
 #endif
 
     Type type() const { return m_type; }
@@ -75,8 +74,6 @@
 
     int releaseFileDescriptor() { int temp = m_fileDescriptor; m_fileDescriptor = -1; return temp; }
     int fileDescriptor() const { return m_fileDescriptor; }
-
-    void dispose();
 #endif
 
     void encode(ArgumentEncoder&) const;

Modified: trunk/Source/WebKit2/Platform/IPC/unix/AttachmentUnix.cpp (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/unix/AttachmentUnix.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/unix/AttachmentUnix.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -67,13 +67,10 @@
     return *this;
 }
 
-void Attachment::dispose()
+Attachment::~Attachment()
 {
-    if (m_fileDescriptor == -1)
-        return;
-
-    closeWithRetry(m_fileDescriptor);
-    m_fileDescriptor = -1;
+    if (m_fileDescriptor != -1)
+        closeWithRetry(m_fileDescriptor);
 }
 
 } // namespace IPC

Modified: trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp (183188 => 183189)


--- trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -155,23 +155,6 @@
     m_isConnected = false;
 }
 
-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()
 {
     if (m_readBufferSize < sizeof(MessageInfo))
@@ -214,7 +197,6 @@
     }
 
     Vector<Attachment> attachments(attachmentCount);
-    AttachmentResourceGuard<Vector<Attachment>, Vector<Attachment>::iterator> attachementDisposer(attachments);
     RefPtr<WebKit::SharedMemory> oolMessageBody;
 
     size_t fdIndex = 0;
@@ -423,8 +405,6 @@
     COMPILE_ASSERT(sizeof(MessageInfo) + attachmentMaxAmount * sizeof(size_t) <= messageMaxSize, AttachmentsFitToMessageInline);
 
     Vector<Attachment> attachments = encoder->releaseAttachments();
-    AttachmentResourceGuard<Vector<Attachment>, Vector<Attachment>::iterator> attachementDisposer(attachments);
-
     if (attachments.size() > (attachmentMaxAmount - 1)) {
         ASSERT_NOT_REACHED();
         return false;

Modified: trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp (183188 => 183189)


--- trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -52,12 +52,11 @@
 
 SharedMemory::Handle::~Handle()
 {
-    clear();
 }
 
 void SharedMemory::Handle::clear()
 {
-    m_attachment.dispose();
+    m_attachment = IPC::Attachment();
 }
 
 bool SharedMemory::Handle::isNull() const

Modified: trunk/Source/WebKit2/UIProcess/WebInspectorProxy.cpp (183188 => 183189)


--- trunk/Source/WebKit2/UIProcess/WebInspectorProxy.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/UIProcess/WebInspectorProxy.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -486,7 +486,7 @@
         return;
 
     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());

Modified: trunk/Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp (183188 => 183189)


--- trunk/Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp	2015-04-23 16:08:43 UTC (rev 183188)
+++ trunk/Source/WebKit2/WebProcess/Plugins/PluginProcessConnectionManager.cpp	2015-04-23 16:53:05 UTC (rev 183189)
@@ -76,13 +76,11 @@
 
 #if OS(DARWIN)
     IPC::Connection::Identifier connectionIdentifier(encodedConnectionIdentifier.port());
-    if (IPC::Connection::identifierIsNull(connectionIdentifier))
-        return 0;
 #elif USE(UNIX_DOMAIN_SOCKETS)
-    IPC::Connection::Identifier connectionIdentifier = encodedConnectionIdentifier.fileDescriptor();
-    if (connectionIdentifier == -1)
-        return 0;
+    IPC::Connection::Identifier connectionIdentifier = encodedConnectionIdentifier.releaseFileDescriptor();
 #endif
+    if (IPC::Connection::identifierIsNull(connectionIdentifier))
+        return nullptr;
 
     RefPtr<PluginProcessConnection> pluginProcessConnection = PluginProcessConnection::create(this, pluginProcessToken, connectionIdentifier, supportsAsynchronousInitialization);
     m_pluginProcessConnections.append(pluginProcessConnection);

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to