(master) modesetting: save cursor in master's sprite_priv instead of slave's

[metux]

When using xdotool mouseup 1 in Chromium 136, a slave device is passed
to drmmode_sprite_set_cursor.

In dix/events.c, dev->spriteInfo->sprite for slave devices points to
their master device when they are not floating, allowing these functions
to handle slave device cases correctly.

Similarly, in mi/misprite.c, miSpritePointerFuncs checks
IsFloating for the incoming device, and GetSprite converts slave
devices to master devices if a non-floating slave is passed.

By contrast, the drmmode_sprite_funcs series functions track
per-screen cursor reference counts. This cursor state should also be
recorded on the master device rather than the non-floating slave device.

This inconsistency between drmmode_sprite_funcs and other sprite
handlers leads to the following issues in the drmmode sprite driver:

1. The slave device stores state that it shouldn't record.
2. The master device's record becomes stale. (e.g., ChangeToCursor in
   dix/events.c called for a slave device updates the master's sprite,
   causing the subsequent comparison between old and new cursors on the
   master device to fail.)

This patch resolves the inconsistency by using GetMaster when the device
is not floating, aligning the behavior with mi/misprite.c.

---

The reason physical devices work while XTEST devices fail is that
ActivateGrab and DeactivateGrab in XTEST devices' deviceGrab are
set to ActivatePointerGrab and DeactivatePointerGrab, respectively.
Physical devices use ActivateKeyboardGrab and
DeactivateKeyboardGrab. Thus, releasing a physical device will not
trigger DeactivatePointerGrab.

The physical device is initialized by AddInputDevice using
"KeyboardGrab", whereas the XTEST device is initialized by
AllocDevicePair, which overrides the default KeyboardGrab with
"PointerGrab".

(gdb) bt
#0 drmmode_sprite_set_cursor (pDev=0x5555569b88f0, pScreen=0x555555c26990, pCursor=0x55555702c580, x=681, y=181)
at ../xorg-server/hw/xfree86/drivers/modesetting/drmmode_display.c:4349
#1 0x00005555555980a6 in miPointerUpdateSprite (pDev=0x5555569b88f0) at ../xorg-server/mi/mipointer.c:490
#2 0x0000555555597877 in miPointerDisplayCursor (pDev=0x5555569b88f0, pScreen=0x555555c26990, pCursor=0x55555702c580) at ../xorg-server/mi/mipointer.c:208
#3 0x000055555568ef1f in CursorDisplayCursor (pDev=0x5555569b88f0, pScreen=0x555555c26990, pCursor=0x55555702c580) at ../xorg-server/xfixes/cursor.c:168
#4 0x0000555555644150 in AnimCurDisplayCursor (pDev=0x5555569b88f0, pScreen=0x555555c26990, pCursor=0x55555702c580) at ../xorg-server/render/animcur.c:197
#5 0x00005555555d5f12 in ChangeToCursor (pDev=0x5555569b88f0, cursor=0x55555702c580) at ../xorg-server/dix/events.c:952
#6 0x00005555555d60ad in PostNewCursor (pDev=0x5555569b88f0) at ../xorg-server/dix/events.c:1003
#7 0x00005555555d79ac in DeactivatePointerGrab (mouse=0x5555569b88f0) at ../xorg-server/dix/events.c:1716
#8 0x000055555569d217 in ProcessDeviceEvent (ev=0x7fffe4073c20, device=0x5555569b88f0) at ../xorg-server/Xi/exevents.c:1955
#9 0x000055555569d433 in ProcessOtherEvent (ev=0x7fffe4073c20, device=0x5555569b88f0) at ../xorg-server/Xi/exevents.c:2020
#10 0x00005555556eb297 in ProcessPointerEvent (ev=0x7fffe4073c20, mouse=0x5555569b88f0) at ../xorg-server/xkb/xkbAccessX.c:756
#11 0x000055555558c86c in mieqProcessDeviceEvent (dev=0x5555569b88f0, event=0x7fffe4073c20, screen=0x555555c26990) at ../xorg-server/mi/mieq.c:503
#12 0x00005555556692d9 in ProcXTestFakeInput (client=0x555556b5bf60) at ../xorg-server/Xext/xtest.c:434
#13 0x000055555566941a in ProcXTestDispatch (client=0x555556b5bf60) at ../xorg-server/Xext/xtest.c:468
#14 0x00005555555c002c in Dispatch () at ../xorg-server/dix/dispatch.c:552
#15 0x00005555555cf546 in dix_main (argc=8, argv=0x7fffffffe9b8, envp=0x7fffffffea00) at ../xorg-server/dix/main.c:274
#16 0x00005555557bbf07 in main (argc=8, argv=0x7fffffffe9b8, envp=0x7fffffffea00) at ../xorg-server/dix/stubmain.c:34
(gdb) p pDev->name
$1 = 0x5555569b9960 "Virtual core XTEST pointer"
(gdb) frame 12
#12 0x00005555556692d9 in ProcXTestFakeInput (client=0x555556b5bf60) at ../xorg-server/Xext/xtest.c:434
434 mieqProcessDeviceEvent(dev, &xtest_evlist[i], miPointerGetScreen(inputInfo.pointer));
(gdb) p client->clientIds[0]->cmdname
$2 = 0x5555570852f0 "xdotool"
(gdb) p client->clientIds[0]->cmdargs
$3 = 0x555557078b30 "mouseup 1"

Closes: #1685
Part-of: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2282
(cherry picked from commit e19e86c)
(cherry picked from commit 035caac7b2468fd3857e0c2bff74956272993974)
(cherry picked from commit f170a12)

[metux]

