[PATCH 4/7] Add support for receiving fds in replies

[Keith Packard]

Mark Kettenis <mark.kettenis at xs4all.nl> writes:

> Same comment about not using the CMSG_ API properly.  Error handling
> of the recvmsg(2) call isn't adequate either.  Probably should
> consider MSG_TRUNC and MSG_CTRUNC as fatal errors and clean up any
> file descriptors that you did receive like we discussed for libxtrans.

MSG_TRUNC "can't happen", but MSG_CTRUNC would indicate an X server
error. Here's a patch that checks for both of them:

--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -882,6 +882,16 @@ int _xcb_in_read(xcb_connection_t *c)
     c->in.in_fd.ifd = 0;
     c->in.in_fd.nfd = 0;
     n = recvmsg(c->fd, &msg, 0);
+
+    /* Check for truncation errors. Only MSG_CTRUNC is
+     * probably possible here, which would indicate that
+     * the sender tried to transmit more than XCB_MAX_PASS_FD
+     * file descriptors.
+     */
+    if (msg.msg_flags & (MSG_TRUNC|MSG_CTRUNC)) {
+        _xcb_conn_shutdown(c, XCB_CONN_ERROR);
+        return 0;
+    }

> I don't see a mechanism to "flush out" file descriptors that were
> received but not consumed, i.e. if a nasty server sent you some file
> descriptors together with a request for which you didn't expect any.
> Am I missing something?

You may have missed the abort at the top of _xcb_in_read where the
application will just exit if extra file descriptors are received.

How about this instead:

--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -878,9 +878,6 @@ int _xcb_in_read(xcb_connection_t *c)
         .msg_control = &c->in.in_fd,
         .msg_controllen = sizeof (struct cmsghdr) + sizeof (int) * XCB_MAX_PASS
     };
-    assert(c->in.in_fd.ifd == c->in.in_fd.nfd);
-    c->in.in_fd.ifd = 0;
-    c->in.in_fd.nfd = 0;
     n = recvmsg(c->fd, &msg, 0);
 
     /* Check for truncation errors. Only MSG_CTRUNC is
@@ -912,6 +909,22 @@ int _xcb_in_read(xcb_connection_t *c)
     }
     while(read_packet(c))
         /* empty */;
+#if HAVE_SENDMSG
+    c->in.in_fd.nfd -= c->in.in_fd.ifd;
+    c->in.in_fd.ifd = 0;
+
+    /* If we have any left-over file descriptors, then
+     * the server sent some that we weren't expecting.
+     * Close them and mark the connection as broken;
+     */
+    if (c->in.in_fd.nfd != 0) {
+        int i;
+        for (i = 0; i < c->in.in_fd.nfd; i++)
+            close(c->in.in_fd.fd[i]);
+        _xcb_conn_shutdown(c, XCB_CONN_ERROR);
+        return 0;
+    }
+#endif
 #ifndef _WIN32
     if((n > 0) || (n < 0 && errno == EAGAIN))
 #else

>> +    return nfd * sizeof (int);
>
> wouldn't it make more sense to simply return nfd here?

How about just making it a boolean function; that's all the caller cares
about:

--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -115,10 +115,10 @@ static int read_fds(xcb_connection_t *c, int *fds, int nfd
     int infd = c->in.in_fd.nfd - c->in.in_fd.ifd;
 
     if (nfd > infd)
-        return -1;
+        return 0;
     memcpy(fds, ifds, nfd * sizeof (int));
     c->in.in_fd.ifd += nfd;
-    return nfd * sizeof (int);
+    return 1;
 }
 #endif
 
@@ -279,7 +279,7 @@ static int read_packet(xcb_connection_t *c)
 #if HAVE_SENDMSG
     if (nfd)
     {
-        if (read_fds(c, (int *) &((char *) buf)[length], nfd) <= 0)
+        if (!read_fds(c, (int *) &((char *) buf)[length], nfd))
         {
             free(buf);
             return 0;

Thanks much for your review.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 827 bytes
Desc: not available
URL: <http://lists.x.org/archives/xorg-devel/attachments/20131106/53d1d50c/attachment-0001.pgp>