[Xcb] [PATCH 4/8] Add xcb_send_fd API
Keith Packard
keithp at keithp.com
Thu Nov 7 13:21:21 PST 2013
Uli Schlachter <psychon at znc.in> writes:
> On 07.11.2013 13:24, Keith Packard wrote:
>> Uli Schlachter <psychon at znc.in> writes:
>>
>>> Having thought about this some more in the shower, I would suggest to turn
>>> xcb_send_fd() into a private _send_fd() and instead add
>>> xcb_send_request_with_fds() (or something like that).
>>
>> This isn't necessary; as I explained, the X server just holds onto FDs
>> that the client sends until a request consumes them. Yes, it has to deal
>> with clients sending unexpected FDs, there's nothing special about this.
>
> Ok, but then xcb needs to do the same (see below).
>
> [...]
>>> - Does the new sendmsg() call work correctly on non-unix sockets when no FDs are
>>> transmitted?
>>
>> You'll note that sendmsg is *not* called when we have no file
>> descriptors to send.
>
> Ah, sorry, I did indeed miss that part.
>
>>> - Should we have some API to test if sending FDs works at all or if xcb just
>>> ignores the FDs?
>>
>> I'm not entirely sure how you'd do that without actually trying to send
>> an FD, given that XCB can be handed a pre-connected file descriptor.
>>
>>> - What happens with partial reads? Let's say we send 4KiB of requests to the
>>> server in one sendmsg() call and the last request has FDs attached. For whatever
>>> reason, the server only reads 2KiB of data from the socket. Will the FDs be
>>> handled correctly in this case? What if the FDs are for the first request
>>> instead of the last one?
>>
>> The FDs are effectively attached to the first byte of the write; read
>> that bytes and you get all of the FDs sent there. So, if you perform
>> a 4kB write with 10 file descriptors, and read only 2kB, then you'll get
>> all 10 file descriptors as you've read the relevant byte.
>
> Ok, seems like the server handles this correctly then.
>
> However, now I wonder about patch 5/8 "Add support for receiving fds" which does
> not do this.
>
> If I understood the code correctly, _xcb_in_read() will close the connection
> with an error if a partial read happens and thus we get FDs before the
> corresponding event/reply.
>
> In other words, the server handles partial reads correctly, xcb
> doesn't
Right, I was thinking that XCB would always read the full reply if it
got any part of it, when it fact it only does that after it receives the
first 32 bytes.
Easy enough to fix -- we don't close the descriptors when we have a
partial packet left in the buffer, and then we allow fds to accumulate
in the input structure instead of assuming that will be empty. I'd
written this code once before I thought this couldn't happen.
diff --git a/src/xcb_in.c b/src/xcb_in.c
index 795d93c..839f615 100644
--- a/src/xcb_in.c
+++ b/src/xcb_in.c
@@ -888,13 +888,17 @@ int _xcb_in_read(xcb_connection_t *c)
.iov_base = c->in.queue + c->in.queue_len,
.iov_len = sizeof(c->in.queue) - c->in.queue_len,
};
+ struct {
+ struct cmsghdr cmsghdr;
+ int fd[XCB_MAX_PASS_FD];
+ } fds;
struct msghdr msg = {
.msg_name = NULL,
.msg_namelen = 0,
.msg_iov = &iov,
.msg_iovlen = 1,
- .msg_control = &c->in.in_fd,
- .msg_controllen = sizeof (struct cmsghdr) + sizeof (int) * XCB_MAX_PASS_FD,
+ .msg_control = &fds,
+ .msg_controllen = sizeof (struct cmsghdr) + sizeof(int) * (XCB_MAX_PASS_FD - c->in.in_fd.nfd),
};
n = recvmsg(c->fd, &msg, 0);
@@ -914,12 +918,14 @@ int _xcb_in_read(xcb_connection_t *c)
#if HAVE_SENDMSG
if (msg.msg_controllen > sizeof (struct cmsghdr))
{
- if (c->in.in_fd.cmsghdr.cmsg_level == SOL_SOCKET &&
- c->in.in_fd.cmsghdr.cmsg_type == SCM_RIGHTS &&
- !((msg.msg_flags & MSG_TRUNC) ||
- (msg.msg_flags & MSG_CTRUNC)))
+ if (fds.cmsghdr.cmsg_level == SOL_SOCKET &&
+ fds.cmsghdr.cmsg_type == SCM_RIGHTS)
{
- c->in.in_fd.nfd = (msg.msg_controllen - sizeof (struct cmsghdr)) / sizeof (int);
+ int nfd = (msg.msg_controllen - sizeof (struct cmsghdr)) / sizeof (int);
+ memmove(&c->in.in_fd.fd[c->in.in_fd.nfd],
+ fds.fd,
+ nfd);
+ c->in.in_fd.nfd += nfd;
}
}
#endif
@@ -928,19 +934,24 @@ 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_CLOSED_FDPASSING_FAILED);
- return 0;
+ if (c->in.in_fd.nfd) {
+ c->in.in_fd.nfd -= c->in.in_fd.ifd;
+ memmove(&c->in.in_fd.fd[0],
+ &c->in.in_fd.fd[c->in.in_fd.ifd],
+ c->in.in_fd.nfd * sizeof (int));
+ c->in.in_fd.ifd = 0;
+
+ /* If we have any left-over file descriptors after emptying
+ * the input buffer, then the server sent some that we weren't
+ * expecting. Close them and mark the connection as broken;
+ */
+ if (c->in.queue_len == 0 && 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_CLOSED_FDPASSING_FAILED);
+ return 0;
+ }
}
#endif
#ifndef _WIN32
> Anyway, I guess I ran out of doubts for this patch (and now have new doubts
> about patch 5/8), so:
>
> Reviewed-By: Uli Schlachter <psychon at znc.in>
>
> Uli
--
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/20131107/575d8c8b/attachment.pgp>
More information about the xorg-devel
mailing list