> From: Keith Packard <[email protected]> > Date: Fri, 01 Nov 2013 15:08:55 -0700 > > > Keith. This implementation isn't quite right as it doesn't use the > > proper CMSG_ macros to manipulate the ancillary data object > > information. You get way with this on Linux, because it deviates from > > POSIX and declares the cmsg_len member as size_t, which means no > > additional padding between the cmsghdr and the data array is > > necessary. But on other systems this code won't work. Attached is an > > (untested) diff that should fix this. If you didn't do so yet, please > > read appendix A of RFC 3542, which has a decent description of the > > API. > > Yeah, I'm afraid that API didn't make any sense to me, so I just made it > work in the only environment I had available. If you've got something > that works differently and can test it, please send patches along.
The testing bit is a bit of a problem as there is no DRI3 support on OpenBSD, at least not yet. But I'll see if I can write a little bit of test code that uses this code. An I'll update the fixes to also cover your new readv implementation. > > I also believe the handling of MSG_TRUNC and MSG_CTRUNC isn't correct, > > and will result in leaked file descriptors. If I read the Linux > > kernel code correctly MSG_CTRUNC gets set when there is not enough > > room to store all filedescriptors. > > The upper levels of the code are careful to not send more descriptors > than the other end is willing to accept, so yeah, treating these cases > as errors (to the point of just closing the client connection) would be > sufficient. You'll still need to close the file descriptors that did get passed in the message. Unfortunately you didn't respond to my general concern about adding this file descriptor passing code: > > But even with that fixed, there are some serious security issues with > > the approach taken here. Anything that calls SocketRead() will > > blindly accept file descriptors. And if I understand things correctly > > that includes xserver. So a malicious client can easily run the > > server out of file descriptors by passing file descriptors along with > > requests for which the server doesn't expect any. I now see that you added code to xserver to cleanup unconsumed file descriptors after each request. However things like xfs don't contain such code and will be vulnerable to a DOS attack like a describe above. And of course people building an old xserver release using a new libxtrans will suffer from the same problem. So I think that unconditionally defining XTRANS_SEND_FDS is a mistake. I think it should be left to the component that embeds libxtrans to request file descriptor passing support. That is the only way libxtrans can be used safely, i.e. something like the diff below. Don't include file descriptor passing code by default Leave it up to the consumer to request this functionality by defining XTRANS_SEND_FDS. Signed-off-by: Mark Kettenis <[email protected]> --- Xtransint.h | 2 -- 1 file changed, 2 deletions(-) diff --git a/Xtransint.h b/Xtransint.h index dd886db..5f96c8a 100644 --- a/Xtransint.h +++ b/Xtransint.h @@ -72,8 +72,6 @@ from The Open Group. # define XTRANSDEBUG 1 #endif -#define XTRANS_SEND_FDS 1 - #ifdef WIN32 # define _WILLWINSOCK_ #endif -- 1.8.4.1 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
