nordic-dev.net/wlroots
2
0
Fork 0
mirror of https://gitlab.freedesktop.org/wlroots/wlroots.git synced 2024-10-30 15:40:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

xwayland: fix use-after-free in selection handling

Browse Source 
Fixes #2425.

wlroots can only handle one outgoing transfer at a time, so it keeps a
list of pending selections. The head of the list is the currently-active
selection, and when that transfer completes and is destroyed, the next
one is started.

The trouble is when you have a transfer to some app that is misbehaving.
fcitx is one such application. With really large transfers, fcitx will
hang and never wake up again. So, you can end up with a transfer list
that looks like this:

| T1: started | T2: pending | T3: pending | T4: pending |

The file descriptor for transfer T1 is registered in libwayland's epoll
loop. The rest are waiting in wlroots' list.

As a user, you want your clipboard back, so you `pkill fcitx`. Now
Xwayland sends `XCB_DESTROY_NOTIFY` to let us know to give up. We clean
up T4 first.

Due to a bug in wlroots code, we register the (fd, transfer data
pointer) pair for T1 with libwayland *again*, despite it already being
registered. We do this 2 more times as we remove T3 and T2.

Finally, we remove T1 and `free` all the memory associated with it,
before `close`-ing its transfer file descriptor.

However, we still have 3 copies of T1's file descriptor left in the
epoll loop, since we erroneously added them as part of removing T2/3/4.
When we `close` the file descriptor as part of T1's teardown, we
actually cause the epoll loop to wake up the next time around, saying
"this file descriptor has activity!" (it was closed, so `read`-ing would
normally return 0 to let us know of EOF).

But instead of returning 0, it returns -1 with `EBADF`, because the file
descriptor has already been closed. And finally, as part of error-handling
this, we access the transfer pointer, which was `free`'d. And we crash.
This commit is contained in:
Tudor Brindus 2020-10-10 22:51:50 -04:00 committed by Simon Ser
parent ab80ad902e
commit feb0e1c74d
1 changed files with 17 additions and 6 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
xwayland/selection/outgoing.c
View File

@ -55,16 +55,27 @@ static int xwm_selection_flush_source_data(
static void xwm_selection_transfer_start_outgoing( static void xwm_selection_transfer_start_outgoing(
struct wlr_xwm_selection_transfer *transfer); struct wlr_xwm_selection_transfer *transfer);
static struct wlr_xwm_selection_transfer *xwm_selection_transfer_get_first(
struct wlr_xwm_selection *selection) {
struct wlr_xwm_selection_transfer *first = NULL;
if (!wl_list_empty(&selection->outgoing)) {
first = wl_container_of(selection->outgoing.prev, first,
outgoing_link);
}
return first;
}
static void xwm_selection_transfer_destroy_outgoing( static void xwm_selection_transfer_destroy_outgoing(
struct wlr_xwm_selection_transfer *transfer) { struct wlr_xwm_selection_transfer *transfer) {
struct wlr_xwm_selection *selection = transfer->selection;
bool was_first = transfer == xwm_selection_transfer_get_first(selection);
wl_list_remove(&transfer->outgoing_link); wl_list_remove(&transfer->outgoing_link);
// Start next queued transfer // Start next queued transfer if we just removed the active one.
struct wlr_xwm_selection_transfer *first = NULL; if (was_first && !wl_list_empty(&selection->outgoing)) {
if (!wl_list_empty(&transfer->selection->outgoing)) { xwm_selection_transfer_start_outgoing(
first = wl_container_of(transfer->selection->outgoing.prev, first, xwm_selection_transfer_get_first(selection));
outgoing_link);
xwm_selection_transfer_start_outgoing(first);
} }
xwm_selection_transfer_remove_source(transfer); xwm_selection_transfer_remove_source(transfer);