Bug 60621

Summary: [UNIX] Allow sending null handles in messages
Product: WebKit Reporter: Carlos Garcia Campos <cgarcia>
Component: WebKit2Assignee: Carlos Garcia Campos <cgarcia>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, aroben, cmarcelo, kimmo.t.kinnunen, mrobinson, ostap73, sam, yael
Priority: P2 Keywords: Gtk, Qt
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Bug Depends on: 60629    
Bug Blocks: 56935    
Attachments:
Description Flags
Patch
andersca: review-
Patch
webkit-ews: commit-queue-
Updated patch according to review
webkit-ews: commit-queue-
Updated patch to current git master
none
Updated patch andersca: review+, webkit-ews: commit-queue-

Description Carlos Garcia Campos 2011-05-11 05:15:27 PDT
It doesn't work for gtk and qt port, since sendmsg() checks whether file descriptors are valid before sending the message, returning EBADF when they are not.
Comment 1 Carlos Garcia Campos 2011-05-11 05:20:44 PDT
Created attachment 93109 [details]
Patch

Instead of checking if the handle is null, we can simply check if the size has changed in the plugin process before creating a new backing store.
Comment 2 Martin Robinson 2011-05-11 09:18:36 PDT
Comment on attachment 93109 [details]
Patch

Looks sane to me, but I'd be more comfortable if the original authors of this code took a look as well.
Comment 3 Anders Carlsson 2011-05-11 09:21:30 PDT
Comment on attachment 93109 [details]
Patch

This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not?
Comment 4 Carlos Garcia Campos 2011-05-11 09:28:14 PDT
(In reply to comment #3)
> (From update of attachment 93109 [details])
> This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not?

It does it, when file descriptor is -1 the handle is null, but then it's impossible to send the message because sendmsg always fails.
Comment 5 Anders Carlsson 2011-05-11 09:31:57 PDT
(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 93109 [details] [details])
> > This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not?
> 
> It does it, when file descriptor is -1 the handle is null, but then it's impossible to send the message because sendmsg always fails.

But in that case, does the file descriptor even need to be passed to sendmsg?
Comment 6 Carlos Garcia Campos 2011-05-11 09:36:36 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 93109 [details] [details] [details])
> > > This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not?
> > 
> > It does it, when file descriptor is -1 the handle is null, but then it's impossible to send the message because sendmsg always fails.
> 
> But in that case, does the file descriptor even need to be passed to sendmsg?

well, it's part of the GeometryDidChange message, the handle is encoded as an attachment. Attachments are sent using control messages with cmsg_level = SOL_SOCKET, cmsg_type = SCM_RIGHTS which makes sendmsg check the file descriptors passed.
Comment 7 Carlos Garcia Campos 2011-05-12 08:05:19 PDT
Your are right, we should be able to send null handles. I'm already working on a patch for ConnectionUnix to allow it.
Comment 8 Carlos Garcia Campos 2011-05-12 09:41:20 PDT
Created attachment 93295 [details]
Patch
Comment 9 Early Warning System Bot 2011-05-12 09:51:17 PDT
Comment on attachment 93295 [details]
Patch

