[PATCH] server: Fix crash when accessing client which is already freed

[Daniel Stone]

Hi Hyun Kook,

On 23 September 2016 at 00:40, Hyun Kook Khang <hyunkook.khang at lge.com>
wrote:

> in wl_client_connection_data(),
> there is no guarantee that client would never be destroyed in the process
> of handling the pending input.
>
> For example,
> 1. It comes to wl_client_connection_data()
> 2. wl_closure_invoke() would be invoked, and then wl_display_flush_clients
> would be invoked anyhow later in the same routine. (wl_closure_invoke() ->
> ? -> wl_display_flush_clients())
>     Here, client will be destroyed if wl_connection_flush() returns
> negative value and errno is not EAGAIN.
>

Out of interest, what is the '?' here? I can't find any examples of this
happening in Wayland, Weston, or Mesa. If you could share your usecase,
this would be quite interesting. I believe the idea behind deferring the
flush was to minimise the number of system calls made, and also somewhat
decrease the latency when multiple clients are involved, by batching all
the flushes (potentially time-consuming) to a point where we do not have
any more requests to process (so are not sensitive to latency).

In general, Wayland compositors should always be responsive: client request
handling should be lightweight and not time-consuming. Wayland clients
should also be completely asynchronous, rather than following a
RPC/method-return pattern of { send_request(); block_for_reply(); }. Given
that, the only reason to flush inside request processing would be because
you are sending a lot of data, but again the Wayland protocol itself should
not be carrying large amounts of data. So I'm not sure what usecase there
is for this.


> 3. It will come back to the while loop in wl_client_connection_data().
>     If wl_connection_pending_input() returns any values which is bigger
> than “size of p”; it will go into the while loop again, and then it will do
> something with the client which is already freed.
>
>
> The simplest way to solve this is checking the source(wl_event_source)’
> fd, but for now there is no way to access wl_event_source in
> wayland-server.c. (wl_event_source will be freed a while later)
>

Hm, this is racy though.

I'm would suggest we document that invoking wl_display_flush_clients() /
wl_connection_flush() from a request handler may cause undefined behaviour,
and that it must only be called outside of dispatch.

Cheers,
Daniel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20161121/4fd09685/attachment.html>