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

[Hyun Kook Khang]

Giulio,

Sorry for resending same email. (previous one got failed to send to wayland-devel at lists.freedesktop.org <mailto:wayland-devel at lists.freedesktop.org>; so I’m sending the same one again)

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.
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)

Thanks,

Hyunkook Khang / 강 현 국
Research Engineer
Reference Platform Task
Mobile: 82-10-7249-9909
E-mail: hyunkook.khang at lge.com <mailto:hyunkook.khang at lge.com>
Seocho R&D Campus, 19, Yangjae-daero 11gil
Seocho-gu, Seoul 137-130, Korea




> On Sep 22, 2016, at 4:50 PM, Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
> 
> Hi,
> 
> could you write how to trigger the crash, also in the commit message
> maybe? Besides that, i have a comment inline below.
> 
> 2016-09-21 9:08 GMT+02:00 Hyunkook Khang <hyunkook.khang at lge.com>:
>> While processing pending data, client could be destroyed in the middle of
>> the process. (e.g. by invoking wl_display_flush_clients()).
>> In this case, client will be freed, but we are still in the processing data
>> of the client, so it could cause a crash.
>> 
>> To avoid this, instead of destroying the client directly,
>> just set the error here and destroy the client where it needs to be.
>> 
>> Signed-off-by: Hyunkook Khang <hyunkook.khang at lge.com>
>> ---
>> src/wayland-server.c |    8 +++++++-
>> 1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/wayland-server.c b/src/wayland-server.c
>> index 9d7d9c1..89d0bac 100644
>> --- a/src/wayland-server.c
>> +++ b/src/wayland-server.c
>> @@ -1103,10 +1103,16 @@ wl_display_terminate(struct wl_display *display)
>> WL_EXPORT void
>> wl_display_run(struct wl_display *display)
>> {
>> +       struct wl_client *client, *next;
>> +
>>        display->run = 1;
>> 
>>        while (display->run) {
>>                wl_display_flush_clients(display);
>> +               wl_list_for_each_safe(client, next, &display->client_list, link) {
>> +                       if (client->error)
>> +                               wl_client_destroy(client);
>> +               }
> 
> wl_display_flush_clients() may be called manually by the user too,
> without using wl_display_run(). So now all the users would need to do
> the loop to destroy the clients, or else they would leak. Instead, you
> should do the loop at the end of wl_display_flush_clients().
> 
> 
> Thanks,
> Giulio
> 
>>                wl_event_loop_dispatch(display->loop, -1);
>>        }
>> }
>> @@ -1124,7 +1130,7 @@ wl_display_flush_clients(struct wl_display *display)
>>                                                  WL_EVENT_WRITABLE |
>>                                                  WL_EVENT_READABLE);
>>                } else if (ret < 0) {
>> -                       wl_client_destroy(client);
>> +                       client->error = 1;
>>                }
>>        }
>> }
>> --
>> 1.7.9.5
>> 
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160923/f9d57ccd/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Email_Signature_image_English.jpg
Type: image/jpeg
Size: 101522 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/wayland-devel/attachments/20160923/f9d57ccd/attachment-0001.jpg>