[PATCH libXau] Avoid heap corruption when calling XauFileName from multiple threads.

[Rami Ylimäki]

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 <rami.ylimaki at vincit.fi>
Signed-off-by: Erkki Seppälä <erkki.seppala at vincit.fi>
---
 AuFileName.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/AuFileName.c b/AuFileName.c
index b21b048..fdf03e0 100644
--- a/AuFileName.c
+++ b/AuFileName.c
@@ -33,14 +33,14 @@ 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>
 
 char *
 XauFileName (void)
 {
     const char *slashDotXauthority = "/.Xauthority";
     char    *name;
-    static char	*buf;
-    static int	bsize;
+    static char	buf[PATH_MAX] = {0};
 #ifdef WIN32
     char    dir[128];
 #endif
@@ -48,6 +48,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 +63,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.
+     */
+    memcpy (buf, name, size);
+    strcpy (buf + size, slashDotXauthority + ((size <= 1) ? 1 : 0));
+
     return buf;
 }
-- 
1.6.3.3