[PATCH v2 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>
---

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