From 7964a313e8c59ed664235884d9b612273b12fc57 Mon Sep 17 00:00:00 2001 From: Tudor Brindus Date: Sat, 30 Jan 2021 23:43:25 -0500 Subject: [PATCH] xwayland/selection: use one X11 window per incoming transfer This commit introduces logic for using a new X11 window for each incoming transfer, rather than having a global window for each selection source. This eliminates a whole class of bugs involving multiple concurrent incoming transfers. For now, we retain the outgoing transfer queue, and the selection source-specific windows to support it. Source-specific windows are no longer used in the incoming path, and will be removed in a future PR. Refs #1497. --- include/xwayland/selection.h | 18 +++-- include/xwayland/xwm.h | 2 + xwayland/selection/incoming.c | 144 ++++++++++++++++++++++++--------- xwayland/selection/outgoing.c | 16 ++-- xwayland/selection/selection.c | 69 ++++++++-------- 5 files changed, 158 insertions(+), 91 deletions(-) diff --git a/include/xwayland/selection.h b/include/xwayland/selection.h index 1d8d3ec4..07659cfd 100644 --- a/include/xwayland/selection.h +++ b/include/xwayland/selection.h @@ -20,37 +20,43 @@ struct wlr_xwm_selection_transfer { struct wl_array source_data; int wl_client_fd; struct wl_event_source *event_source; + struct wl_list link; // when sending to x11 xcb_selection_request_event_t request; - struct wl_list outgoing_link; // when receiving from x11 int property_start; xcb_get_property_reply_t *property_reply; + xcb_window_t incoming_window; }; struct wlr_xwm_selection { struct wlr_xwm *xwm; + xcb_atom_t atom; xcb_window_t window; xcb_window_t owner; xcb_timestamp_t timestamp; - struct wlr_xwm_selection_transfer incoming; + struct wl_list incoming; struct wl_list outgoing; }; +struct wlr_xwm_selection_transfer * +xwm_selection_find_incoming_transfer_by_window( + struct wlr_xwm_selection *selection, xcb_window_t window); + void xwm_selection_transfer_remove_event_source( struct wlr_xwm_selection_transfer *transfer); void xwm_selection_transfer_close_wl_client_fd( struct wlr_xwm_selection_transfer *transfer); void xwm_selection_transfer_destroy_property_reply( struct wlr_xwm_selection_transfer *transfer); -void xwm_selection_transfer_init(struct wlr_xwm_selection_transfer *transfer); -void xwm_selection_transfer_finish(struct wlr_xwm_selection_transfer *transfer); -bool xwm_selection_transfer_get_selection_property( - struct wlr_xwm_selection_transfer *transfer, bool delete); +void xwm_selection_transfer_init(struct wlr_xwm_selection_transfer *transfer, + struct wlr_xwm_selection *selection); +void xwm_selection_transfer_destroy( + struct wlr_xwm_selection_transfer *transfer); void xwm_selection_transfer_destroy_outgoing( struct wlr_xwm_selection_transfer *transfer); diff --git a/include/xwayland/xwm.h b/include/xwayland/xwm.h index 27b79b30..f467e107 100644 --- a/include/xwayland/xwm.h +++ b/include/xwayland/xwm.h @@ -103,6 +103,8 @@ struct wlr_xwm { xcb_render_pictformat_t render_format_id; xcb_cursor_t cursor; + // FIXME: need one per selection to simultaneously request both mimetypes, + // I think. xcb_window_t selection_window; struct wlr_xwm_selection clipboard_selection; struct wlr_xwm_selection primary_selection; diff --git a/xwayland/selection/incoming.c b/xwayland/selection/incoming.c index 183d5f88..304ba8e3 100644 --- a/xwayland/selection/incoming.c +++ b/xwayland/selection/incoming.c @@ -11,13 +11,84 @@ #include "xwayland/selection.h" #include "xwayland/xwm.h" +static struct wlr_xwm_selection_transfer * +xwm_selection_transfer_create_incoming(struct wlr_xwm_selection *selection) { + struct wlr_xwm_selection_transfer *transfer = calloc(1, sizeof(*transfer)); + if (!transfer) { + return NULL; + } + + xwm_selection_transfer_init(transfer, selection); + + wl_list_insert(&selection->incoming, &transfer->link); + + struct wlr_xwm *xwm = selection->xwm; + transfer->incoming_window = xcb_generate_id(xwm->xcb_conn); + xcb_create_window( + xwm->xcb_conn, + XCB_COPY_FROM_PARENT, + transfer->incoming_window, + xwm->screen->root, + 0, 0, + 10, 10, + 0, + XCB_WINDOW_CLASS_INPUT_OUTPUT, + xwm->screen->root_visual, + XCB_CW_EVENT_MASK, (uint32_t[]){ + XCB_EVENT_MASK_SUBSTRUCTURE_NOTIFY | XCB_EVENT_MASK_PROPERTY_CHANGE + } + ); + xcb_flush(xwm->xcb_conn); + + return transfer; +} + +struct wlr_xwm_selection_transfer * +xwm_selection_find_incoming_transfer_by_window( + struct wlr_xwm_selection *selection, xcb_window_t window) { + struct wlr_xwm_selection_transfer *transfer; + wl_list_for_each(transfer, &selection->incoming, link) { + if (transfer->incoming_window == window) { + return transfer; + } + } + + return NULL; +} + +static bool xwm_selection_transfer_get_incoming_selection_property( + struct wlr_xwm_selection_transfer *transfer, bool delete) { + struct wlr_xwm *xwm = transfer->selection->xwm; + + xcb_get_property_cookie_t cookie = xcb_get_property( + xwm->xcb_conn, + delete, + transfer->incoming_window, + xwm->atoms[WL_SELECTION], + XCB_GET_PROPERTY_TYPE_ANY, + 0, // offset + 0x1fffffff // length + ); + + transfer->property_start = 0; + transfer->property_reply = + xcb_get_property_reply(xwm->xcb_conn, cookie, NULL); + + if (!transfer->property_reply) { + wlr_log(WLR_ERROR, "cannot get selection property"); + return false; + } + + return true; +} + static void xwm_notify_ready_for_next_incr_chunk( struct wlr_xwm_selection_transfer *transfer) { struct wlr_xwm *xwm = transfer->selection->xwm; assert(transfer->incr); wlr_log(WLR_DEBUG, "deleting property"); - xcb_delete_property(xwm->xcb_conn, transfer->selection->window, + xcb_delete_property(xwm->xcb_conn, transfer->incoming_window, xwm->atoms[WL_SELECTION]); xcb_flush(xwm->xcb_conn); @@ -40,7 +111,7 @@ static int write_selection_property_to_wl_client(int fd, uint32_t mask, ssize_t len = write(fd, property + transfer->property_start, remainder); if (len == -1) { wlr_log_errno(WLR_ERROR, "write error to target fd %d", fd); - xwm_selection_transfer_finish(transfer); + xwm_selection_transfer_destroy(transfer); return 0; } @@ -56,7 +127,7 @@ static int write_selection_property_to_wl_client(int fd, uint32_t mask, xwm_notify_ready_for_next_incr_chunk(transfer); } else { wlr_log(WLR_DEBUG, "transfer complete"); - xwm_selection_transfer_finish(transfer); + xwm_selection_transfer_destroy(transfer); } return 0; @@ -93,7 +164,7 @@ void xwm_get_incr_chunk(struct wlr_xwm_selection_transfer *transfer) { return; } - if (!xwm_selection_transfer_get_selection_property(transfer, false)) { + if (!xwm_selection_transfer_get_incoming_selection_property(transfer, false)) { return; } @@ -101,15 +172,15 @@ void xwm_get_incr_chunk(struct wlr_xwm_selection_transfer *transfer) { xwm_write_selection_property_to_wl_client(transfer); } else { wlr_log(WLR_DEBUG, "incremental transfer complete"); - xwm_selection_transfer_finish(transfer); + xwm_selection_transfer_destroy(transfer); } } -static void xwm_selection_get_data(struct wlr_xwm_selection *selection) { - struct wlr_xwm *xwm = selection->xwm; - struct wlr_xwm_selection_transfer *transfer = &selection->incoming; +static void xwm_selection_transfer_get_data( + struct wlr_xwm_selection_transfer *transfer) { + struct wlr_xwm *xwm = transfer->selection->xwm; - if (!xwm_selection_transfer_get_selection_property(transfer, true)) { + if (!xwm_selection_transfer_get_incoming_selection_property(transfer, true)) { return; } @@ -127,7 +198,6 @@ static void source_send(struct wlr_xwm_selection *selection, struct wl_array *mime_types, struct wl_array *mime_types_atoms, const char *requested_mime_type, int fd) { struct wlr_xwm *xwm = selection->xwm; - struct wlr_xwm_selection_transfer *transfer = &selection->incoming; xcb_atom_t *atoms = mime_types_atoms->data; bool found = false; @@ -150,23 +220,16 @@ static void source_send(struct wlr_xwm_selection *selection, return; } - // FIXME: we currently can't handle two X11-to-Wayland transfers at once due - // to reusing the same X11 window. Proceeding further here would lead us to - // lose track of the current `transfer->wl_client_fd` and use-after-free - // during cleanup. This doesn't happen often, but bail now to avoid a - // compositor crash later. - if (transfer->wl_client_fd >= 0) { - wlr_log(WLR_ERROR, "source_send fd %d, but %d already in progress", fd, - transfer->wl_client_fd); - if (transfer->wl_client_fd != fd) { - close(fd); - } - + struct wlr_xwm_selection_transfer *transfer = + xwm_selection_transfer_create_incoming(selection); + if (!transfer) { + wlr_log(WLR_ERROR, "Cannot create transfer"); + close(fd); return; } xcb_convert_selection(xwm->xcb_conn, - selection->window, + transfer->incoming_window, selection->atom, mime_type_atom, xwm->atoms[WL_SELECTION], @@ -269,7 +332,7 @@ static bool source_get_targets(struct wlr_xwm_selection *selection, XCB_GET_PROPERTY_TYPE_ANY, 0, // offset 4096 // length - ); + ); xcb_get_property_reply_t *reply = xcb_get_property_reply(xwm->xcb_conn, cookie, NULL); @@ -395,23 +458,26 @@ void xwm_handle_selection_notify(struct wlr_xwm *xwm, return; } + struct wlr_xwm_selection_transfer *transfer = + xwm_selection_find_incoming_transfer_by_window(selection, event->requestor); + if (event->property == XCB_ATOM_NONE) { - wlr_log(WLR_ERROR, "convert selection failed"); - xwm_selection_transfer_finish(&selection->incoming); + if (transfer) { + wlr_log(WLR_ERROR, "convert selection failed"); + xwm_selection_transfer_destroy(transfer); + } } else if (event->target == xwm->atoms[TARGETS]) { // No xwayland surface focused, deny access to clipboard if (xwm->focus_surface == NULL) { wlr_log(WLR_DEBUG, "denying write access to clipboard: " "no xwayland surface focused"); - // Would leak this transfer otherwise. Should never happen. - assert(selection->incoming.wl_client_fd < 0); return; } // This sets the Wayland clipboard (by calling wlr_seat_set_selection) xwm_selection_get_targets(selection); - } else { - xwm_selection_get_data(selection); + } else if (transfer) { + xwm_selection_transfer_get_data(transfer); } } @@ -428,8 +494,7 @@ int xwm_handle_xfixes_selection_notify(struct wlr_xwm *xwm, if (event->owner == XCB_WINDOW_NONE) { if (selection->owner != selection->window) { - // A real X client selection went away, not our - // proxy selection + // A real X client selection went away, not our proxy selection if (selection == &xwm->clipboard_selection) { wlr_seat_request_set_selection(xwm->seat, NULL, NULL, wl_display_next_serial(xwm->xwayland->wl_display)); @@ -450,22 +515,23 @@ int xwm_handle_xfixes_selection_notify(struct wlr_xwm *xwm, selection->owner = event->owner; - // We have to use XCB_TIME_CURRENT_TIME when we claim the - // selection, so grab the actual timestamp here so we can - // answer TIMESTAMP conversion requests correctly. if (event->owner == selection->window) { + // We have to use XCB_TIME_CURRENT_TIME when we claim the selection, so + // grab the actual timestamp here so we can answer TIMESTAMP conversion + // requests correctly. selection->timestamp = event->timestamp; return 1; } - struct wlr_xwm_selection_transfer *transfer = &selection->incoming; - transfer->incr = false; // doing this will give a selection notify where we actually handle the sync - xcb_convert_selection(xwm->xcb_conn, selection->window, + xcb_convert_selection( + xwm->xcb_conn, + selection->window, selection->atom, xwm->atoms[TARGETS], xwm->atoms[WL_SELECTION], - event->timestamp); + event->timestamp + ); xcb_flush(xwm->xcb_conn); return 1; diff --git a/xwayland/selection/outgoing.c b/xwayland/selection/outgoing.c index 8074e3a7..787298d4 100644 --- a/xwayland/selection/outgoing.c +++ b/xwayland/selection/outgoing.c @@ -59,8 +59,7 @@ 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); + first = wl_container_of(selection->outgoing.prev, first, link); } return first; @@ -70,7 +69,7 @@ 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_transfer_get_first(selection); - wl_list_remove(&transfer->outgoing_link); + wl_list_remove(&transfer->link); wlr_log(WLR_DEBUG, "Destroying transfer %p", transfer); // Start next queued transfer if we just removed the active one. @@ -297,8 +296,7 @@ static bool xwm_selection_send_data(struct wlr_xwm_selection *selection, return false; } - xwm_selection_transfer_init(transfer); - transfer->selection = selection; + xwm_selection_transfer_init(transfer, selection); transfer->request = *req; wl_array_init(&transfer->source_data); @@ -325,7 +323,7 @@ static bool xwm_selection_send_data(struct wlr_xwm_selection *selection, // from it. It appears to only ever read from the latest, so purge stale // transfers to prevent clipboard hangs. struct wlr_xwm_selection_transfer *outgoing, *tmp; - wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, outgoing_link) { + wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, link) { if (transfer->request.requestor == outgoing->request.requestor) { wlr_log(WLR_DEBUG, "Destroying stale transfer %p", outgoing); xwm_selection_send_notify(selection->xwm, &outgoing->request, false); @@ -333,7 +331,7 @@ static bool xwm_selection_send_data(struct wlr_xwm_selection *selection, } } - wl_list_insert(&selection->outgoing, &transfer->outgoing_link); + wl_list_insert(&selection->outgoing, &transfer->link); // We can only handle one transfer at a time if (wl_list_length(&selection->outgoing) == 1) { @@ -341,7 +339,7 @@ static bool xwm_selection_send_data(struct wlr_xwm_selection *selection, xwm_selection_transfer_start_outgoing(transfer); } else { struct wlr_xwm_selection_transfer *outgoing; - wl_list_for_each(outgoing, &selection->outgoing, outgoing_link) { + wl_list_for_each(outgoing, &selection->outgoing, link) { wlr_log(WLR_DEBUG, "Transfer %p still queued", outgoing); } } @@ -477,7 +475,7 @@ void xwm_handle_selection_destroy_notify(struct wlr_xwm *xwm, struct wlr_xwm_selection *selection = selections[i]; struct wlr_xwm_selection_transfer *outgoing, *tmp; - wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, outgoing_link) { + wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, link) { if (event->window == outgoing->request.requestor) { xwm_selection_transfer_destroy_outgoing(outgoing); } diff --git a/xwayland/selection/selection.c b/xwayland/selection/selection.c index c526fd2a..0996f94d 100644 --- a/xwayland/selection/selection.c +++ b/xwayland/selection/selection.c @@ -32,42 +32,30 @@ void xwm_selection_transfer_destroy_property_reply( transfer->property_reply = NULL; } -void xwm_selection_transfer_init(struct wlr_xwm_selection_transfer *transfer) { +void xwm_selection_transfer_init(struct wlr_xwm_selection_transfer *transfer, + struct wlr_xwm_selection *selection) { + transfer->selection = selection; transfer->wl_client_fd = -1; } -void xwm_selection_transfer_finish( +void xwm_selection_transfer_destroy( struct wlr_xwm_selection_transfer *transfer) { - transfer->incr = false; + if (!transfer) { + return; + } + xwm_selection_transfer_destroy_property_reply(transfer); xwm_selection_transfer_remove_event_source(transfer); xwm_selection_transfer_close_wl_client_fd(transfer); -} -bool xwm_selection_transfer_get_selection_property( - struct wlr_xwm_selection_transfer *transfer, bool delete) { - struct wlr_xwm *xwm = transfer->selection->xwm; - - xcb_get_property_cookie_t cookie = xcb_get_property( - xwm->xcb_conn, - delete, - transfer->selection->window, - xwm->atoms[WL_SELECTION], - XCB_GET_PROPERTY_TYPE_ANY, - 0, // offset - 0x1fffffff // length - ); - - transfer->property_start = 0; - transfer->property_reply = - xcb_get_property_reply(xwm->xcb_conn, cookie, NULL); - - if (!transfer->property_reply) { - wlr_log(WLR_ERROR, "cannot get selection property"); - return false; + if (transfer->incoming_window) { + struct wlr_xwm *xwm = transfer->selection->xwm; + xcb_destroy_window(xwm->xcb_conn, transfer->incoming_window); + xcb_flush(xwm->xcb_conn); } - return true; + wl_list_remove(&transfer->link); + free(transfer); } xcb_atom_t xwm_mime_type_to_atom(struct wlr_xwm *xwm, char *mime_type) { @@ -123,17 +111,22 @@ static int xwm_handle_selection_property_notify(struct wlr_xwm *xwm, for (size_t i = 0; i < sizeof(selections)/sizeof(selections[0]); ++i) { struct wlr_xwm_selection *selection = selections[i]; - if (event->window == selection->window) { - if (event->state == XCB_PROPERTY_NEW_VALUE && - event->atom == xwm->atoms[WL_SELECTION] && - selection->incoming.incr) { - xwm_get_incr_chunk(&selection->incoming); + if (event->state == XCB_PROPERTY_NEW_VALUE && + event->atom == xwm->atoms[WL_SELECTION]) { + struct wlr_xwm_selection_transfer *transfer = + xwm_selection_find_incoming_transfer_by_window(selection, + event->window); + if (transfer) { + if (transfer->incr) { + xwm_get_incr_chunk(transfer); + } + + return 1; } - return 1; } struct wlr_xwm_selection_transfer *outgoing; - wl_list_for_each(outgoing, &selection->outgoing, outgoing_link) { + wl_list_for_each(outgoing, &selection->outgoing, link) { if (event->window == outgoing->request.requestor) { if (event->state == XCB_PROPERTY_DELETE && event->atom == outgoing->request.property && @@ -184,9 +177,8 @@ void xwm_selection_init(struct wlr_xwm_selection *selection, selection->xwm = xwm; selection->atom = atom; selection->window = xwm->selection_window; - selection->incoming.selection = selection; + wl_list_init(&selection->incoming); wl_list_init(&selection->outgoing); - xwm_selection_transfer_init(&selection->incoming); uint32_t mask = XCB_XFIXES_SELECTION_EVENT_MASK_SET_SELECTION_OWNER | @@ -202,12 +194,15 @@ void xwm_selection_finish(struct wlr_xwm_selection *selection) { } struct wlr_xwm_selection_transfer *outgoing, *tmp; - wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, outgoing_link) { + wl_list_for_each_safe(outgoing, tmp, &selection->outgoing, link) { wlr_log(WLR_INFO, "destroyed pending transfer %p", outgoing); xwm_selection_transfer_destroy_outgoing(outgoing); } - xwm_selection_transfer_finish(&selection->incoming); + struct wlr_xwm_selection_transfer *incoming; + wl_list_for_each_safe(incoming, tmp, &selection->incoming, link) { + xwm_selection_transfer_destroy(incoming); + } } static void xwm_selection_set_owner(struct wlr_xwm_selection *selection,