Bug 60621 – [UNIX] Allow sending null handles in messages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Martin Robinson]

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.

[Anders Carlsson]

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?

[Carlos Garcia Campos]

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

[Anders Carlsson]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Your are right, we should be able to send null handles. I'm already working on a patch for ConnectionUnix to allow it.

[Carlos Garcia Campos]

Created attachment 93295 [details]
Patch

[Early Warning System Bot]

Comment on attachment 93295 [details]
Patch

Attachment 93295 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8684816

[Carlos Garcia Campos]

(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

[Martin Robinson]

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?

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

Created attachment 94062 [details]
Updated patch according to review

[Early Warning System Bot]

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

[Kimmo Kinnunen]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Viatcheslav Ostapenko]

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

[Carlos Garcia Campos]

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

[Carlos Garcia Campos]

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.

[Early Warning System Bot]

Comment on attachment 98330 [details]
Updated patch

Attachment 98330 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/8923982

[Carlos Garcia Campos]

Committed r91016: <http://trac.webkit.org/changeset/91016>