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

[Mark Kettenis]

> Date: Tue, 29 Mar 2011 12:06:34 +0300
> From: =?ISO-8859-1?Q?Rami_Ylim=E4ki?= <rami.ylimaki at vincit.fi>
> 
> On 03/28/2011 09:14 PM, Mark Kettenis wrote:
> >> Date: Mon, 28 Mar 2011 10:21:06 -0700
> >> From: Alan Coopersmith<alan.coopersmith at oracle.com>
> >>
> >>> 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.
> >> Perhaps we just need to define a new function to do this, that doesn't
> >> use a static global variable of any sort, and declare that multi-threaded
> >> code needs to use it.
> > That does make more sense than adding hacks to limit the potential
> > damage.  The current interface simply is inherently thread-unsafe.
> 
> I was actually thinking about implementing XauGetFileName as an 
> alternative for XauFileName, that would return dynamically allocated 
> filename string and require it to be freed by callers.

It's probably better to have an interface where the caller provides
the storage.

> However, the real problem is that getenv has to be called at some
> point to get XAUTHORITY or HOME, and that can't be done safely
> inside a library in any portable way.

NetBSD has getenv_r(3), whch solves this issue completely.  The
Solaris man page suggests that it partially addresses the issue by
preventing concurrent access to to list of environment variables.  So
presumable there is only an issue if you use putenv(3) to modify $HOME
or $XAUTHORITY.

Note that getenv(3) only has thread-safety issues if you mix it with
putenv(3) or setenv(3).  And programs generally don't modify their own
environment.  One can even argue that putenv(3) and setenv(3) should
not be used in any multi-threaded code because of their thread-safety
issues.  Your libc probably already does several getenv(3) calls under
the hood.

> The only way to solve the problem in a thread safe manner would be to 
> pass copies of XAUTHORITY and HOME to Xlib/Xcb when creating a new 
> connection.

Not sure what you mean here, but this sounds like just shifting the problem.

> Alternatively the semantics of connection creation should be 
> changed so that environment variables would be completely ignored. Both 
> of these options aren't very good from backward compatibility point of view.

Defenitely not an option.

> We chose to fix the common case and make connection creation safe as 
> long as environment is not modified at the same time. We didn't 
> implement a new function, because if we offer a new function for 
> multithreaded clients, it should be also protected against environment 
> changes. That would again require a lot of changes in users of libXau. 
> In other words, the fix avoids a real problem and while not perfect, 
> it's good enough. 
> [http://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good :)]

Sorry, but it remains a hack.

> Could you give your opinion about the environment problem? If a new 
> function is created for multithreaded clients, what should be done about 
> reading XAUTHORITY and HOME in a safe manner? Alternatively, do you 
> think that protecting against environment changes is not that important 
> and fixing the allocation problems are enough?

I think the memory allocation issue on its own is enough to change the
interface.