[PATCH weston] xwm: fix use after free

[Bryce Harrington]

On Thu, Jan 29, 2015 at 07:17:31PM +0200, Giulio Camuffo wrote:
> 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.

Alright, thanks for the explanation.  Applied:

   e82ba53..9e1aeb8  master -> master


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