From 6a3f1f669f6e53c0db3057d4f62da3a473c07c71 Mon Sep 17 00:00:00 2001 From: Hideyuki Nagase Date: Mon, 3 Jan 2022 13:18:28 -0800 Subject: [PATCH] fix clipboard race condition ends up as assertion failure (#54) * add debug msg and asserts * cleanup must be done at compositor thread Co-authored-by: Hideyuki Nagase --- libweston/backend-rdp/rdpclip.c | 134 ++++++++++++++++++++------------ xwayland/selection.c | 31 ++++---- 2 files changed, 103 insertions(+), 62 deletions(-) diff --git a/libweston/backend-rdp/rdpclip.c b/libweston/backend-rdp/rdpclip.c index a772aa1b8..d21f2b391 100644 --- a/libweston/backend-rdp/rdpclip.c +++ b/libweston/backend-rdp/rdpclip.c @@ -148,7 +148,7 @@ clipboard_data_source_state_to_string(struct rdp_clipboard_data_source *source) case RDP_CLIPBOARD_SOURCE_CANCEL_PENDING: return "cancel pending"; case RDP_CLIPBOARD_SOURCE_CANCELED: - return "cenceled"; + return "canceled"; case RDP_CLIPBOARD_SOURCE_RETRY: return "retry"; case RDP_CLIPBOARD_SOURCE_FAILED: @@ -721,13 +721,18 @@ clipboard_data_source_unref(struct rdp_clipboard_data_source *source) wl_event_source_remove(source->defer_event_source); } - if (source->data_source_fd != -1) + if (source->data_source_fd != -1) { + ASSERT_COMPOSITOR_THREAD(b); close(source->data_source_fd); + } - wl_array_release(&source->data_contents); + if (!wl_list_empty(&source->base.destroy_signal.listener_list)) { + ASSERT_COMPOSITOR_THREAD(b); + wl_signal_emit(&source->base.destroy_signal, + &source->base); + } - wl_signal_emit(&source->base.destroy_signal, - &source->base); + wl_array_release(&source->data_contents); wl_array_for_each(p, &source->base.mime_types) free(*p); @@ -880,6 +885,60 @@ clipboard_data_source_read(int fd, uint32_t mask, void *arg) return 0; } +/* client's reply with error for data request, clean up */ +static int +clipboard_data_source_fail(int fd, uint32_t mask, void *arg) +{ + struct rdp_clipboard_data_source *source = (struct rdp_clipboard_data_source *)arg; + freerdp_peer *client = (freerdp_peer *) source->context; + RdpPeerContext *peerCtx = (RdpPeerContext *)client->context; + struct rdp_backend *b = peerCtx->rdpBackend; + + rdp_debug_clipboard_verbose(b, "RDP %s (%p:%s) fd:%d\n", __func__, + source, clipboard_data_source_state_to_string(source), fd); + + ASSERT_COMPOSITOR_THREAD(b); + + assert(source->data_source_fd == fd); + /* this data source must be tracked as inflight */ + assert(source == peerCtx->clipboard_inflight_client_data_source); + + /* remove event source now, and if write is failed with EAGAIN, queue back to display loop. */ + wl_event_source_remove(source->transfer_event_source); + source->transfer_event_source = NULL; + + if (source->data_contents.size) { + /* if data is recieved, but failed by other reason, + then keep data and format index for future request, + otherwise data is purged at last reference release. */ + /* wl_array_release(&source->data_contents); */ + /* wl_array_init(&source->data_contents); */ + } else { + /* data has been never recieved, thus must be empty. */ + assert(source->data_contents.size == 0); + assert(source->data_contents.alloc == 0); + assert(source->data_contents.data == NULL); + /* clear previous requested format so it can be requested later again. */ + source->format_index = -1; + } + /* don't clear format id table, so it allows to retry to get data from client. */ + /* memset(source->client_format_id_table, 0, sizeof(source->client_format_id_table)); */ + /* data has never been sent to write(), thus must be no inflight write. */ + assert(source->inflight_write_count == 0); + assert(source->inflight_data_to_write == NULL); + assert(source->inflight_data_size == 0); + /* data never has been sent to write(), so must not be processed. */ + assert(source->is_data_processed == FALSE); + /* close fd to server clipboard stop pulling data. */ + close(source->data_source_fd); + source->data_source_fd = -1; + /* clear inflight data source from client to server. */ + peerCtx->clipboard_inflight_client_data_source = NULL; + clipboard_data_source_unref(source); + + return 0; +} + /* Send client's clipboard data to the requesting application at server side */ static int clipboard_data_source_write(int fd, uint32_t mask, void *arg) @@ -1273,6 +1332,10 @@ clipboard_data_source_request(int fd, uint32_t mask, void *arg) source->data_source_fd = p[0]; + rdp_debug_clipboard_verbose(b, "RDP %s (%p:%s) pipe write:%d -> read:%d\n", + __func__, source, clipboard_data_source_state_to_string(source), + p[1], p[0]); + /* Request data from data source */ source->state = RDP_CLIPBOARD_SOURCE_REQUEST_DATA; selection_data_source->send(selection_data_source, requested_mime_type, p[1]); @@ -1505,7 +1568,7 @@ clipboard_client_format_list(CliprdrServerContext* context, const CLIPRDR_FORMAT CLIPRDR_FORMAT_LIST_RESPONSE formatListResponse = {}; formatListResponse.msgType = CB_FORMAT_LIST_RESPONSE; - formatListResponse.msgFlags = source ? CB_RESPONSE_OK : CB_RESPONSE_FAIL; + formatListResponse.msgFlags = (source && isPublished) ? CB_RESPONSE_OK : CB_RESPONSE_FAIL; formatListResponse.dataLen = 0; if (peerCtx->clipboard_server_context->ServerFormatListResponse(peerCtx->clipboard_server_context, &formatListResponse) != 0) { source->state = RDP_CLIPBOARD_SOURCE_FAILED; @@ -1514,8 +1577,10 @@ clipboard_client_format_list(CliprdrServerContext* context, const CLIPRDR_FORMAT return -1; } - if (!isPublished && source) + if (!isPublished && source) { + assert(source->refcount == 1); clipboard_data_source_unref(source); + } return 0; } @@ -1566,53 +1631,20 @@ clipboard_client_format_data_response(CliprdrServerContext* context, const CLIPR source->state = RDP_CLIPBOARD_SOURCE_FAILED; source->data_response_fail_count++; } - rdp_debug_clipboard_verbose(b, "Client: %s (%p:%s:%d)\n", + rdp_debug_clipboard_verbose(b, "Client: %s (%p:%s) fail count:%d)\n", __func__, source, clipboard_data_source_state_to_string(source), source->data_response_fail_count); - if (Success) { - assert(source->transfer_event_source == NULL); - source->transfer_event_source = - wl_event_loop_add_fd(loop, source->data_source_fd, WL_EVENT_WRITABLE, - clipboard_data_source_write, source); - if (!source->transfer_event_source) { - source->state = RDP_CLIPBOARD_SOURCE_FAILED; - rdp_debug_clipboard_error(b, "Client: %s (%p:%s) wl_event_loop_add_fd failed\n", - __func__, source, clipboard_data_source_state_to_string(source)); - } - } - + assert(source->transfer_event_source == NULL); + source->transfer_event_source = + wl_event_loop_add_fd(loop, source->data_source_fd, WL_EVENT_WRITABLE, + Success ? clipboard_data_source_write : clipboard_data_source_fail, source); if (!source->transfer_event_source) { - if (formatDataResponse->msgFlags == CB_RESPONSE_OK) { - /* if data is recieved, but failed to sent to write(), - then keep data and format index for future request, - otherwise data is purged at last reference release. */ - /* wl_array_release(&source->data_contents); */ - /* wl_array_init(&source->data_contents); */ - } else { - /* data has been never recieved, thus must be empty. */ - assert(source->data_contents.size == 0); - assert(source->data_contents.alloc == 0); - assert(source->data_contents.data == NULL); - /* clear previous requested format so it can be requested later again. */ - source->format_index = -1; - } - /* don't clear format id table, so it allows to retry to get data from client. */ - /* memset(source->client_format_id_table, 0, sizeof(source->client_format_id_table)); */ - /* data has never been sent to write(), thus must be no inflight write. */ - assert(source->inflight_write_count == 0); - assert(source->inflight_data_to_write == NULL); - assert(source->inflight_data_size == 0); - /* data never has been sent to write(), so must not be processed. */ - assert(source->is_data_processed == FALSE); - /* close fd to server clipboard stop pulling data. */ - close(source->data_source_fd); - source->data_source_fd = -1; - /* clear inflight data source from client to server. */ - assert(source == peerCtx->clipboard_inflight_client_data_source); - peerCtx->clipboard_inflight_client_data_source = NULL; - clipboard_data_source_unref(source); + source->state = RDP_CLIPBOARD_SOURCE_FAILED; + rdp_debug_clipboard_error(b, "Client: %s (%p:%s) wl_event_loop_add_fd failed\n", + __func__, source, clipboard_data_source_state_to_string(source)); + return -1; } } else { rdp_debug_clipboard(b, "Client: %s client send data without server asking. protocol error", __func__); @@ -1630,6 +1662,7 @@ clipboard_client_format_list_response(CliprdrServerContext* context, const CLIPR RdpPeerContext *peerCtx = (RdpPeerContext *)client->context; struct rdp_backend *b = peerCtx->rdpBackend; rdp_debug_clipboard(b, "Client: %s msgFlags:0x%x\n", __func__, formatListResponse->msgFlags); + ASSERT_NOT_COMPOSITOR_THREAD(b); return 0; } @@ -1734,6 +1767,9 @@ void rdp_clipboard_destroy(RdpPeerContext *peerCtx) { struct rdp_clipboard_data_source* data_source; + struct rdp_backend *b = peerCtx->rdpBackend; + + ASSERT_COMPOSITOR_THREAD(b); if (peerCtx->clipboard_selection_listener.notify) { wl_list_remove(&peerCtx->clipboard_selection_listener.link); diff --git a/xwayland/selection.c b/xwayland/selection.c index d40c0b47b..a99717894 100644 --- a/xwayland/selection.c +++ b/xwayland/selection.c @@ -60,13 +60,15 @@ writable_callback(int fd, uint32_t mask, void *data) if (wm->property_source) wl_event_source_remove(wm->property_source); wm->property_source = NULL; + weston_log("write error to target fd:%d start:%d reminder:%d %s\n", + fd, wm->property_start, remainder, strerror(errno)); close(fd); - weston_log("write error to target fd: %s\n", strerror(errno)); + wm->data_source_fd = -1; return 1; } - weston_log("wrote %d (chunk size %d) of %d bytes\n", - wm->property_start + len, + weston_log("wrote fd:%d %d (chunk size %d) of %d bytes\n", + fd, wm->property_start + len, len, xcb_get_property_value_length(wm->property_reply)); wm->property_start += len; @@ -82,8 +84,9 @@ writable_callback(int fd, uint32_t mask, void *data) wm->selection_window, wm->atom.wl_selection); } else { - weston_log("transfer complete\n"); + weston_log("transfer write complete\n"); close(fd); + wm->data_source_fd = -1; } } @@ -139,8 +142,9 @@ weston_wm_get_incr_chunk(struct weston_wm *wm) * for freeing it */ weston_wm_write_property(wm, reply); } else { - weston_log("transfer complete\n"); + weston_log("transfer write complete (zero size reply)\n"); close(wm->data_source_fd); + wm->data_source_fd = -1; free(reply); } } @@ -403,19 +407,20 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data) len = read(fd, p, available); if (len == -1) { - weston_log("read error from data source: %s\n", - strerror(errno)); + weston_log("read error from data source fd:%d %s\n", + fd, strerror(errno)); weston_wm_send_selection_notify(wm, XCB_ATOM_NONE); if (wm->property_source) wl_event_source_remove(wm->property_source); wm->property_source = NULL; close(fd); + wm->data_source_fd = -1; wl_array_release(&wm->source_data); return 1; } - weston_log("read %d (available %d, mask 0x%x)\n", - len, available, mask); + weston_log("read fd:%d %d (available %d, mask 0x%x)\n", + fd, len, available, mask); /*remove actual data due to privacy*/ /*weston_log("read %d (available %d, mask 0x%x) bytes: \"%.*s\"\n", len, available, mask, len, (char *) p);*/ @@ -454,7 +459,7 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data) weston_wm_flush_source_data(wm); } } else if (len == 0 && !wm->incr) { - weston_log("non-incr transfer complete\n"); + weston_log("non-incr transfer read complete\n"); /* Non-incr transfer all done. */ weston_wm_flush_source_data(wm); weston_wm_send_selection_notify(wm, wm->selection_request.property); @@ -463,10 +468,11 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data) wl_event_source_remove(wm->property_source); wm->property_source = NULL; close(fd); + wm->data_source_fd = -1; wl_array_release(&wm->source_data); wm->selection_request.requestor = XCB_NONE; } else if (len == 0 && wm->incr) { - weston_log("incr transfer complete\n"); + weston_log("incr transfer read complete\n"); wm->flush_property_on_delete = 1; if (wm->selection_property_set) { @@ -482,9 +488,8 @@ weston_wm_read_data_source(int fd, uint32_t mask, void *data) if (wm->property_source) wl_event_source_remove(wm->property_source); wm->property_source = NULL; - close(wm->data_source_fd); - wm->data_source_fd = -1; close(fd); + wm->data_source_fd = -1; } else { weston_log("nothing happened, buffered the bytes\n"); }