Attachment 93295 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8684816
Comment 10 Carlos Garcia Campos 2011-05-16 23:54:26 PDT
(In reply to comment #9)
> (From update of attachment 93295 [details])
> Attachment 93295 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/8684816

Qt build failed because this patch depends on patch attached to bug #60629
Comment 11 Martin Robinson 2011-05-18 10:25:53 PDT
Comment on attachment 93295 [details]
Patch

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

Look sane, but I'd like to at least get a nod from the personal who originally wrote this code.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:104
> +    void setType(Attachment::Type type) { m_type = type; }
> +
> +    Attachment::Type getType() { return m_type; }

No need for this extra newline.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:110
> +        ASSERT(m_type == Attachment::MappedMemoryType);
> +
> +        m_size = size;

Ditto.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:117
> +        ASSERT(m_type == Attachment::MappedMemoryType);
> +
> +        return m_size;

Ditto.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:124
> +    void setNull() { m_isNull = true; }
> +
> +    bool isNull() { return m_isNull; }
> +

Ditto.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:247
> +        size_t attachmentFDCount = 0;

Please call this something less similar to attachmentCount like attachmentFileDescriptorCount. I confused the two variables on my first read through.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:436
> +        int* fdptr = 0;

Should be fdPtr.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:442
> +            cmsg = CMSG_FIRSTHDR(&message);

It seems that you only use this insidde this block, so you should declare it here instead of above. It would be better to rename this to something more readable like firstCMSGHeader.

> Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:457
> +                // Fall trhough.

Please expand this. Why should this fall through?
Comment 12 Carlos Garcia Campos 2011-05-18 11:21:54 PDT
(In reply to comment #11)
> (From update of attachment 93295 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=93295&action=review
> 
> Look sane, but I'd like to at least get a nod from the personal who originally wrote this code.

Thanks for reviewing.

> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:104
> > +    void setType(Attachment::Type type) { m_type = type; }
> > +
> > +    Attachment::Type getType() { return m_type; }
> 
> No need for this extra newline.

I added these lines for consistency with MessageInfo class, but I can remove them.

> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:110
> > +        ASSERT(m_type == Attachment::MappedMemoryType);
> > +
> > +        m_size = size;
> 
> Ditto.
> 
> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:117
> > +        ASSERT(m_type == Attachment::MappedMemoryType);
> > +
> > +        return m_size;
> 
> Ditto.
> 
> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:124
> > +    void setNull() { m_isNull = true; }
> > +
> > +    bool isNull() { return m_isNull; }
> > +
> 
> Ditto.
> 
> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:247
> > +        size_t attachmentFDCount = 0;
> 
> Please call this something less similar to attachmentCount like attachmentFileDescriptorCount. I confused the two variables on my first read through.

Ok.

> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:436
> > +        int* fdptr = 0;
> 
> Should be fdPtr.

I haven't added that variable, it's in the current code, I'll rename it anyway. 

> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:442
> > +            cmsg = CMSG_FIRSTHDR(&message);
> 
> It seems that you only use this insidde this block, so you should declare it here instead of above.

Ok.

> It would be better to rename this to something more readable like firstCMSGHeader.

This is also current code, I'll rename it too.

> > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:457
> > +                // Fall trhough.
> 
> Please expand this. Why should this fall through?

Bacause both mapped mem and socket attachments have a file descriptor.
Comment 13 Carlos Garcia Campos 2011-05-19 05:22:29 PDT
Created attachment 94062 [details]
Updated patch according to review
Comment 14 Early Warning System Bot 2011-05-19 05:36:26 PDT
Comment on attachment 94062 [details]
Updated patch according to review

Attachment 94062 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8719071
Comment 15 Kimmo Kinnunen 2011-05-26 03:25:22 PDT
A comment as the original implementor:
The patch looks a bit too complex, e.g the AttachmentInfo seems a bit redundant.

What is the condition when the handle file descriptor is -1? I think that is the bug that should be solved? Does the problem appear as ASSERT Handle::encode?

If so, you should be able to solve it like following:

	Modified Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
diff --git a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
index 3dce1c2..9789f81 100644
--- a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
+++ b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
@@ -69,9 +69,13 @@ bool SharedMemory::Handle::isNull() const
 
 void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder* encoder) const
 {
-    ASSERT(!isNull());
+    if (isNull())
+        encoder->encode(false);
+    else {
+        encoder->encode(true);
+        encoder->encode(releaseToAttachment());
+    }
 
-    encoder->encode(releaseToAttachment());
 }
 
 bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& handle)
@@ -79,6 +83,13 @@ bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& han
     ASSERT_ARG(handle, !handle.m_size);
     ASSERT_ARG(handle, handle.isNull());
 
+    bool hasContent = false;
+    if (!decoder->decode(hasContent))
+        return false;
+
+    if (!hasContent)
+        return true;
+
     CoreIPC::Attachment attachment;
     if (!decoder->decode(attachment))
         return false;


Note, that's just pseudocode, I haven't tried it..


In general: also if only possible, it would be great if the bug reports would provide a bit more concrete details about the actual symptom. Eg. if it asserts or crashes, you should provide info about where an which conditions. Jumping to conclusions might be a bit non-productive..
Comment 16 Carlos Garcia Campos 2011-05-26 03:38:31 PDT
(In reply to comment #15)
> A comment as the original implementor:
> The patch looks a bit too complex, e.g the AttachmentInfo seems a bit redundant.
> 
> What is the condition when the handle file descriptor is -1? I think that is the bug that should be solved? Does the problem appear as ASSERT Handle::encode?

GeometryDidChange message sends a null handle on purpose when there's no need to create a new backing store. Receiver checks whether handle is null to decide whether to create a new backing store or not. I talked to Anders Carlsson and he confirmed that the expected behaviour so we should find a way to send a null handle. 

As I said before the problem of sending a null handle is that the file descriptor is -1. When sending the message using sendmsg(), it tries to dup the descriptors and when it finds the invalid file descritor, sendmsg fails with errno = EBADF and the message is not called. 

> If so, you should be able to solve it like following:
> 
>     Modified Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
> diff --git a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
> index 3dce1c2..9789f81 100644
> --- a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
> +++ b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp
> @@ -69,9 +69,13 @@ bool SharedMemory::Handle::isNull() const
> 
>  void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder* encoder) const
>  {
> -    ASSERT(!isNull());
> +    if (isNull())
> +        encoder->encode(false);
> +    else {
> +        encoder->encode(true);
> +        encoder->encode(releaseToAttachment());
> +    }
> 
> -    encoder->encode(releaseToAttachment());
>  }
> 
>  bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& handle)
> @@ -79,6 +83,13 @@ bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& han
>      ASSERT_ARG(handle, !handle.m_size);
>      ASSERT_ARG(handle, handle.isNull());
> 
> +    bool hasContent = false;
> +    if (!decoder->decode(hasContent))
> +        return false;
> +
> +    if (!hasContent)
> +        return true;
> +
>      CoreIPC::Attachment attachment;
>      if (!decoder->decode(attachment))
>          return false;
> 
> 
> Note, that's just pseudocode, I haven't tried it..
> 
> 
> In general: also if only possible, it would be great if the bug reports would provide a bit more concrete details about the actual symptom. Eg. if it asserts or crashes, you should provide info about where an which conditions. Jumping to conclusions might be a bit non-productive..

