Skip to content

Commit

Permalink
xwayland: fix use-after-free in selection handling
Browse files Browse the repository at this point in the history
Fixes swaywm#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:

| swaywm#1: started | swaywm#2: pending | swaywm#3: pending | swaywm#4: pending |

The file descriptor for transfer swaywm#1 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 swaywm#4 first.

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

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

However, we still have 3 copies of swaywm#1's file descriptor left in the
epoll loop, since we erroneously added them as part of removing swaywm#2/3/4.
When we `close` the file descriptor as part of swaywm#1'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.
  • Loading branch information
Xyene committed Oct 11, 2020
1 parent 8ad2cc3 commit ac5a9be
Showing 1 changed file with 20 additions and 7 deletions.
27 changes: 20 additions & 7 deletions xwayland/selection/outgoing.c
Original file line number Diff line number Diff line change
Expand Up @@ -55,16 +55,27 @@ static int xwm_selection_flush_source_data(
static void xwm_selection_transfer_start_outgoing(
struct wlr_xwm_selection_transfer *transfer);

static struct wlr_xwm_selection_transfer *xwm_selection_get_first_transfer(
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(
struct wlr_xwm_selection_transfer *transfer) {
struct wlr_xwm_selection *selection = transfer->selection;
bool was_first = transfer == xwm_selection_get_first_transfer(selection);
wl_list_remove(&transfer->outgoing_link);

// Start next queued transfer
struct wlr_xwm_selection_transfer *first = NULL;
if (!wl_list_empty(&transfer->selection->outgoing)) {
first = wl_container_of(transfer->selection->outgoing.prev, first,
outgoing_link);
xwm_selection_transfer_start_outgoing(first);
// Start next queued transfer if we just removed the active one.
if (was_first && !wl_list_empty(&selection->outgoing)) {
xwm_selection_transfer_start_outgoing(
xwm_selection_get_first_transfer(selection));
}

xwm_selection_transfer_remove_source(transfer);
Expand Down Expand Up @@ -307,8 +318,10 @@ static void xwm_selection_send_data(struct wlr_xwm_selection *selection,
wl_list_insert(&selection->outgoing, &transfer->outgoing_link);

// We can only handle one transfer at a time
struct wlr_xwm_selection_transfer *first;
if (wl_list_length(&selection->outgoing) == 1) {
xwm_selection_transfer_start_outgoing(transfer);
first = wl_container_of(selection->outgoing.prev, first, outgoing_link);
xwm_selection_transfer_start_outgoing(first);
}
}

Expand Down

0 comments on commit ac5a9be

Please sign in to comment.