Currently, when the X server crashes or a client is disconnected with
XKillClient, you get a somewhat confusing error message from libX11
along the lines of:
XIO: fatal IO error 11 (Resource temporarily unavailable) on X server ":0"
after 98 requests (40 known processed) with 0 events remaining.
What's happening here is the previous recvmsg has thrown EAGAIN, since
the socket is non-blocking. In this case, check whether the socket has
any more data to read, and if not treat it like EPIPE.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Fixes: warning: variable 'req' set but not used [-Wunused-but-set-variable]
by marking req _X_UNUSED
Solution was discussed on xorg-devel ML
Peter Hutter, Alan Coopersmith
Re: [PATCH libX11 3/5] fix: warning: pointer targets in passing argument 2 of '_XSend' differ in signedness [-Wpointer-sign]
Signed-off-by: harms wharms@bfs.de
_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.
Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Reviewed-by: Jamey Sharp <jamey@minilop.net>
Make use of the new 64-bit sequence number API in XCB 1.11.1 to avoid
the 32-bit sequence number wrap in libX11.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=71338
Signed-off-by: Christian Linhart <chris@demorecorder.com>
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This patch removes some redundant null checks before free.
It should not change the code otherwise. Be aware that this
is only the first series.
Signed-off-by: Harms <wharms@bfs,de>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Left behind when 15e5eaf628 removed support for building without XCB.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Jasper St. Pierre <jstpierre@mecheye.net>
Two users of GetReqExtra pass arbitrarily sized allocations from the
caller (ModMap and Host). Adjust _XGetRequest() (called by the GetReqExtra
macro) to double-check the requested length and invalidate "req" when
this happens. Users of GetReqExtra passing lengths greater than the Xlib
buffer size (normally 16K) must check "req" and fail gracefully instead
of crashing.
Any callers of GetReqExtra that do not check "req" for NULL
will experience this change, in the pathological case, as a NULL
dereference instead of a buffer overflow. This is an improvement, but
the documentation for GetReqExtra has been updated to reflect the need
to check the value of "req" after the call.
Bug that manifested the problem:
https://bugs.launchpad.net/ubuntu/+source/x11-xserver-utils/+bug/792628
Signed-off-by: Kees Cook <kees@outflux.net>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
WORD64 seems to have only been defined in <X11/Xmd.h> when building for
CRAY, to handle int being a 64-bit value (ILP64, not LP64) and having
64-bit alignment requirements.
It hadn't been fully supported even before autotooling, as can be
seen by removed code such as:
#ifdef WORD64
_XkbWriteCopyData32 Not Implemented Yet for sizeof(int)==8
#endif
(mostly performed with unifdef, followed by some manual cleanup of
the remaining code)
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
You could analyze most of these and quickly recognize that there was no
chance of buffer overflow already, but why make everyone spend time doing
that when we can just make it obviously safe?
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Casts were annoying gcc by dropping constness when changing types,
when routines simply either copy data into the request buffer or
send it directly to the X server, and never modify the input.
Fixes gcc warnings including:
ChProp.c: In function 'XChangeProperty':
ChProp.c:65:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
ChProp.c:65:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
ChProp.c:74:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
ChProp.c:74:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
ChProp.c:83:6: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
SetHints.c: In function 'XSetStandardProperties':
SetHints.c:262:20: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
SetPntMap.c: In function 'XSetPointerMapping':
SetPntMap.c:46:5: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
SetPntMap.c:46:5: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
StBytes.c: In function 'XStoreBuffer':
StBytes.c:97:33: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
StName.c: In function 'XStoreName':
StName.c:40:27: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
StName.c: In function 'XSetIconName':
StName.c:51:27: warning: cast discards '__attribute__((const))' qualifier from pointer target type [-Wcast-qual]
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Historically, Xlib dropped the Display lock around the upcall to any
user-supplied _XErrorFunction, but somewhere along the way I quit doing
that if you built with XCB. The reasons are lost somewhere in the
pre-git history of Xlib/XCB, and I can't now see any reason to hold the
lock.
The documentation for XSetErrorHandler still applies though:
Because this condition is not assumed to be fatal, it is acceptable
for your error handler to return; the returned value is ignored.
However, the error handler should not call any functions (directly
or indirectly) on the display that will generate protocol requests
or that will look for input events.
So while you are now once again permitted to re-enter Xlib from the
error handler, you're only allowed to call non-protocol functions.
Signed-off-by: Jamey Sharp <jamey@minilop.net>
Possible overrun of 8192 byte fixed size buffer "buffer" by copying
"ext->name" without length checking
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan-de-oliveira@nokia.com>
Signed-off-by: Erkki Seppälä <erkki.seppala@vincit.fi>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
info_list->watch_data was being reallocated, but the return value of
the reallocation was stored only into a local variable. This might
cause some funky behavior and crashes.
Variable "wd_array" goes out of scope
Value "wd_array" is overwritten in "wd_array = (XPointer*)realloc((char*)info_list->watch_data, (((dpy->watcher_count + 1) * 4U == 0U) ? 1U : ((dpy->watcher_count + 1) * 4U)))"
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Erkki Seppälä <erkki.seppala@vincit.fi>
Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan-de-oliveira@nokia.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reordered code to first to do the comparison and then to release data
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Ander Conselvan de Oliveira <ander.conselvan-de-oliveira@nokia.com>
Signed-off-by: Erkki Seppälä <erkki.seppala@vincit.fi>
If we receive unsupported event closing connection triggers valgrind
error.
==12017== Conditional jump or move depends on uninitialised value(s)
==12017== at 0x487D454: _XFreeDisplayStructure (OpenDis.c:607)
==12017== by 0x486857B: XCloseDisplay (ClDisplay.c:72)
*snip*
==12017== Uninitialised value was created by a heap allocation
==12017== at 0x4834C48: malloc (vg_replace_malloc.c:236)
==12017== by 0x4894147: _XEnq (XlibInt.c:877)
==12017== by 0x4891BF3: handle_response (xcb_io.c:335)
==12017== by 0x4892263: _XReply (xcb_io.c:626)
*snip*
Problem is that XFreeDisplaySturture is checking for qelt->event.type ==
GenericEvent while _XUnknownWireEvent doesn't store the type.
Reviewed-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Pauli Nieminen <ext-pauli.nieminen@nokia.com>
And there was much rejoicing.
Signed-off-by: Josh Triplett <josh@joshtriplett.org>
Reviewed-by: Jamey Sharp <jamey@minilop.net>
Consensus on #xorg-devel agrees with removing --without-xcb; in
particular, acks from Adam Jackson, Daniel Stone, Kristian Høgsberg,
Julien Cristau, and Rémi Cardona.
imLcIm.c: In function '_XimCachedFileName':
imLcIm.c:361: warning: format '%03x' expects type 'unsigned int', but argument 8 has type 'long unsigned int'
imLcIm.c:364: warning: format '%03x' expects type 'unsigned int', but argument 8 has type 'long unsigned int'
imRm.c: In function '_XimDefaultArea':
imRm.c:597: warning: cast from pointer to integer of different size
imRm.c: In function '_XimDefaultColormap':
imRm.c:626: warning: cast from pointer to integer of different size
lcFile.c:224: warning: no previous prototype for 'xlocaledir'
lcUTF8.c: In function 'iconv_cstombs':
lcUTF8.c:1841: warning: assignment discards qualifiers from pointer target type
lcUTF8.c:1869: warning: pointer targets in passing argument 1 of 'wctomb' differ in signedness
lcUTF8.c:1873: warning: pointer targets in passing argument 1 of 'wctomb' differ in signedness
lcUTF8.c: In function 'iconv_mbstocs':
lcUTF8.c:1935: warning: pointer targets in passing argument 2 of 'mbtowc' differ in signedness
lcUTF8.c: In function 'iconv_mbtocs':
lcUTF8.c:2031: warning: pointer targets in passing argument 2 of 'mbtowc' differ in signedness
lcUTF8.c: In function 'iconv_mbstostr':
lcUTF8.c:2121: warning: pointer targets in passing argument 2 of 'mbtowc' differ in signedness
lcUTF8.c: In function 'iconv_strtombs':
lcUTF8.c:2180: warning: pointer targets in passing argument 1 of 'wctomb' differ in signedness
lcUTF8.c: In function '_XlcAddGB18030LocaleConverters':
lcUTF8.c:2367: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
lcUTF8.c:2368: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
lcUTF8.c:2373: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
lcUTF8.c:2374: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
lcUTF8.c:2375: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
lcUTF8.c:2376: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
lcUTF8.c:2377: warning: passing argument 5 of '_XlcSetConverter' from incompatible pointer type
XlibInt.c: In function '_XGetHostname':
XlibInt.c:3441: warning: implicit declaration of function 'gethostname'
XlibInt.c:3441: warning: nested extern declaration of 'gethostname'
ConnDis.c: In function '_XDisconnectDisplay':
ConnDis.c:540: warning: old-style function definition
ConnDis.c: In function '_XSendClientPrefix':
ConnDis.c:554: warning: old-style function definition
ConnDis.c: In function 'XSetAuthorization':
ConnDis.c:677: warning: old-style function definition
Signed-off-by: Jeremy Huddleston <jeremyhu@apple.com>
It's easier to reason about the code when we can't re-enter the
Xlib-private sync-handlers while they're already running.
Signed-off-by: Jamey Sharp <jamey@minilop.net>
Reviewed-by: Josh Triplett <josh@freedesktop.org>
_XIDHandler and _XSeqSyncFunction originally ran from dpy->synchandler, and
thus had to return an int. Now, they only run from _XPrivSyncHandler or
LockDisplay, neither of which needs to check their return value since they
always returned 0. Make them both void.
Signed-off-by: Josh Triplett <josh@freedesktop.org>
Signed-off-by: Jamey Sharp <jamey@minilop.net>
XID and sync handling happened via _XPrivSyncHandler, assigned to
dpy->synchandler and called from SyncHandle. _XPrivSyncHandler thus ran
without the Display lock, so manipulating the Display caused races, and
these races led to assertions in multithreaded code (demonstrated via
ico).
In the XTHREADS case, after you've called XInitThreads, we can hook
LockDisplay and UnlockDisplay. Use that to run _XIDHandler and
_XSeqSyncHandler from LockDisplay rather than SyncHandle; we then know
that we hold the lock, and thus we can avoid races. We think it makes
sense to do these both from LockDisplay rather than UnlockDisplay, so
that you know you have valid sync and a valid XID before you start
setting up the request you locked to prepare.
In the !XTHREADS case, or if you haven't called XInitThreads, you don't
get to use Xlib from multiple threads, so we can use the logic we have
now (with synchandler and savedsynchandler) without any concern about
races.
This approach gets a bit exciting when the XID and sequence sync
handlers drop and re-acquire the Display lock. Reacquisition will re-run
the handlers, but they return immediately unless they have work to do,
so they can't recurse more than once. In the worst case, if both of
them have work to do, we can nest the Display lock three deep. In the
case of the _XIDHandler, we drop the lock to call xcb_generate_id, which
takes the socket back if it needs to request more XIDs, and taking the
socket back will reacquire the lock; we take care to avoid letting
_XIDHandler run again and re-enter XCB from the return_socket callback
(which causes Very Bad Things, and is Not Allowed).
Tested with ico (with 1 and 20 threads), and with several test programs
for XID and sequence sync. Tested with and without XInitThreads(), and
with and without XCB.
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=23192
Signed-off-by: Jamey Sharp <jamey@minilop.net>
Signed-off-by: Josh Triplett <josh@freedesktop.org>
Generic events require more bytes than Xlib provides in the standard XEvent.
Memory allocated by the extension and stored as pointers inside the event is
prone to leak by simple 'while (1) { XNextEvent(...); }' loops.
This patch adds cookie handling for generic events. Extensions may register
a cookie handler in addition to the normal event vectors. If an extension
has registered a cookie handler, _all_ generic events for this extensions
must be handled through cookies. Otherwise, the default event handler is
used.
The cookie handler must return an XGenericEventCookie with a pointer to the
data.The rest of the event (type, serialNumber, etc.) are to be filled as
normal. When a client retrieves such a cookie event, the data is stored in
an internal queue (the 'cookiejar'). This data is freed on the next call to
XNextEvent().
New extension interfaces:
XESetWireToEventCookie(display, extension_number, cookie_handler)
Where cookie_handler must set cookie->data. The data pointer is of arbitray
size and type but must be a single memory block. This memory block
represents the actual extension's event.
New client interfaces:
XGetEventData(display, *cookie);
XFreeEventData(display, *cookie);
If the client needs the actual event data, it must call XGetEventData() with
the cookie. This returns the data pointer (and removes it from the cookie
jar) and the client is then responsible for freeing the event with
XFreeEventData(). It is safe to call either function with a non-cookie
event. Events unclaimed or not handled by the XGetEventData() are cleaned up
automatically.
Example client code:
XEvent event;
XGenericEventCookie *cookie = &ev;
XNextEvent(display, &event);
if (XGetEventData(display, cookie)) {
XIEvent *xievent = cookie->data;
...
} else if (cookie->type == GenericEvent) {
/* handle generic event */
} else {
/* handle extension/core event */
}
XFreeEventData(display, cookie);
Cookies are not multi-threading safe. Clients that use XGetEventData() must
lock between XNextEvent and XGetEventData to avoid other threads freeing
cookies.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
The code is wrong since the first git revision, so it seens that it has
not been compiled with WORD64 for quite some time, there is also another
interesting code in xkb/XKBRdBuf.c:
<hash>ifdef WORD64
_XkbWriteCopyData32 Not Implemented Yet for sizeof(int)==8
<hash>endif
and possibly there are other similar problems.
Only convert to use "ansi prototypes" the functions warned from
compilation with "./autogen.sh --prefix=/usr", on a Linux computer.
Also, only address "trivial" compiler warning fixes in this commit.
The new .gitignore is the output of a command like:
% find . -name .gitignore -exec cat {} \; | sort | uniq
and only the toplevel .gitignore file was kept.
Xlib has several independent tasks that need to be performed with the
display unlocked. It does this by replacing the existing sync handler with
one of a variety of internal sync handlers. However, if multiple internal
sync handlers need to run, then the last one registering wins and
previously registered internal sync handlers are never invoked. This
manifested as a bug with DRI applications on Xlib/XCB as that requires
both an XID handler after every XID allocation, and the periodic sequence
number handler. The XID handler would win, and the sequence number handler
would never be invoked.
Fix this by unifying the internal sync handler mechanism into a single
function that calls all of the known internal sync handlers. They all need
to deal with being called when not strictly necessary now.
Signed-off-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Jamey Sharp <jamey@minilop.net>
Signed-off-by: Josh Triplett <josh@freedesktop.org>
I refuse to take any responsibily for this code. It works, I guess.
But - all the flushing is done somewhere before that, so we might need to
flush here. Under some circumstances anyway. Don't ask me, I'm an optical
illusion.
Build with xcb as transport layer highly recommended.
If given many requests without replies, Xlib may not sync until it flushes
the output buffer. Thus, if Xlib can fit enough requests in the buffer to
pass by the number of requests it would normally sync after (65536 -
BUFSIZE/sizeof(xReq)), it will sync too late. The test case in bug 15023
demonstrated this by issuing a request with a reply (ListExtensions) at
just the right time to get confused with the GetInputFocus reply issued in
response to the sync 65,536 requests later; the test case used an async
handler to watch the replies, since otherwise it could not issue a request
without waiting for the response. When the test case failed, Xlib's sync
handler would eat the ListExtensions reply, and the test case's async
handler would see the GetInputFocus reply.
Fix this by replacing SEQLIMIT with a function sync_hazard() that uses the
buffer size to figure out when the sequence numbers could potentially wrap
before the next flush.
With this commit, the test case consistently passed, and the async reply
handler always saw the ListExtensions reply.
Commit by Jamey Sharp and Josh Triplett.
Some libraries try to clean up X resources from atexit handlers, _fini,
or C++ destructors. To make these work, the Display lock should be
downgraded to a user lock (as in XLockDisplay) before calling exit(3).
This blocks Xlib calls from threads other than the one calling exit(3)
while still allowing the exit handlers to call Xlib.
This assumes that the thread calling exit will call any atexit handlers.
If this does not hold, then an alternate solution would involve
registering an atexit handler to take over the lock, which would only
assume that the same thread calls all the atexit handlers.
Commit by Josh Triplett and Jamey Sharp.
