[PATCH libX11] _XDefaultError: set XlibDisplayIOError flag before calling exit

[Arthur Huillet]

Hi Jamey,

On Wed, 1 Feb 2017 09:12:47 -0800
Jamey Sharp <jamey at minilop.net> wrote:

> I don't love this either, but it's Xlib; there's little there to love. ;-)
> I don't see anything fundamentally wrong with this approach.

Thanks.

> I'm a little confused by the notes in the commit message about whether this
> is legal. I don't know why it should be considered legal to use a Display
> from a _fini or atexit handler… although this has been a source of bugs for
> C++ apps in the past, so clearly people do it. I guess "there exist apps in
> the wild that do this" is as good a definition of what's legal in Xlib as
> any. But if you have a more specific reference for why it's legal, it'd be
> nice to have that.

I am surprised that the notion that it is legal would require discussion. 
It's not forbidden anywhere (that I'm aware of), therefore, it's allowed. Doesn't that reasoning work?

Yes, it sounds completely useless...

> I also don't understand why OpenGL libraries specifically would need to
> clean up X resources at disconnect, aside from the retain-permanent case.
> Or did you mean that when the library is dlclosed you want to clean up when
> the connection is not being closed? Perhaps you could clarify that in the
> commit message.

... but _fini also applies to dlclose(). The driver has no practical way of knowing 
whether its _fini is being called because of exit() (in which case it could decide not to use the dpy connection),
or because of dlclose() (in which case it absolutely does need to talk to the X server to avoid resource leaks).

As for clarification of the commit message, I thought that the paragraph mentioning dlopen/dlclose was covering the problem.
What do you feel needs clarifying?

Thanks,
-- 
Arthur

> 
> Thanks,
> Jamey
> 
> On Feb 1, 2017 6:02 AM, <arthur.huillet at free.fr> wrote:
> 
> From: Arthur Huillet <ahuillet at nvidia.com>
> 
> _XReply isn't reentrant, and it can lead to deadlocks when the default
> error handler is called: _XDefaultError calls exit(1). It is called
> indirectly by _XReply when a X protocol error comes in that isn't
> filtered/handled by an extension or the application. This means that if the
> application (or one of its loaded shared libraries such as the NVIDIA
> OpenGL driver) has registered any _fini destructor, _fini will get called
> while still on the call stack of _XReply. If the destructor interacts with
> the X server and calls _XReply, it will hit a deadlock, looping on the
> following in _XReply:
>     ConditionWait(dpy, dpy->xcb->reply_notify);
> 
> It is legal for an application to make Xlib calls during _fini, and that is
> useful for an OpenGL driver to avoid resource leaks on the X server side,
> for example in the dlopen/dlclose case. However, the driver can not readily
> tell whether its _fini is being called because Xlib called exit, or for
> another reason (dlclose), so it is hard to cleanly work around this issue
> in the driver.
> 
> This change makes it so _XReply effectively becomes a no-op when called
> after _XDefaultError was called, as though an XIOError had happened. The
> dpy connection isn't broken at that point, but any call to _XReply is going
> to hang. This is a bit of a kludge, because the more correct solution would
> be to make _XReply reentrant, maybe by broadcasting the reply_notify
> condition before calling the default error handler. However, such a change
> would carry a grater risk of introducing regressions in Xlib.
> 
> This change will drop some valid requests on the floor, but this should not
> matter, as it will only do so in the case where the application is dying: X
> will clean up after it once exit() is done running. There is the case of
> XSetCloseDownMode(RETAIN_PERMANENT), but an application using that and
> wishing to clean up resources in _fini would currently be hitting a
> deadlock, which is hardly a better situation.
> ---
> 
> I'm not in love with this change, to be honest, and if anyone has a better
> suggestion I am eager to hear it. It is the best change I could think of,
> in terms of making the problem go away without risking regressions.
> 
> Thanks
> --
> Arthur Huillet
> 
>  src/XlibInt.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/src/XlibInt.c b/src/XlibInt.c
> index 9e67dff..1cb386d 100644
> --- a/src/XlibInt.c
> +++ b/src/XlibInt.c
> @@ -1382,6 +1382,16 @@ int _XDefaultError(
>         XErrorEvent *event)
>  {
>      if (_XPrintDefaultError (dpy, event, stderr) == 0) return 0;
> +
> +    /*
> +     * Store in dpy flags that the client is exiting on an unhandled XError
> +     * (pretend it is an IOError, since the application is dying anyway it
> +     * does not make a difference).
> +     * This is useful for _XReply not to hang if the application makes Xlib
> +     * calls in _fini as part of process termination.
> +     */
> +    dpy->flags |= XlibDisplayIOError;
> +
>      exit(1);
>      /*NOTREACHED*/
>  }
> --
> 2.10.2


-- 
A. Huillet