Trying to change the pointer control settings on a device without
PtrFeedbackPtr would be a bug and a crash in the Xserver.
Guard against that case by returning early with a BadImplementation
error, that might kill the X11 client but the Xserver would survive.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Related: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1137
(imported from commit dee2bb033e)
This add a new flag POINTER_RAWONLY for GetPointerEvents() which does
pretty much the opposite of POINTER_NORAW.
Basically, this tells GetPointerEvents() that we only want the
DeviceChanged events and any raw events for this motion but no actual
motion events.
This is preliminary work for Xwayland to be able to use relative motion
events for raw events. Xwayland would use absolute events for raw
events, but some X11 clients (wrongly) assume raw events to be always
relative.
To allow such clients to work with Xwayland, it needs to switch to
relative raw events (if those are available from the Wayland
compositor).
However, Xwayland cannot use relative motion events for actual pointer
location because that would cause a drift over time, the pointer being
actually controlled by the Wayland compositor.
So Xwayland needs to be able to send only relative raw events, hence
this API.
Bump the ABI_XINPUT_VERSION minor version to reflect that API addition.
v2: Actually avoid sending motion events (Peter)
v3: Keep sending raw emulated events with RAWONLY (Peter)
Suggested-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Related: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1130
(cherry picked from commit b5e1f13681)
Resolves warning from Oracle Parfait static analyser:
Warning: Impossible or redundant condition
Impossible or redundant condition [impossible-redundant-condition]:
Condition 'dev != NULL' of branch is determined by previous branch
at line 270 of dix/devices.c in function 'AddInputDevice'.
Condition 'dev != NULL' from this branch implies following branch is always true at line 262
Fixes: commit 493ad83323
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Avoid leaking un-initalized memory to clients by zeroing the
whole pixmap on initial allocation.
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Most (but not all) of these were found by using
codespell --builtin clear,rare,usage,informal,code,names
but not everything reported by that was fixed.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
If a client is in the process of being closed down, then its client->osPrivate
pointer will be set to NULL by CloseDownConnection. This can cause a crash if
freeing the client's resources results in a call to AttendClient. For example,
if the client has a pending sync fence:
Thread 1 "X" received signal SIGSEGV, Segmentation fault.
AttendClient (client=0x5571c4aed9a0) at ../os/connection.c:942
(gdb) bt
#0 AttendClient (client=0x5571c4aed9a0) at ../os/connection.c:942
#1 0x00005571c3dbb865 in SyncAwaitTriggerFired (pTrigger=<optimized out>) at ../Xext/sync.c:694
#2 0x00005571c3dd5749 in miSyncDestroyFence (pFence=0x5571c5063980) at ../miext/sync/misync.c:120
#3 0x00005571c3dbbc69 in FreeFence (obj=<optimized out>, id=<optimized out>) at ../Xext/sync.c:1909
#4 0x00005571c3d7a01d in doFreeResource (res=0x5571c506e3d0, skip=skip@entry=0) at ../dix/resource.c:880
#5 0x00005571c3d7b1dc in FreeClientResources (client=0x5571c4aed9a0) at ../dix/resource.c:1146
#6 FreeClientResources (client=0x5571c4aed9a0) at ../dix/resource.c:1109
#7 0x00005571c3d5525f in CloseDownClient (client=0x5571c4aed9a0) at ../dix/dispatch.c:3473
#8 0x00005571c3d55eeb in Dispatch () at ../dix/dispatch.c:492
#9 0x00005571c3d59e96 in dix_main (argc=3, argv=0x7ffe7854bc28, envp=<optimized out>) at ../dix/main.c:276
#10 0x00007fea4837cb6b in __libc_start_main (main=0x5571c3d1d060 <main>, argc=3, argv=0x7ffe7854bc28, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7ffe7854bc18) at ../csu/libc-start.c:308
#11 0x00005571c3d1d09a in _start () at ../Xext/sync.c:2378
(gdb) print client->osPrivate
$1 = (void *) 0x0
Since the client is about to be freed, its ignore count doesn't matter and
AttendClient can simply be a no-op. Check for client->clientGone in AttendClient
and remove similar checks from two callers that had them.
Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
This ensures that any prep work for the drawable we're about to read
from is already done before we call down to GetImage. This should be no
functional change as most of the callers with a non-trivial
SourceValidate are already wrapping GetImage and doing the equivalent
thing, but we'll be simplifying that shortly.
More importantly this ensures that if any of that prep work would
generate events - like automatic compositing flushing rendering to a
parent pixmap which then triggers damage - then it happens entirely
before we start writing the GetImage reply header.
Note that we do not do the same for GetSpans, but that's okay. The only
way to get to GetSpans is through miCopyArea or miCopyPlane - where the
callers must already call SourceValidate - or miGetImage - which this
commit now protects with SourceValidate.
Fixes: xorg/xserver#902
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Slightly simplifies the callers since they don't need to check for
non-NULL anymore.
I do extremely hate the workarounds here to suppress misprite taking the
cursor down though. Surely there's a better way.
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
When we set these up initially, no subsystems have allocated any
privates yet, so the storage address will be null, and memset(NULL, ...)
is undefined.
Request-handlers as registered in the requestVector array, always get
passed the clientPtr for the client which sent the request.
But the implementation of many request-handlers typically consists of
a generic handler calling implementation specific callbacks and / or
various helpers often multiple levels deep and in many cases the clientPtr
does not get passed to the callbacks / helpers.
This means that in some places where we would like to have access to the
current-client, we cannot easily access it and fixing this would require
a lot of work and often would involve ABI breakage.
This commit adds a GetCurrentClient helper which can be used as a
shortcut to get access to the clienPtr for the currently being processed
request without needing a lot of refactoring and ABI breakage.
Note using this new GetCurrentClient helper is only safe for code
which only runs from the main thread, this new variable MUST NOT be used
by code which runs from signal handlers or from the input-thread.
The specific use-case which resulted in the creation of this patch is adding
support for emulation of randr / vidmode resolution changes to Xwayland.
This emulation will not actually change the monitor resolution instead it
will scale any window with a size which exactly matches the requested
resolution to fill the entire monitor. The main use-case for this is
games which are hard-coded to render at a specific resolution and have
sofar relied on randr / vidmode to change the monitor resolution when going
fullscreen.
To make this emulation as robust as possible (e.g. avoid accidentally scaling
windows from other apps) we want to make the emulated resolution a per client
state. But e.g. the RRSetCrtc function does not take a client pointer; and is
a (used) part of the Xorg server ABI (note the problem is not just limited
to RRSetCrtc).
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
There is a race when reseting the XServer that causes spriteInfo to be
NULL in GetPairedDevice resulting a segfault and subsequent crash. The
problem was noticed when opening a connection, creating master devices,
destroying master devices and closing the connection during testing.
Signed-off-by: Arthur Williams <taaparthur@gmail.com>
When compiling with link time optimization, GCC thinks it's discovered
undefined behavior:
events.c: In function 'XineramaConfineCursorToWindow':
events.c:609:13: warning: iteration 2147483647 invokes undefined behavior [-Waggressive-loop-optimizations]
events.c:609:11: note: within this loop
events.c:605:49: warning: array subscript -1 is below array bounds of 'struct _Window *[16]' [-Warray-bounds]
events.c:606:31: warning: array subscript -1 is below array bounds of 'struct _Screen *[16]' [-Warray-bounds]
events.c:610:39: warning: array subscript -2 is below array bounds of 'struct _Screen *[16]' [-Warray-bounds]
events.c:617:38: warning: array subscript -2 is below array bounds of 'struct _Window *[16]' [-Warray-bounds]
events.c:619:35: warning: array subscript -2 is below array bounds of 'struct _Screen *[16]' [-Warray-bounds]
This results from
i = PanoramiXNumScreens - 1;
RegionCopy(&pSprite->Reg1, &pSprite->windows[i]->borderSize);
off_x = screenInfo.screens[i]->x;
off_y = screenInfo.screens[i]->y;
where GCC believes that PanoramiXNumScreens might be 0. Unfortunately
GCC is just smart enough to be an annoyance because this case is not
actually possible: XineramaConfineCursorToWindow() is only called when
noPanoramiXExtension is false, and if noPanoramiXExtension is false then
PanoramiXNumScreens must be >1 (see PanoramiXExtensionInit()).
So, add an assert(!noPanoramiXExtension), which to my surprise provides
GCC with information even in release builds and lets GCC understand that
the code is not doing anything that is undefined behavior.
I chose this solution instead of the proposed assert(i >= 0) because the
same pattern occurs in CheckVirtualMotion() but is inside an
'if (!noPanoramiXExtension)' and does not generate any warnings.
Fixes: xorg/xserver#590
Signed-off-by: Matt Turner <mattst88@gmail.com>
Separate each statement of the form "assert(a && b);" into "assert(a);"
and "assert(b);" for more precise diagnostics, except for this clever
use in drmmode_display.c where it was used to pass a hint to developers:
assert(num_infos <= 32 && "update return type");
We hide CWBackingStore from the screen hook if nothing's actually
changing, which means compChangeWindowAttributes no longer needs to
compare the requested state with the present one.
Terms:
dev->last.valuator[] is the last value given to us by the driver
dev->valuator.axisVal[] is the last value sent to the client
dev->last.scroll[] is the abs value of the scroll axis as given by the driver,
used for button emulation calculation (and the remainder)
This function updates the device's last.valuator state based on the current
master axis state. This way, relative motion continues fluidly when switching
between devices. Before mouse 2 comes into effect, it's valuator state is
updated to wherever the pointer currently is so the relative event applies on
top of that.
This can only work for x/y axes, all other axes aren't guaranteed to have the
same meaning and/or may not be present:
- xtest device: no valuator 2
- mouse: valuator 2 is horizontal scroll axis
- tablet: valuator 2 is pressure
Scaling the current value from the pressure range into the range for
horizontal scrolling makes no sense. And it causes scroll jumps:
- scroll down, last.valuator == axisVal == 20
- xdotool click 1, the XTest device doesn't have that valuator
- scroll up
- updateSlaveDeviceCoords reset last.valuator to 0 (axisVal == 20)
- DeviceClassesChangedEvent includes value 20 for the axis
- event is processed, last.value changes from 0 to -1
- axisVal is updated to -1, causing a jump of -21
The same applies when we switch from tablet to mouse wheel if the pressure
value is 0 on proximity out (basically guaranteed). So let's drop this code
altogether and only leave the scaling for the relative x/y motion.
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
The `LimitClient` is set once and for all at startup, whereas the
function `ResourceClientBits()` which returns the client field offset
within the XID based on the value of `LimitClient` can be called
repeatedly.
Small optimization, cache the result of `ilog2()`, that saves running
the same loop over and over each time `ResourceClientBits()` is called.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
If the server resets, most client workqueues are cleaned up as the
clients are killed.
The one exception is the server's client, which is exempt from
the killing spree.
If that client has a queued work procedure active, it won't get
cleared on reset.
This commit ensures it gets cleared too.
c67f2eac56 ("dix: always send focus event on grab change") made dix
always sent events when it's a NotifyGrab or NotifyUngrab, even if
from == to, because 'from' can just come from a previous XSetInputFocus
call.
However, when an application calls XGrabKeyboard several times on
the same window, we are now sending spurious FocusOut+FocusIn with
NotifyGrab, even if the grab does not actually change. This makes screen
readers for blind people spuriously emit activity events which disturb
screen reading workflow when e.g. switching between menus.
This commit avoids calling DoFocusEvents in that precise case, i.e. when
oldWin is a previous grab and the new grab is the same window.
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Adam Jackson <ajax@redhat.com>
The screensaver can regularly move its window to random offsets. It should
use the ConfigureWindow function instead of calling the Screen's MoveWindow
directly. Some MoveWindow implementations, such as compMoveWindow, rely on
Screen's ConfigNotify being called first as it happens in ConfigureWindow.
Reviewed-by: Adam Jackson <ajax@redhat.com>
This reverts commit b45c74f0f2.
It broke the cursor in other games. Apparently those use cursor data
with premultiplied alpha, but with some pixels having r/g/b values
larger than the alpha value (which corresponds to original r/g/b
values > 1.0), triggering the workaround.
Seems the cure turned out worse than the disease, so revert.
Bugzilla: https://bugs.freedesktop.org/108650
Turns out some apps (e.g. the Civilization VI game) use
non-premultiplied cursor data which doesn't have any pixels with 0 alpha
but non-0 non-alpha, but can still result in visual artifacts.
This uses the method suggested by Kamil in
https://bugs.freedesktop.org/92309#c19: check for pixels where any
colour component value is larger than the alpha value, which isn't
possible with premultiplied alpha.
There can still be non-premultiplied data which won't be caught by this,
but that should result in slightly incorrect colours and/or blending at
the worst, not wildly incorrect colours such as shown in the bug report
below.
Bugzilla: https://bugs.freedesktop.org/108355
Suggested-by: Kamil Paral <kamil.paral@gmail.com>
This hasn't done anything besides return TRUE in a long long time.
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Adam Jackson <ajax@redhat.com>
This variable was no longer being read anywhere. MAXCLIENTS the macro is
the compile-time maximum limit, LIMITCLIENTS the macro is the default
limit, LimitClients the variable is the limit for the current server.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Coverity complains about a use after free in here after the
freeing, I can't follow the linked list so well, but whot
says the device can only be on one list once, so break should
fix it.
Signed-off-by: Dave Airlie <airlied@redhat.com>
Not sure what if anything calls XSetDeviceModifierMapping() but this would've
failed all the time. check_modmap_change() returns Success but we were
treating it like a boolean. Fix this.
Reported-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
