[PATCH weston] xwm: fix use after free

[Giulio Camuffo]

2015-01-28 5:29 GMT+02:00 Bryce Harrington <bryce at osg.samsung.com>:
> On Fri, Dec 26, 2014 at 06:10:35PM +0200, Giulio Camuffo wrote:
>> Calling wl_event_source_remove() will free the event source later, so
>> reset the pointer to avoid calling it two times on the same pointer.
>> Fix a compositor crash when copying some text from weston terminal,
>> pasting it in the same terminal and hitting ctrl-u, while a X client
>> is running.
>> ---
>>  xwayland/selection.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>
> Looking at wl_event_source_remove(), I definitely see what you mean
> about it delaying the free's.
>
> Verifying the pointer does eventually get freed is a bit tricky but I
> think I understand it: wl_event_source_remove() adds source->link to the
> loop->destroy_list, then later on this list is processed by a C macro:
>
>         wl_list_for_each_safe(source, next, &loop->destroy_list, link)
>                 free(source);
>
> This uses wl_container_of() on the link stored in destroy_list to find
> its parent container, source, and then frees it.
>
> So, presuming my understanding of the logic is correct:

That is my understanding as well.

>
> Reviewed-by: Bryce Harrington <bryce at osg.samsung.com>
>
>> diff --git a/xwayland/selection.c b/xwayland/selection.c
>> index b694477..24bd56d 100644
>> --- a/xwayland/selection.c
>> +++ b/xwayland/selection.c
>> @@ -46,6 +46,7 @@ writable_callback(int fd, uint32_t mask, void *data)
>>               wm->property_reply = NULL;
>>               if (wm->property_source)
>>                       wl_event_source_remove(wm->property_source);
>> +             wm->property_source = NULL;
>>               close(fd);
>>               weston_log("write error to target fd: %m\n");
>>               return 1;
>> @@ -61,6 +62,7 @@ writable_callback(int fd, uint32_t mask, void *data)
>>               wm->property_reply = NULL;
>>               if (wm->property_source)
>>                       wl_event_source_remove(wm->property_source);
>
> If we need to check wm->property_source before calling
> wl_event_source_remove(), shouldn't this also be done for the subsequent
> calls as well?

No, because the calls below are all inside
weston_wm_read_data_source(), which is the handler of
wm->property_source. So inside there we know that the source is valid
and we can freely destroy it without checking. But in the function
above we cannot do that assumption.


--
Giulio

>
>> +             wm->property_source = NULL;
>>
>>               if (wm->incr) {
>>                       xcb_delete_property(wm->conn,
>> @@ -352,6 +354,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>>               weston_log("read error from data source: %m\n");
>>               weston_wm_send_selection_notify(wm, XCB_ATOM_NONE);
>>               wl_event_source_remove(wm->property_source);
>> +             wm->property_source = NULL;
>>               close(fd);
>>               wl_array_release(&wm->source_data);
>>       }
>> @@ -375,6 +378,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>>                       wm->selection_property_set = 1;
>>                       wm->flush_property_on_delete = 1;
>>                       wl_event_source_remove(wm->property_source);
>> +                     wm->property_source = NULL;
>>                       weston_wm_send_selection_notify(wm, wm->selection_request.property);
>>               } else if (wm->selection_property_set) {
>>                       weston_log("got %zu bytes, waiting for "
>> @@ -382,6 +386,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>>
>>                       wm->flush_property_on_delete = 1;
>>                       wl_event_source_remove(wm->property_source);
>> +                     wm->property_source = NULL;
>>               } else {
>>                       weston_log("got %zu bytes, "
>>                               "property deleted, seting new property\n",
>> @@ -395,6 +400,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>>               weston_wm_send_selection_notify(wm, wm->selection_request.property);
>>               xcb_flush(wm->conn);
>>               wl_event_source_remove(wm->property_source);
>> +             wm->property_source = NULL;
>>               close(fd);
>>               wl_array_release(&wm->source_data);
>>               wm->selection_request.requestor = XCB_NONE;
>> @@ -413,6 +419,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data)
>>               }
>>               xcb_flush(wm->conn);
>>               wl_event_source_remove(wm->property_source);
>> +             wm->property_source = NULL;
>>               close(wm->data_source_fd);
>>               wm->data_source_fd = -1;
>>               close(fd);
>> --
>> 2.2.1
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel