Commit 1472048b7 to fix a colormap threading issue added a display
lock/unlock and a call to SyncHandle() to _XcmsFreeClientCmaps().
When running synchronized, that means calling XSync().
_XcmsFreeClientCmaps() is called from _XFreeDisplayStructure() via
XCloseDisplay() after the xcb connection is closed.
So when running synchronized, we may end up calling XSync() after the
xcb connection to the display is closed, which will generate a spurious
XIO error:
| #0 in _XDefaultIOError () at /lib64/libX11.so.6
| #1 in _XIOError () at /lib64/libX11.so.6
| #2 in _XReply () at /lib64/libX11.so.6
| #3 in XSync () at /lib64/libX11.so.6
| #4 in _XSyncFunction () at /lib64/libX11.so.6
| 8#5 in _XFreeDisplayStructure () at /lib64/libX11.so.6
| 8#6 in XCloseDisplay () at /lib64/libX11.so.6
To avoid that issue, closed the xcb connection to the display last.
v2: And same in OutOfMemory() as well (José Expósito)
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: José Expósito <jexposit@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/264>
Xlib is now built with threading support enabled from the constructor
by default.
XRebindKeysym() acquires the display lock, then calls:
| XRebindKeysym()
| LockDisplay()
| ComputeMaskFromKeytrans()
| -> XkbKeysymToModifiers()
| -> _XkbLoadDpy()
| -> XkbGetMap()
| -> XkbGetUpdatedMap()
| LockDisplay()
And the dead lock:
| Xlib ERROR: XKBGetMap.c line 575 thread 1fc6e580: locking display already
| locked at KeyBind.c line 937
To avoid the issue, call ComputeMaskFromKeytrans() from outside the display
lock.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Closes: https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/216
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/256>
That commit 99a2cf1aa was moving the calls to the _Xcms*CmapRec*()
family of functions within a display lock to make the XCMS colormap
functions thread safe.
Unfortunately, that causes a deadlock in XCopyColormapAndFree(), because
_XcmsCopyCmapRecAndFree() calls CmapRecForColormap() which calls
XGetVisualInfo() which also tries to acquire the display lock.
So, instead of moving the entire functions within the display lock,
let's try to make the functions themselves thread safe in the following
commit, and revert this change which causes a deadlock.
This reverts commit 99a2cf1aa0.
Fixes: https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/215
See-also: https://gitlab.freedesktop.org/xorg/lib/libx11/-/issues/94
Reviewed-by: Adam Jackson <ajax@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/254>
Passing a negative value in `needed` to the `XkbResizeKeyActions()`
function can create a `newActs` array of an unespected size.
Check the value and return if it is invalid.
This error has been found by a static analysis tool. This is the report:
Error: OVERRUN (CWE-119):
libX11-1.8.7/src/xkb/XKBMAlloc.c:811: cond_const:
Checking "xkb->server->size_acts == 0" implies that
"xkb->server->size_acts" is 0 on the true branch.
libX11-1.8.7/src/xkb/XKBMAlloc.c:811: buffer_alloc:
"calloc" allocates 8 bytes dictated by parameters
"(size_t)((xkb->server->size_acts == 0) ? 1 : xkb->server->size_acts)"
and "8UL".
libX11-1.8.7/src/xkb/XKBMAlloc.c:811: var_assign:
Assigning: "newActs" = "calloc((size_t)((xkb->server->size_acts == 0) ? 1 : xkb->server->size_acts), 8UL)".
libX11-1.8.7/src/xkb/XKBMAlloc.c:815: assignment:
Assigning: "nActs" = "1".
libX11-1.8.7/src/xkb/XKBMAlloc.c:829: cond_at_least:
Checking "nCopy > 0" implies that "nCopy" is at least 1 on the
true branch.
libX11-1.8.7/src/xkb/XKBMAlloc.c:830: overrun-buffer-arg:
Overrunning buffer pointed to by "&newActs[nActs]" of 8 bytes by
passing it to a function which accesses it at byte offset 15
using argument "nCopy * 8UL" (which evaluates to 8).
# 828|
# 829| if (nCopy > 0)
# 830|-> memcpy(&newActs[nActs], XkbKeyActionsPtr(xkb, i),
# 831| nCopy * sizeof(XkbAction));
# 832| if (nCopy < nKeyActs)
Signed-off-by: José Expósito <jexposit@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/250>
When input focuses are switched quickly with shortcut keys in a Java
window, the focus is sometimes lost and the Window=0 is assigned in
XFilterEvent() but the XKeyEvent was forwarded by a XIM serer(IBus)
with XIM_FORWARD_EVENT -> XNextEvent() -> XFilterEvent() and the event
needs to be forwarded to the XIM XKeyEvent press and release filters
to update the XIM state with Window=0 likes _XimPendingFilter() and
_XimUnfabricateSerial().
Closes: #205, #206
Fixes: 024d229f ("ximcp: Unmark to fabricate key events with XKeyEvent serial")
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/246>
When users type keys quickly, some applications using Steam or Java
do not call XNextEvent() for a key event but _XimFilterKeypress()
and _XimFilterKeyrelease() expect to receive the key events
forwarded by input methods.
Now fabricated_time Time value is added to XimProtoPrivate to check
the timeout value.
Closes: #205
Fixes: 024d229f ("ximcp: Unmark to fabricate key events with XKeyEvent serial")
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/246>
_XimProtoKeypressFilter() and _XimProtoKeyreleaseFilter() can
receive XKeyEvent from both the typing on the keyboard and the
callback of XIM_FORWARD_EVENT.
If the filter functions unmark to fabricate XKeyEvent from the typing
on the keyboard during receiving XKeyEvent from the callback of
XIM_FORWARD_EVENT with typing keys very quickly likes an bar code
scanner (or evemu-play), XIM server cannot receive some key events and
it causes the key typing order to get scrambled.
Now XIM client saves the serial in XKeyEvent and the filter functions
unmark to fabricate XKeyEvent from the callback of XIM_FORWARD_EVENT
only.
This and 024d229f are same patches but the regression issues will be
fixed by the later patches.
Closes: #198
Fixes: 024d229f ("ximcp: Unmark to fabricate key events with XKeyEvent serial")
Part-of: <https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/246>
I can't find any evidence this was ever defined, should only have
been needed for odd-ball pre-C89 compilers.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
_XimProtoKeypressFilter() and _XimProtoKeyreleaseFilter() can
receive XKeyEvent from both the typing on the keyboard and the
callback of XIM_FORWARD_EVENT.
If the filter functions unmark to fabricate XKeyEvent from the typing
on the keyboard during receiving XKeyEvent from the callback of
XIM_FORWARD_EVENT with typing keys very quickly likes an bar code
scanner (or evemu-play), XIM server cannot receive some key events and
it causes the key typing order to get scrambled.
Now XIM client saves the serial in XKeyEvent and the filter functions
unmark to fabricate XKeyEvent from the callback of XIM_FORWARD_EVENT
only.
Fixes: #198
XkbGetDeviceInfo(dpy, XkbXI_ButtonActionsMask, 2, 0, 0) always returns
NULL because the number of buttons on the device equals (unsurpisingly)
the number of buttons requested (i.e. first + nBtns == dev->nbuttons).
This currently causes it to bail out and return NULL.
Fixes f293659d5a
When the format is `Pixmap` it calculates the size of the image data as:
ROUNDUP((bits_per_pixel * width), image->bitmap_pad);
There is no validation on the `width` of the image, and so this
calculation exceeds the capacity of a 4-byte integer, causing an overflow.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
The CreatePixmap request specifies height & width of the image as CARD16
(unsigned 16-bit integer), so if either is larger than that, set it to 0
so the X server returns a BadValue error as the protocol requires.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
The PutImage request specifies height & width of the image as CARD16
(unsigned 16-bit integer), same as the maximum dimensions of an X11
Drawable, which the image is being copied to.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
When splitting a single line of pixels into chunks to send to the
X server, be sure to take into account the number of bits per pixel,
so we don't just loop forever trying to send more pixels than fit in
the given request size and not breaking them down into a small enough
chunk to fix.
Fixes: "almost complete rewrite" (Dec. 12, 1987) from X11R2
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Make sure we allocate enough memory in the first place, and
also handle error returns from _XkbReadBufferCopyKeySyms() when
it detects out-of-bounds issues.
Reported-by: Gregory James DUCK <gjduck@gmail.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Other code assumes this pointer cannot be NULL, so fail the connection
if a bug has caused the X server to give a non-existent visual ID for
the default visual of any screen.
Reported-by: Gregory James DUCK <gjduck@gmail.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Fixes CVE-2023-3138: X servers could return values from XQueryExtension
that would cause Xlib to write entries out-of-bounds of the arrays to
store them, though this would only overwrite other parts of the Display
struct, not outside the bounds allocated for that structure.
Reported-by: Gregory James DUCK <gjduck@gmail.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
This reverts commit d6d6cba902.
The reverted commit intended to fix the problem where an unpadded X
event struct is passed into XPutBackEvent, by creating a padded struct
with _XEventToWire and _XWireToEvent. However, _XWireToEvent updates the
last sequence number in Display, which may cause xlib to complain about
lost sequence numbers.
IMO, the problem that commit tried to solve is a bug in the client
library, and workaround it inside Xlib is bad practice, especially given
the problem it caused. Plus, the offender cited in the original commit
message, freeglut, has already fixed this problem.
Fixes: #176#174
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
It seems to be common practice of some X11 clients to pass specific event
types into APIs that take XEvent*. For example, freeglut does:
XConfigureEvent fakeEvent = {0};
...
XPutBackEvent(fgDisplay.Display, (XEvent*)&fakeEvent);
This can result in reads overflowing the input event when libX11 does:
XEvent store = *event;
=================================================================
==75304==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x00016ee4a8e8 at pc 0x000101c54d14 bp 0x00016ee4a0d0 sp 0x00016ee49888
READ of size 192 at 0x00016ee4a8e8 thread T0
#0 0x101c54d10 in __asan_memcpy+0x1a4 (libclang_rt.asan_osx_dynamic.dylib:arm64e+0x3cd10)
#1 0x102848a18 in _XPutBackEvent PutBEvent.c:41
#2 0x1028490a4 in XPutBackEvent PutBEvent.c:84
#3 0x1013295c8 in fgOpenWindow freeglut_window.c:1178
#4 0x101321984 in fgCreateWindow freeglut_structure.c:108
#5 0x10132b138 in glutCreateWindow freeglut_window.c:1551
#6 0x100fb7d94 in main+0x78 (checkeredTriangles:arm64+0x100003d94)
#7 0x197de3e4c (<unknown module>)
Address 0x00016ee4a8e8 is located in stack of thread T0 at offset 840 in frame
#0 0x1013282f8 in fgOpenWindow freeglut_window.c:1063
This frame has 8 object(s):
[32, 40) 'title.addr'
[64, 176) 'winAttr' (line 1066)
[208, 240) 'textProperty' (line 1067)
[272, 352) 'sizeHints' (line 1068)
[384, 440) 'wmHints' (line 1069)
[480, 672) 'eventReturnBuffer' (line 1070)
[736, 740) 'num_FBConfigs' (line 1072)
[752, 840) 'fakeEvent' (line 1074) <== Memory access at offset 840 overflows this variable
This change allows XPutBackEvent() to support such clients without
risk of memory read overflow.
Reviewed-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Tested-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>
Use the same indentation as the surrounding code.
Signed-off-by: Ulrich Sibiller <uli42@gmx.de>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
- the activation logic is reversed
- there is also _XInternalLockDisplay() that needs protection
- I've found cases (in fvwm2) where the callback calls XCheckIfEvent()
recursively. So the flag needs to be a counter.
Reviewed-by: Adam Jackson <ajax@redhat.com>
The documentation for this family of functions very clearly says not to
call into xlib in your predicate function, but historically single
threaded apps could get away with it just fine, and now that we've
forced thread-safety on the world such apps will now deadlock instead.
That's not an acceptable regression even if the app is technically
broken. This has been reported with XFCE and FVWM, and Motif's
cut-and-paste code has the same bug by inspection, so this does need to
be addressed.
This change nerfs LockDisplay/UnlockDisplay while inside the critical
bit of an IfEvent function. This is still safe in the sense that the
display remains locked and no other thread should be able to change it
from under us, but the loop that scans the event queue might not be
robust against it being modified as a side effect of protocol emitted by
the predicate callback. But that's not new, non-XInitThreads'd xlib
would have the same caveat.
Closes: xorg/lib/libx11#157
Avoid conflicts when libX11 calls libxcb and gets its pthread functions
overriding our ancient stubs.
v2: Keep linking with real threads libraries when thread safety
constructor is enabled.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