Yes, sorry, this bug report was created when working on other bugs, I explained the problem in bugs #57617 and #60629, and I talked about the issue with apple guys on irc.
Comment 17 Carlos Garcia Campos 2011-06-06 05:30:45 PDT
Created attachment 96078 [details]
Updated patch to current git master

ConnectionUnix.cpp file has changed a bit after r88147, so I've updated the patch to apply to current git master.
Comment 18 Viatcheslav Ostapenko 2011-06-15 09:54:23 PDT
(In reply to comment #17)
> Created an attachment (id=96078) [details]
> Updated patch to current git master
> 
> ConnectionUnix.cpp file has changed a bit after r88147, so I've updated the patch to apply to current git master.

Is it really depends now on 60629? It seems it can be applied without 60629. Am I right?
Comment 19 Carlos Garcia Campos 2011-06-15 09:59:07 PDT
(In reply to comment #18)
> (In reply to comment #17)
> > Created an attachment (id=96078) [details] [details]
> > Updated patch to current git master
> > 
> > ConnectionUnix.cpp file has changed a bit after r88147, so I've updated the patch to apply to current git master.
> 
> Is it really depends now on 60629? It seems it can be applied without 60629. Am I right?

No, it still requires 60629, because it adds the attachment types (SocketType and MappedType, this one renamed actually) used by this patch.
Comment 20 Carlos Garcia Campos 2011-06-23 03:15:04 PDT
Created attachment 98330 [details]
Updated patch

It simply removes a couple of asserts that were wrong and are not actually needed and fixes a memory corruption issue with the file descriptors buffer.
Comment 21 Early Warning System Bot 2011-06-23 03:23:10 PDT
Comment on attachment 98330 [details]
Updated patch

Attachment 98330 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8923982
Comment 22 Carlos Garcia Campos 2011-07-14 10:53:13 PDT
Committed r91016: <http://trac.webkit.org/changeset/91016>