🤖 Automated review — generated by Starfleet ship Voyager (model: heavy-model) on behalf of @metux. Not a human review.

Review: changes requested — mandatory trailer missing

The code change is sound. The commit message is not acceptable yet.

Code: fine

The macro

#define msGetSpritePriv(dev, ms, screen) dixLookupScreenPrivate(&(dev)->devPrivates, ...)

becomes a function that resolves the master device first:

    if (!IsFloating(pDev))
        pDev = GetMaster(pDev, MASTER_POINTER);
    return dixLookupScreenPrivate(&(pDev)->devPrivates, ...);

Checked for completeness: the private key is registered in driver.c:2230 and read
through msGetSpritePriv at exactly two call sites (drmmode_sprite_set_cursor,
drmmode_sprite_move_cursor), both of which now go through the helper. No third site
looks the key up directly, so nothing is left on the un-resolved path. Converting the
macro to a function is the right move — a macro could not have expressed the two-step
resolve-then-lookup without evaluating dev twice.

This matches the reported failure: xdotool mouseup 1 in Chromium passes a slave device,
whose devPrivates does not carry the screen-private that dixLookupScreenPrivate wants,
so the lookup returns the wrong (or no) private. Closes #1685.

Blocking: Signed-off-by: is missing

gh api repos/X11Libre/xserver/commits/<head> -q .commit.message | grep -c '^Signed-off-by:'
0

Signed-off-by: is mandatory in this project, and every other PR in this xorg-backport
batch carries it. The incubator commit for this PR (55a29cf913, author
Ben Song <bensongsyz@gmail.com>) lacks it too, so this is not something introduced by
the submission tooling — the trailer was never added.

Note that the sibling case #3777 has the same gap in the incubator and was fixed before
submission, so the fix path already exists for this batch; it just was not applied here.

Per the convention for xorg ports, the line names the original author of the upstream
commit, not whoever transferred it. The original author here is Ben Song <bensongsyz@gmail.com>, which is also the commit's Author: — so the trailer can be
taken directly from that.

What to do

Add the trailer to the incubator commit and to the PR branch, then re-run CI (the message
change alone should be enough, but the check will re-trigger).

This is a process defect rather than a code defect, so it is the only thing blocking the
merge — nothing in the diff itself needs changing.

Backport

Client-triggerable use-after-lookup/wrong-private in the modesetting driver under
ProcXTestFakeInput. Worth a decision per release line after the trailer is fixed;
recommend flagging it as a backport candidate, applicability per branch is the
maintainer's call.

[metux]

🤖 Automated review — generated by Starfleet ship Voyager (model: heavy-model) on behalf of @metux. Not a human review.

Review: passed (superseding the earlier changes-requested)

The blocking finding from the previous review — an unresolvable IsFloating — is fixed.
Verified against the current head 7d3f1a9441, not against the earlier one.

What the fix is

-    if (!IsFloating(pDev))
+    if (!InputDevIsFloating(pDev))
         pDev = GetMaster(pDev, MASTER_POINTER);

IsFloating does not exist in this tree, which is what produced:

error: implicit declaration of function 'IsFloating'
error: nested extern declaration of 'IsFloating'

InputDevIsFloating is the correct name, declared in dix/input_priv.h:488, and reachable
from this file because dix/dix_priv.h:20 includes dix/input_priv.h and drmmode_display.c:35
includes dix/dix_priv.h. No new include was needed, and none was added — verified that the
file does not grow a stray include.

Completeness of the change

The conversion from macro to function was the point, so I checked that nothing still uses
the old path. All lookups now go through the one helper:

5002  msGetSpritePriv(DeviceIntPtr pDev, modesettingPtr ms, ScreenPtr pScreen)   <- definition
5006      return dixLookupScreenPrivate(...)                                     <- the single lookup
5038  msSpritePrivPtr sprite_priv = msGetSpritePriv(pDev, ms, pScreen);          <- set_cursor
5051  msSpritePrivPtr sprite_priv = msGetSpritePriv(pDev, ms, pScreen);          <- move_cursor

No remaining direct dixLookupScreenPrivate call against spritePrivateKeyRec outside the
helper, so there is no second site that would still record the slave's state. That was the
substantive risk when a macro became a function; it is not present.

The semantics are right

The commit's premise is that drmmode_sprite_funcs tracks per-screen cursor reference
counts, and that this state belongs on the master device rather than on a non-floating
slave — matching what mi/misprite.c already does. Resolving via
GetMaster(pDev, MASTER_POINTER) when the device is not floating is the same approach
miSpritePointerFuncs uses, so the two handlers now agree. Floating devices keep their own
private, which is the case the original code got right and must not lose.

Trailers

Signed-off-by: Ben Song <bensongsyz@gmail.com> present, author unchanged — the original
xorg contribution, not rewritten.

Note on the two earlier reports on this PR

Recorded because both were wrong in ways that are worth not repeating:

• The first report said the PR "carries a bug that must be fixed before merge" and that the
  blocker was the missing trailer. The trailer was present from the start; the real blocker
  was the unresolvable IsFloating.
• A later message of mine stated the fix had landed in the PR branch and that the incubator
  was unchanged. Both were checked afterwards and the incubator does carry the fix, so the
  claim was accidentally right but was asserted before it was verified.

CI

At the time of this comment the five xserver-build-qemu-user lanes were still running;
they are the lanes that failed on the unresolvable symbol. No lane has failed so far on
this head. The earlier failure mode is therefore expected to be resolved, but that is a
prediction, not a measurement — the merge decision should follow those five lanes.

Merge: master PR, so a bot-review-passed verdict plus green CI permits a merge if the
maintainer asks for one. Always --rebase on this repository.