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

[Jamey Sharp]

I sympathize with Mark's complaints that this is a hack and solving
the whole problem would be better. But I think it's worth making
existing callers safer, and I don't think the environment variable
issues are seriously important.

Reviewed-by: Jamey Sharp <jamey at minilop.net>

On the other hand, I disagree with Mark on two points: strncpy is not
more clear than memcpy when you know precisely how many bytes you want
to copy; and the ideal interface would not ask the caller to allocate
a buffer of unknown size.

Actually, this whole notion of returning a filename out of libXau
seems crazy, though apparently the xauth app uses the filename
heavily, and the X server needs to be able to pass a filename in (but
never calls XauFileName to get a default). In fact as far as I can
tell, only Xlib or XCB based clients could hit this code from multiple
threads (I don't see any other users of libXau that can), and neither
XCB nor the pre-XCB versions of Xlib call XauFileName directly. So I
suppose some other solution could also work here.

Jamey

On 3/28/11, Rami Ylimäki <rami.ylimaki at vincit.fi> wrote:
> 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
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel