Fix serial validation for xdg-shell grabs

[ddevault]

https://github.com/swaywm/wlroots/pull/731 contains a workaround that just disables validation

---

wlroots has migrated to gitlab.freedesktop.org. This issue has been moved to:

https://gitlab.freedesktop.org/wlroots/wlroots/-/issues/755

[emersion]

Related: serials are also used for some requests like setting selection. Currently we either don't check it (see wlr_seat_validate_grab_serial) or do some weird stuff like:

seat->serial - serial < UINT32_MAX / 2

[mstoeckl]

Another issue with the seat's selection serial value is that the serial used to perform the validation (now seat->selection_serial) is shared between all programs using the seat, so if one client sets its selection with a very large serial value, then validation will fail for all other clients until that serial is reached by the compositor. Serial values thus should be per-client.

[emersion]

if one client sets its selection with a very large serial value

We should check that the serial sent by the client is smaller than the current display serial.

Weston has likely the same issue.

Serial values thus should be per-client.

This isn't possible because it won't protect against races.

[mstoeckl]

Right, I think I understand it now. The "correct" validation for primary/gtk selection serials is to verify that

1. The client provided a serial value that the compositor previously sent to it. (Otherwise, the client could try a large set of sequential serial values, and steal the role of being the selection source from another application by picking.)
2. The serial provided by the request comes after the seat's selection source's serial. (Otherwise, given applications A,B, input events iA, iB, and selection requests sA,sB, if the ordering is iA,iB,sB,sA , then application A would provide the copy-paste content even though the user most recently asked for it from B.)

Because serials are only 32-bit, "after" can only be correctly defined for serial values closer than 2^31.

Validating the second property is easy and has already been done. Fully validating the first is significantly more complicated: it looks like the only way to do this correctly is to record all serial values sent to a client. We can approximate the first property by ignoring all "future" serial values.

If wlroots would aim for full validation, the serial recording can be relaxed slightly and kept fast by using a fixed size ring buffer (say, using 32 KB of memory) and assuming that all sufficiently old serial values are valid (but would probably fail the second test.)

Weston has likely the same issue.

It too only checks property 2. KWayland also has a bunch of "TODO: validate serial" comments. mutter also only checks property 2, with the exact same comparison as the other two.

[mstoeckl]

I have a rough implementation of the ring-buffer approach; see https://github.com/mstoeckl/wlroots/commits/serial-validation for details. It currently stores the validation ring buffer with the seat, and only validates selections, so there's quite a bit of work and testing left to do. (xdg_popup.grab and other serial validation, creating only one validation structure per wl_display, verifying that multiseat works, sending error messages to clients that use serials they were never given...) If anyone else wants to pick this up, they may, as I don't know when I'll get back to this.

Edit: In retrospect, it would significantly more reliable to associate a ring-buffer-set of serial numbers with each struct wlr_seat_client; this would also require fewer interface changes. But then, because serials aren't shared between seats, an action on one seat cannot authorize a selection request on another. See https://github.com/mstoeckl/wlroots/commits/serial-cache for an implementation.

Edit 2: Both implementations are fatally flawed, in that wl_display::sync provides clients with the current serial, and cannot easily be replaced with a custom implementation.