Currently it is possible to iterate over client-owned resources
during client destruction that have had their associated memory
released.
This can occur when client code calls wl_client_destroy(). The
following sequence illustrates how this may occur.
1. The server initiates destruction of the connected client via
call to wl_client_destroy().
2. Resource destroy listeners / destructors are invoked and
resource memory is freed one resource at a time [1].
3. If a listener / destructor for a resource results in a call
to wl_client_for_each_resource(), the iteration will proceed
over resources that have been previously freed in step 2,
resulting in UAFs / crashes.
The issue is that resources remain in the client's object map
even after they have had their memory freed, and are removed
from the map only after each individual resource has had its
memory released.
This patch corrects this by ensuring resource destruction first
invokes listeners / destructors and then removing them from the
client's object map before releasing the associated memory.
[1] https://gitlab.freedesktop.org/wayland/wayland/-/blob/main/src/wayland-server.c?ref_type=heads#L928
Signed-off-by: Thomas Lukaszewicz thomaslukaszewicz@gmail.com
For years it's been common practice to free the object containing
the wl_listener inside resource destruction notifiers, but not
remove the listener from the list.
That is: It's been safe to assume (when only one listener is present)
that the wl_listener will never be touched again, since this is
a destruction callback.
Recently some patches were reviewed that made some positive changes
to our internal signal handling code, but would've violated this
assumption, and changed free()d memory in several existing compositors
(weston, mutter, enlightenment).
Since the breakage was extremely subtle, codify this assumption in
a test case (thus promoting it to an ABI promise).
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Reviewed-by: Markus Ongyerth <wl@ongy.net>
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Some headers and source files have been using types such as uint32_t
without explicitly including stdint.h.
Explicitly include stdint.h where appropriate.
Signed-off-by: Yong Bakos <ybakos@humanoriented.com>
Reviewed-by: Eric Engestrom <eric.engestrom@imgtec.com>
We're creating resources with versions up to 4. wl_display isn't version 4,
so this is technically verifying that we can do something we shouldn't.
wl_seat already has versions this high, so switch to that.
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Reviewed-by: Marek Chalupa <mchqwerty@gmail.com>
Reviewed-by: Bryce Harrington <bryce@osg.samsung.com>
Even if nothing receives the even, the arguments still need to be valid.
The test is sending out event 0 from the wl_display interface, which is
the error event. This requires arg 0 to be a valid object and arg 2 to
be a non-null string. The test just leaves that undefined, causing
intermittent test failures.
As it is, the resource destroy test doesn't need to send an event to
validate the various resource destroy hooks, so we can just remove the
call to wl_resource_post_event() alltogether.
Thanks to Matt Turner <mattst88@gmail.com> for pointing out the failure.
