If the default queue is being destroyed, the client is disconnecting
from the wl_display, so there is no possibility of subsequent events
being queued to the destroyed default queue, which is what this warning
is about.
Note that interacting with (e.g., destroying) a wl_proxy after its
wl_display is destroyed is a certain memory error, and this warning will
indirectly warn about this issue. However, this memory error should be
detected and warned about through a more deliberate mechanism.
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
Detect when we are trying to add an event to a destroyed queue,
and abort instead of causing a use-after-free memory error.
This situation can occur when an wl_event_queue is destroyed before
its attached wl_proxy objects.
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
Log a warning if the queue is destroyed while proxies are still
attached, to help developers debug and fix potential memory errors.
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
Maintain a list of all wl_proxy objects that are attached to a
wl_event_queue. We will use this information in upcoming commits to warn
about improper object destruction order that can lead to memory errors.
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
Assignments to a wl_proxy's queue member are currently not synchronized
with potential reads of that member during event reading/queuing.
Assuming atomic pointer value reads and writes (which is a reasonable
assumption), and using the documented best practices to handle event
queue changes, a queue change should still be safe to perform.
That being said, such implicitly atomic accesses are difficult to assess
for correctness, especially since they do not introduce memory barriers.
To make the code more obviously correct, and handle any potential races
we are not currently aware of, this commit updates wl_proxy_set_queue()
to set the proxy's event queue under the display lock (all other
proxy queue accesses are already done under the display lock).
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Document the proper way to deal with event queue changes, in order to
guarantee proper handing of all events which were queued before the
queue change takes effect, especially in multi-threaded setups.
Make a special note about queue changes of newly created proxies,
which require the use of a proxy wrapper for thread safety.
Signed-off-by: Alexandros Frantzis <alexandros.frantzis@collabora.com>
Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
The [spec][1] reads:
> All paths set in these environment variables must be absolute. If an
> implementation encounters a relative path in any of these variables it should
> consider the path invalid and ignore it.
and
> If $XDG_DATA_HOME is either not set or empty, a default equal to
> $HOME/.local/share should be used.
Testing that the path is absolute also entails that is is non-empty.
[1]: https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html
Signed-off-by: Antonin Décimo <antonin.decimo@gmail.com>
When allocating memory for structs, use zalloc instead of malloc.
This ensures the memory is zero-initialized, and reduces the risk
of forgetting to initialize all struct fields.
Signed-off-by: Simon Ser <contact@emersion.fr>
The client side closure traces have incorrect object ids for new server
generated objects. This is because create_proxies() overwrites the id in
'n' type arguments by storing a pointer to the actual object in the 'o'
field of the union.
Getting back to an id from this pointer requires accessing a structure
that isn't visible outside of wayland-client.c.
Add a function pointer to fish the correct value out of the argument and
pass it to wl_closure_print.
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
This makes wl_display_connect fail immediately instead of
succeeding when the integer provided by WAYLAND_SOCKET does
not refer to a valid file descriptor.
Signed-off-by: Manuel Stoeckl <code@mstoeckl.com>
There's a race when destroying wayland objects in a multi-threaded client.
This occurs because we call:
wl_proxy_marshal(foo);
wl_proxy_destroy(foo);
And each of these functions takes, and releases, the display mutex.
Between the two calls, the display is not locked.
In order to allow atomically marshalling the proxy and destroying the
proxy without releasing the lock, add yet more wl_proxy_marshal_*
functions. This time add flags and jam in all existing warts with the
hope that we can make it future proof this time.
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Split wl_proxy_destroy into two pieces, wl_proxy_destroy_unlocked which
performs the critical section code with no locking, and wl_proxy_destroy
which locks before calling that.
We'll use the new unlocked variant later in code that already holds the
lock.
There is a slight functional change - an aborting check is now called
with the lock held. This should be harmless as wl_abort() performs
no locking.
Signed-off-by: Derek Foreman <derek.foreman@collabora.com>
Specifically, in the log formed when WAYLAND_DEBUG is set, this commit
ensures that floating point numbers are formatted using '.' instead of
the locale-specific decimal separator. As the debug logs are not
otherwise localized for end-users, and may be parsed by scripts, it is
better to have consistent output here.
The 24.8 fixed point numbers are now represented with 8 digits after
the decimal, since this is both exact and simpler to compute.
Signed-off-by: Manuel Stoeckl <code@mstoeckl.com>
Before this patch, setting WAYLAND_DEBUG=1 or WAYLAND_DEBUG=client made
a program log all requests sent and events that it processes. However,
some events received are not processed. This can happen when a Wayland
server sends an event to an object that does not exist, or was recently
destroyed by the client program (either before the event was decoded,
or after being decoded but before being dispatched.)
This commit prints all discarded messages in the debug log, producing
lines like:
[1234567.890] discarded [unknown]@42.[event 0](0 fd, 12 byte)
[1234567.890] discarded wl_callback@3.done(34567)
[1234567.890] discarded [zombie]@13.[event 1](3 fd, 8 byte)
The first indicates an event to an object that does not exist; the
second, an event to an object that was deleted after decoding, but
before dispatch; the third, an event to an object that left a
'zombie' marker behind to indicate which events have associated
file descriptors.
Signed-off-by: Manuel Stoeckl <code@mstoeckl.com>
In wl_proxy_set_queue, passing a wl_event_queue from a completely
unrelated wl_display could lead to object IDs mismatches.
Add an assertion to catch this case. It's always a user bug if this
happens.
Signed-off-by: Simon Ser <contact@emersion.fr>
Add a paragraph about WAYLAND_SOCKET and describe what happens when the display
name is a relative path.
Signed-off-by: Simon Ser <contact@emersion.fr>
Instead, set a fatal display error which will let an application
using libwayland-client shutdown cleanly.
Signed-off-by: Manuel Stoeckl <code@mstoeckl.com>
Once there has been a fatal display error, any new object requests
potentially rely on invalid state. (For example, a failure to read
from the compositor could hide a important event.) The safest way to
handle the new requests is not to make them.
Proxies produced by the request are still created, to ensure that
any code using the library does not crash from an unexpected NULL
pointer.
Signed-off-by: Manuel Stoeckl <code@mstoeckl.com>
If the client binds to a global with an interface mismatch, it may receive an
event from the server with an unknown opcode. See [1].
Instead of crashing, print a more useful debug message and close the connection.
[1]: https://gitlab.freedesktop.org/wayland/wayland/issues/113
Signed-off-by: Simon Ser <simon.ser@intel.com>
When an application and a toolkit share the same Wayland connection,
it will receive events with each others objects. For example if the
toolkit manages a set of surfaces, and the application another set, if
both the toolkit and application listen to pointer focus events,
they'll receive focus events for each others surfaces.
In order for the toolkit and application layers to identify whether a
surface is managed by itself or not, it cannot only rely on retrieving
the proxy user data, without going through all it's own proxy objects
finding whether it's one of them.
By adding the ability to "tag" a proxy object, the toolkit and
application can use the tag to identify what the user data pointer
points to something known.
To create a tag, the recommended way is to define a statically allocated
constant char array containing some descriptive string. The tag will be
the pointer to the non-const pointer to the beginning of the array.
For example, to identify whether a focus event is for a surface managed
by the code in question:
static const char *my_tag = "my tag";
static void
pointer_enter(void *data,
struct wl_pointer *wl_pointer,
uint32_t serial,
struct wl_surface *surface,
wl_fixed_t surface_x,
wl_fixed_t surface_y)
{
struct window *window;
const char * const *tag;
tag = wl_proxy_get_tag((struct wl_proxy *) surface);
if (tag != &my_tag)
return;
window = wl_surface_get_user_data(surface);
...
}
...
static void
init_window_surface(struct window *window)
{
struct wl_surface *surface;
surface = wl_compositor_create_surface(compositor);
wl_surface_set_user_data(surface, window);
wl_proxy_set_tag((struct wl_proxy *) surface,
&my_tag);
}
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Rather than have two versions of the macro with slightly different
interfaces, just use wl_container_of internally.
This also removes use of statement expressions, a GNU C extension.
Signed-off-by: Michael Forney <mforney@mforney.org>
Many languages such as C++ or Rust have an unwinding error-reporting
mechanism. Code in these languages can (and must!) wrap request handling
callbacks in unwind guards to avoid undefined behaviour.
As a consequence such code will detect internal server errors, but have
no way to communicate such failures to the client.
This adds a WL_DISPLAY_ERROR_IMPLEMENTATION error to wl_display so that
such code can notify (and disconnect) clients which hit internal bugs.
While servers can currently abuse other wl_display errors for the same
effect, adding an explicit error code allows clients to tell the
difference between errors which are their fault and errors which are the
server's fault. This is particularly interesting for automated bug
reporting.
v2: Rename error from "internal" to "implementation", in sympathy with
X11's BadImplementation error.
Add more justification in the commit message.
Signed-off-by: Christopher James Halse Rogers <christopher.halse.rogers@canonical.com>
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Nothing on the client side uses it since
9fe75537ad which was just before the 0.99
release.
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Reviewed-By: Markus Ongyerth <wl@ongy.net>
previous commit, a9187853d4 added
a trailing { on a line it shouldn't have, and I pushed without
building first.
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
commit 239ba39331 which was intended
to stop leaking fds in events for zombie objects didn't notice that
passing 0 to wl_connection_close_fds_in() would still close fds.
Test the fd count before calling.
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
We need to close file descriptors sent to zombie proxies to avoid leaking
them, and perhaps more importantly, to prevent them from being dispatched
in events on other objects (since they would previously be left in the
buffer and potentially fed to following events destined for live proxies)
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Using the singleton zombie object doesn't allow us to posthumously retain
object interface information, which makes it difficult to properly inter
future events destined for the recently deceased proxy.
Notably, this makes it impossible for zombie proxy destined file
descriptors to be properly consumed.
When we create a proxy, we now create a zombie-state object to hold
information about the file descriptors in events it can receive. This
will allow us, in a future patch, to close those FDs.
[daniels: Split Derek's patch into a few smaller ones.]
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Closures created to hold events which will be dispatched on the client,
take a reference to the proxy for the object the event was sent to, as
well as the proxies for all objects referenced in that event.
These references are dropped immediately before dispatch, with the
display lock also being released. This leaves the potential for a
vanishingly small race, where another thread drops the last reference
on one of the proxies used in an event as it is being dispatched.
Fix this by splitting decrease_closure_args_refcount into two functions:
one which validates the objects (to ensure that clients are not returned
objects which they have destroyed), and another which unrefs all proxies
on the closure (object event was sent to, all referenced objects) as
well as the closure itself. For symmetry, increase_closure_args_refcount
is now the place where the refcount for the proxy for the object the
event was sent to, is increased.
This also happens to fix a bug: previously, if an event was sent to a
client-destroyed object, and the event had object arguments, a reference
would be leaked on the proxy for each of the object arguments.
Found by inspection whilst reviewing the zombie-FD-leak series.
Signed-off-by: Daniel Stone <daniels@collabora.com>
Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Cc: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Rather than open-coded decrement-and-maybe-free, introduce a
wl_proxy_unref helper to do this for us. This will come in useful for
future patches, where we may also have to free a zombie object.
Signed-off-by: Daniel Stone <daniels@collabora.com>
Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Commit e273c7cde added a refcount to wl_proxy. The refcount is set to 1
on creation, decreased when the client explicitly destroys the proxy,
and is increased and decreased every time an event referencing that
proxy is queued.
Assuming no bugs, this means the refcount cannot reach 0 without the
proxy being explicitly destroyed. However, some (not all) of the
proxy-unref paths were only destroying the proxy if it had already been
deleted. This should already be enforced by refcounting, so remove the
check and rely solely on the refcount as the arbiter of when to free a
proxy.
Signed-off-by: Daniel Stone <daniels@collabora.com>
Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
Cc: Jonas Ådahl <jadahl@gmail.com>
Since we now have the WL_MAP_ENTRY_ZOMBIE flag to determine whether or
not a client-side object is a zombie, we can remove the faux object.
[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
This makes it easier for future patches in the series, which can
possibly return NULL for extant map entries.
[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]
Reviewed-by: Daniel Stone <daniels@collabora.com>
Add a new map entry flag to indicate that the object received is valid,
but a zombie. Previously this relied on a fixed object pointer, but
future patches in this series will have map entries returning either
NULL, or a different structure type entirely, for zombie objects.
wl_object_is_zombie() now solely uses the new flag to determine whether
or not the object is a zombie.
[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Add a helper function which determines whether or not an object is a
zombie.
[daniels: Extracted from Derek's bespoke-zombie patch as an intermediate
step.]
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
In order to support system compositor instances, it is necessary to
allow clients' wl_display_connect() to find the compositor's listening
socket somewhere outside of XDG_RUNTIME_DIR. For a full account, see
the discussion beginning here:
https://lists.freedesktop.org/archives/wayland-devel/2017-November/035664.html
This change adjusts the client-side connection logic so that, if
WAYLAND_DISPLAY is formatted as an absolute pathname, the socket
connection attempt is made to just $WAYLAND_DISPLAY rather than
usual user-private location $XDG_RUNTIME_DIR/$WAYLAND_DISPLAY.
This change is based on Davide Bettio's submission of the same concept
at:
https://lists.freedesktop.org/archives/wayland-devel/2015-August/023838.html.
v4 changes:
* Improved internal comments and some boundary-condition
error checks in test case.
* Refer to compositor as "Wayland server" rather than "Wayland
display" in wl_display_connect() doxygen comments.
* Remove redundant descriptions of parameter-interpretation
mechanics from wl_display_connect() manpage. Reworked things
to make it clear that 'name' and $WAYLAND_DISLAY are each
capable of encoding absolute server socket paths.
* Remove callout to reference implementation behavior in protocol
documented. In its place there is now a simple statement that
implementations can optionally support absolute socket paths.
v3 changes:
* Added test case.
* Clarified documentation to note that 'name' parameter to wl_display_connect()
can also be an absolute path.
v2 changes:
* Added backward incompatibility note to wl_display_connect() manpage.
* Rephased wl_display_connect() manpage changes to precisely match actual
changed behavior.
* Added mention of new absolute path behavior in wl_display_connect()
doxygen comments.
* Mentioned new absolute path interpretation of WAYLAND_DISPLAY in
protocol documentation.
Signed-off-by: Matt Hoosier <matt.hoosier@gmail.com>
Acked-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Both the blocks in this if/else clause do the same thing, so combine
the comparisons into one.
No functional change.
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
Reviewed-by: Bryce Harrington <bryce@osg.samsung.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
The third arg to strtol() specifies the base to assume for the number.
When 0 is passed, as is currently done in wayland-client.c, hexadecimal
and octal numbers are permitted and automatically detected and
converted.
I can find no indication that we would ever expect use of hexadecimal or
octal for socket fd's. So be explicit about what base we're assuming
here and avoid any potential surprises.
Signed-off-by: Bryce Harrington <bryce@osg.samsung.com>
Reviewed-by: Yong Bakos <ybakos@humanoriented.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Without this commit, wl_display_roundtrip_queue() is vulnerable to a
race condition, causing the callback to be dispatched on the wrong
queue.
The race condition happens if some non-main thread calls
wl_display_roundtrip_queue() with its thread local queue, and the main
thread reads and dispatches the callback event from the
wl_display_sync() call before the thread local queue is set.
The issue is fixed by using a proxy wrapper, making the initialization
of the callback proxy atomic, effectively making it no longer possible
for some other thread to dispatch the proxy before the correct thread
local queue is set.
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
[Pekka: check display_wrapper]
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Using the libwayland-client client API with multiple threads with
thread local queues are prone to race conditions.
The problem is that one thread can read and queue events after another
thread creates a proxy but before it sets the queue.
This may result in the event to the proxy being silently dropped, or
potentially dispatched on the wrong thread had the creating thread set
the implementation before setting the queue.
This patch introduces API to solve this case by introducing "proxy
wrappers". In short, a proxy wrapper is a wl_proxy struct that will
never itself proxy any events, but may be used by the client to set a
queue, and use it instead of the original proxy when sending requests
that creates new proxies. When sending requests, the wrapper will
work in the same way as the normal proxy object, but the proxy created
by sending a request (for example wl_display.sync) will inherit to the
same proxy queue as the wrapper.
https://bugs.freedesktop.org/show_bug.cgi?id=91273
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Reviewed-by: Derek Foreman <derekf@osg.samsung.com>
proxy_destroy() is just the implementation of the atomic part of
wl_proxy_destroy(), so lets make it static.
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
If an error is received on a destroyed object, we'd get NULL passed
to display_handle_error() instead of a pointer to a valid wl_proxy.
The logging is changed to report [unknown interface] and [unknown id]
instead of the actual interface name and id.
The wl_display_get_protocol_error() documentation is updated to handle
the situation. For when the proxy was NULL, the object id 0 and
interface NULL is written.
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Marek Chalupa <mchqwerty@gmail.com>
Reviewed-by: Bryce Harrington <bryce@osg.samsung.com>
[Pekka: changed the error message wording]
Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Acked-by: Jonas Ådahl <jadahl@gmail.com>
This provides a standardized mechanism for tracking protocol object
versions in client code. The wl_display object is created with version 1.
Every time an object is created from within wl_registry_bind, it gets the
bound version. Every other time an object is created, it simply inherits
it's version from the parent object that created it.
(comments and minor reformatting added
by Derek Foreman <derekf@osg.samsung.com>)
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Second trivial commit squashed into this one:
Authored by Derek Foreman <derekf@osg.samsung.com>
Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
(it's literally one of code and a lot of comments)
This sets wl_display's version (for proxy version query purposes)
to 0. Any proxy created with unversioned API (this happens when
a client compiled with old headers links against new wayland)
will inherit this 0.
This gives us a way for new libraries linked by old clients to
realize they can't know a proxy's version.
wl_display's version being unqueryable (always returning 0) is
an acceptable side effect, since it's a special object you can't
bind specific versions of anyway.
Second half:
Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.co.uk>
Reviewed-by: Jason Ekstrand <jason@jlekstrand.net>
wl_display_flush() may fail with EAGAIN which means that not all data
waiting in the buffer has been flushed. We later block until there is
data to read, which could mean that we block on input from the
compositor without having sent out all data from the client. Avoid this
by fully flushing the socket before starting to wait.
This commit also changes the array length of the struct pollfd array
from 2 to 1, as only one element was ever used.
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
Instead of doing things that do the equivalent of using
wl_display_prepare_read() and friends, just use the public API. The
only semantical difference is that we will now unlock and lock the mutex
more times compared to before.
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
If flushing hits EPIPE it should not make it a fatal error since it
would make it impossible to process the rest of the data available in
the buffer. Instead, let reading the socket make EPIPE fatal, letting
the client have the possibility to process the last messages including
any error causing the termination.
Signed-off-by: Jonas Ådahl <jadahl@gmail.com>
Reviewed-by: Daniel Stone <daniels@collabora.com>
