Hi, On 12 July 2013 22:40, Jamey Sharp <ja...@minilop.net> wrote: > Hmm. XInitThreads wasn't designed to be used this way. For instance, > initializing thread support isn't thread-safe, for fairly obvious > reasons. > > This patch might mask more bugs than it causes, and thereby be a net > win. But it seems equally likely to turn out the other way. > > I'd suggest an awful lot of caution here.
Hmm. So the failure mode here would be n threads both calling XOpenDisplay simultaneously, all using the Displays independently (i.e. never mixing threads) and not having called XInitThreads. If XInitThreads gets entered simultaneously between the test and assignment of _Xglobal_lock, then we'll leak n - 1 copies of all the mutexes. The real disaster is if pthread_mutex_init isn't thread-safe (non-atomic pointer stores?), in which case the mutex pointers are going to be half of one and half of the other. So there's a theoretical API break for that case, but that can also be fixed by calling XInitThreads beforehand, which I think is acceptable. The immediate provocation was the Mali GLES/EGL implementation, which uses multiple threads itself, and thus relies on XInitThreads having been called somewhere; so if you ever use that specific implementation, every app has to call XInitThreads first to ensure it doesn't die horribly. Cheers, Daniel > Jamey > > On Fri, Jul 12, 2013 at 10:25:38PM +0100, Daniel Stone wrote: >> Make XOpenDisplay always call XInitThreads when opening a display, thus >> guarding it against possible disaster scenarios like calling >> XOpenDisplay without XInitThreads, then passing the Display pointer into >> an EGL library which uses threads. Or any of the other five similar >> failure scenarios I've seen just this week. >> >> Signed-off-by: Daniel Stone <dan...@fooishbar.org> >> --- >> src/OpenDis.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/OpenDis.c b/src/OpenDis.c >> index fc67d1a..2104845 100644 >> --- a/src/OpenDis.c >> +++ b/src/OpenDis.c >> @@ -87,6 +87,8 @@ XOpenDisplay ( >> long int conn_buf_size; >> char *xlib_buffer_size; >> >> + XInitThreads(); >> + >> /* >> * If the display specifier string supplied as an argument to this >> * routine is NULL or a pointer to NULL, read the DISPLAY variable. >> -- >> 1.8.3.1 >> >> _______________________________________________ >> xorg-devel@lists.x.org: X.Org development >> Archives: http://lists.x.org/archives/xorg-devel >> Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
