[PATCH libX11] Always initialise thread support

[Daniel Stone]

Hi,

On 12 July 2013 22:40, Jamey Sharp <jamey at 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 <daniel at 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 at lists.x.org: X.Org development
>> Archives: http://lists.x.org/archives/xorg-devel
>> Info: http://lists.x.org/mailman/listinfo/xorg-devel