An XCB test application will always crash because of heap corruption if it's running xcb_connect/xcb_disconnect continuously from multiple threads. The problem can also happen in real applications if XOpenDisplay and xcb_connect are called simultaneously.
This commit fixes only the heap corruption and sporadic crashes. It's still possible that XauFileName returns a badly formed filename string if called from multiple threads. For example, changing contents of HOME environment variable could make the returned string to be malformed. However, there shouldn't be crashes. Signed-off-by: Rami Ylimäki <[email protected]> Signed-off-by: Erkki Seppälä <[email protected]> --- Minor fixes have been made based on review comments. v2 -- Mark Kettenis: - Don't initialize static array explicitly to zero. This change is OK to us. - Use strcat as it was used before for clarity. Unfortunately this is not possible, because strcat assumes that there is a null character in the middle of the string. We want to avoid at all cost of splitting the result string temporarily, so that multithreaded users wouldn't suffer. We did replace the memcpy of v1 with strncpy though. Arnaud Fontaine: - Don't use PATH_MAX as it's not always defined. We have chosen to use it and define it as 1024 if it doesn't come from a header. This is in line with similar Xorg code that is using PATH_MAX. The reason for this is that we need to allocate space for the returned filename string at compile time and some constant is needed. It could be possible to allocate the required filename string dynamically. However, doing this would require linking against pthreads to prevent allocation race conditions and possibly using GCC constructors/destructors to avoid memory leaks if libXau is loaded dynamically. It's much simpler to just use static storage. Also enabling pthreads wouldn't improve things much, as the getenv() of XauFileName can't really be protected against putenv() happening elsewhere, the function would still remain unsafe if environment would be changed. AuFileName.c | 47 ++++++++++++++++++++++++++++++++++------------- 1 files changed, 34 insertions(+), 13 deletions(-) diff --git a/AuFileName.c b/AuFileName.c index b21b048..9d28011 100644 --- a/AuFileName.c +++ b/AuFileName.c @@ -33,14 +33,27 @@ in this Software without prior written authorization from The Open Group. #include <X11/Xauth.h> #include <X11/Xos.h> #include <stdlib.h> +#include <limits.h> /* PATH_MAX */ + +/* PATH_MAX is not necessarily defined. However, the specification of + * XauFileName requires us to use statically allocated space for the + * filename string. Therefore we need some kind of estimate for a + * maximum path length. + */ +#ifndef PATH_MAX +#ifdef MAXPATHLEN +#define PATH_MAX MAXPATHLEN +#else /* MAXPATHLEN */ +#define PATH_MAX 1024 +#endif /* MAXPATHLEN */ +#endif /* PATH_MAX */ char * XauFileName (void) { const char *slashDotXauthority = "/.Xauthority"; char *name; - static char *buf; - static int bsize; + static char buf[PATH_MAX]; #ifdef WIN32 char dir[128]; #endif @@ -48,6 +61,9 @@ XauFileName (void) if ((name = getenv ("XAUTHORITY"))) return name; + /* This function assumes that the HOME environment variable + * doesn't change between multiple threads. If it does change, the + * returned string may not contain a valid file name. */ name = getenv ("HOME"); if (!name) { #ifdef WIN32 @@ -60,16 +76,21 @@ XauFileName (void) #endif return NULL; } - size = strlen (name) + strlen(&slashDotXauthority[1]) + 2; - if (size > bsize) { - if (buf) - free (buf); - buf = malloc ((unsigned) size); - if (!buf) - return NULL; - bsize = size; - } - strcpy (buf, name); - strcat (buf, slashDotXauthority + (name[1] == '\0' ? 1 : 0)); + /* Length of home directory location. */ + size = strlen (name); + /* Check whether "/home/user" + "/.Xauthority" + "\0" fits into + * "buf". */ + if (size + strlen (slashDotXauthority) + 1 > sizeof(buf)) + return NULL; + /* Construct "/home/user/.Xauthority". Avoid writing null + * character when the first part of the string is copied. That + * could temporarily split the contents of "buf" and cause a + * problem with multiple threads. We are assuming here that the + * contents of "name" stay the same if this function is called + * simultaneously from multiple threads. + */ + strncpy (buf, name, size); + strcpy (buf + size, slashDotXauthority + ((size <= 1) ? 1 : 0)); + return buf; } -- 1.6.3.3 _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
