[libX11] Always initialise thread support

Submitted by Daniel Stone on July 12, 2013, 9:25 p.m.

Details

Message ID 1373664338-3697-1-git-send-email-daniel@fooishbar.org
State New
Headers show

Not browsing as part of any series.

Commit Message

Daniel Stone July 12, 2013, 9:25 p.m.
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@fooishbar.org>
---
 src/OpenDis.c | 2 ++
 1 file changed, 2 insertions(+)

Patch hide | download patch | download mbox

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.

Comments

Daniel Stone, le Fri 12 Jul 2013 22:25:38 +0100, a écrit :
> 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.

+1.  We have issues in accessibility toolkits just because they use
threads to be able to work and can't manage to get XInitThreads called
before the application initializes X11.

Samuel
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.

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@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
Hi,

On 12 July 2013 22:40, Jamey Sharp <jamey@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@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
On Fri, Jul 12, 2013 at 11:16:16PM +0100, Daniel Stone wrote:
> On 12 July 2013 22:40, Jamey Sharp <jamey@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.

I mostly agree with your reasoning, except the subtleties of memory
ordering semantics make this even more murky. Calling XInitThreads first
only helps if it's followed by a write barrier, and the later calls are
preceded by read barriers.

I think pthread_create should have the right barrier semantics in the
parent and child threads, so if you call XInitThreads before
pthread_create, it should be fine--but POSIX apparently doesn't specify
anything about that.

> 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.

Yeah, I won't argue that XInitThreads was ever a good idea... :-/ For
that matter, I'm not certain why --disable-xthreads is still allowed.

In fact, I don't really mean to argue against the patch. I'm just
proposing caution.

Jamey
On 07/12/13 03:16 PM, Daniel Stone wrote:
> 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.

I don't know that it's any saner, but the patches we carried in the Solaris
libX11 for years to allow libraries to be thread safe even when calling
apps are not are posted, along with description, at:

http://people.freedesktop.org/~alanc/thread-fixes/

Unfortunately, I never got around to trying to figure out if these
would be good to push upstream or not.
On Fri, Jul 12, 2013 at 05:09:00PM -0700, Alan Coopersmith wrote:
> On 07/12/13 03:16 PM, Daniel Stone wrote:
> >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.
> 
> I don't know that it's any saner, but the patches we carried in the Solaris
> libX11 for years to allow libraries to be thread safe even when calling
> apps are not are posted, along with description, at:
> 
> http://people.freedesktop.org/~alanc/thread-fixes/
> 
> Unfortunately, I never got around to trying to figure out if these
> would be good to push upstream or not.

I think the first one looks like a reasonable idea, though I'd have to
think very hard to decide if it's correct.

http://people.freedesktop.org/~alanc/thread-fixes/#1234757

But as the second patch demonstrates, the first one could be opening up
a wide variety of other issues.

http://people.freedesktop.org/~alanc/thread-fixes/#4010755

Patch three looks crazy at first glance, and is probably superceded by
the fix for #1182, as you noted.

http://people.freedesktop.org/~alanc/thread-fixes/#4041914

Patch four seems unrelated to the current discussion, but sounds like a
plausible patch based on the description.

http://people.freedesktop.org/~alanc/thread-fixes/#4614834

Jamey
Hello,

Couldn't pthread_once be used in order to make sure initialization
happens only once?

Samuel
> From: Daniel Stone <daniel@fooishbar.org>
> Date: Fri, 12 Jul 2013 22:25:38 +0100
> 
> 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.

Isn't the real solution to these problems to change the EGL library to
use xcb?

Forcing the overhead of per-request locking down everybodies throat
isn't very nice, even if the overhead is relatively small.

> Signed-off-by: Daniel Stone <daniel@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
>
Hi,

On 14 July 2013 17:17, Mark Kettenis <mark.kettenis@xs4all.nl> 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.
>
> Isn't the real solution to these problems to change the EGL library to
> use xcb?

As the last sentence alludes to, it's not just this one EGL stack, or
EGL in general.  It's pretty hard to construct a program more complex
than xterm whilst totally avoiding threads.

> Forcing the overhead of per-request locking down everybodies throat
> isn't very nice, even if the overhead is relatively small.

Beats the overhead of having to figure out what's wrong and add
XInitThreads() to random apps which don't use threads themselves, but
have threads forced on them by any number of support libraries, most
of which can't call XInitThreads() in time before someone else calls
XOpenDisplay().

An XUnInitThreads() for programs which could 100% guarantee no threads
will ever be used might be more interesting.  Or just have those
performance-critical programs use XCB, since Xlib sucks for
performance anyway.

Cheers,
Daniel

>> Signed-off-by: Daniel Stone <daniel@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
>>
Hello,

On 14 July 2013 18:31, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi,
>
> On 14 July 2013 17:17, Mark Kettenis <mark.kettenis@xs4all.nl> 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.
>>
>> Isn't the real solution to these problems to change the EGL library to
>> use xcb?
>
> As the last sentence alludes to, it's not just this one EGL stack, or
> EGL in general.  It's pretty hard to construct a program more complex
> than xterm whilst totally avoiding threads.
>
>> Forcing the overhead of per-request locking down everybodies throat
>> isn't very nice, even if the overhead is relatively small.
>
> Beats the overhead of having to figure out what's wrong and add
> XInitThreads() to random apps which don't use threads themselves, but
> have threads forced on them by any number of support libraries, most
> of which can't call XInitThreads() in time before someone else calls
> XOpenDisplay().
>
> An XUnInitThreads() for programs which could 100% guarantee no threads
> will ever be used might be more interesting.  Or just have those
> performance-critical programs use XCB, since Xlib sucks for
> performance anyway.
>
> Cheers,
> Daniel

What's happened to this patch?

Thanks

Michal

>
>>> Signed-off-by: Daniel Stone <daniel@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