From 55ae16739486672c9cfbd0c8c92b60a6081e4de5 Mon Sep 17 00:00:00 2001 From: rustdesk Date: Sat, 4 Jul 2026 18:23:58 +0800 Subject: [PATCH] harden the format-data path against late/duplicate responses The format-data rendezvous had the same single-slot race the file-contents path just fixed: the channel thread rewrote clipboard->hmem with no lock while explorer-thread consumers read/freed it, nothing serialized concurrent requests, and no flag told an expected response from a stray one. - Add format_request_mutex (serializes the whole request/response cycle) and hmem_mutex (guards the hmem hand-off and formatDataRespExpected). - cliprdr_send_data_request now takes ownership of the response buffer under hmem_mutex and returns it to the caller, so a later response cannot touch a buffer a consumer is using. All three consumers (GetData, WM_RENDERFORMAT, DELAYED_RENDERING) and the WM_CLIPBOARDUPDATE cleanup use the returned/taken handle instead of the shared slot. - The response handler drops any response arriving while formatDataRespExpected is clear (late/duplicate/unsolicited), consumes the flag on the first response, and no longer dereferences a NULL clipboard in the SetEvent path. Pre-existing issue, not introduced by this branch; generalizes the file-contents hardening to the format-data path. Co-Authored-By: Claude Fable 5 --- libs/clipboard/src/windows/wf_cliprdr.c | 229 ++++++++++++++++++------ tests/test_invariant_wf_cliprdr.c | 29 +++ 2 files changed, 200 insertions(+), 58 deletions(-) diff --git a/libs/clipboard/src/windows/wf_cliprdr.c b/libs/clipboard/src/windows/wf_cliprdr.c index b731f8899..59fd8e985 100644 --- a/libs/clipboard/src/windows/wf_cliprdr.c +++ b/libs/clipboard/src/windows/wf_cliprdr.c @@ -283,6 +283,14 @@ struct wf_clipboard HANDLE thread; HANDLE formatDataRespEvent; BOOL formatDataRespReceived; + // Format-data rendezvous, mirroring the req_f* fields. format_request_mutex + // serializes whole request/response cycles; hmem_mutex guards the hmem hand-off + // and formatDataRespExpected, which lets the response handler drop a late, + // duplicate or unsolicited response instead of overwriting the slot a consumer + // is about to take. + BOOL formatDataRespExpected; + HANDLE hmem_mutex; + HANDLE format_request_mutex; LPDATAOBJECT data_obj; HANDLE data_obj_mutex; @@ -334,7 +342,8 @@ BOOL wf_do_empty_cliprdr(wfClipboard *clipboard); static BOOL wf_create_file_obj(UINT32 *connID, wfClipboard *clipboard, IDataObject **ppDataObject); static void wf_destroy_file_obj(IDataObject *instance); static UINT32 get_remote_format_id(wfClipboard *clipboard, UINT32 local_format); -static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UINT32 format); +static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UINT32 formatId, + HANDLE *outHmem, SIZE_T *outLen); static UINT cliprdr_send_lock(wfClipboard *clipboard); static UINT cliprdr_send_unlock(wfClipboard *clipboard); static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, UINT32 streamId, @@ -443,6 +452,23 @@ static void take_req_fdata(wfClipboard *clipboard, char **data, ULONG *size) ReleaseMutex(clipboard->req_f_mutex); } +// Take ownership of the pending format-data buffer under hmem_mutex. The caller owns +// the returned HGLOBAL and must GlobalFree it (or hand it to the clipboard / OLE). +static void take_hmem(wfClipboard *clipboard, HANDLE *hmem, SIZE_T *len) +{ + if (!lock_mutex(clipboard->hmem_mutex, INFINITE)) + { + *hmem = NULL; + *len = 0; + return; + } + *hmem = clipboard->hmem; + *len = clipboard->hmem_data_len; + clipboard->hmem = NULL; + clipboard->hmem_data_len = 0; + ReleaseMutex(clipboard->hmem_mutex); +} + // Serialize whole file-contents request/response cycles (request, wait, take) across // streams, enforcing the one-outstanding-request invariant of the req_f* slot. The // wait is bounded so a wedged request degrades into a failed read instead of blocking @@ -830,18 +856,16 @@ static void wf_cliprdr_reset_streams(CliprdrDataObject *instance) instance->m_nStreams = 0; } -/* Only call after clipboard->hmem has been locked by GlobalLock. */ -static HRESULT wf_cliprdr_fail_locked_file_descriptor_data(wfClipboard *clipboard, +/* Only call after hMem has been locked by GlobalLock; takes ownership and frees it. */ +static HRESULT wf_cliprdr_fail_locked_file_descriptor_data(HANDLE hMem, STGMEDIUM *medium, CliprdrDataObject *instance, IStream **streams, ULONG stream_count, HRESULT error) { - GlobalUnlock(clipboard->hmem); - GlobalFree(clipboard->hmem); - clipboard->hmem = NULL; - clipboard->hmem_data_len = 0; + GlobalUnlock(hMem); + GlobalFree(hMem); medium->hGlobal = NULL; wf_cliprdr_release_streams(streams, stream_count); wf_cliprdr_reset_streams(instance); @@ -968,49 +992,45 @@ static HRESULT STDMETHODCALLTYPE CliprdrDataObject_GetData(IDataObject *This, FO FILEGROUPDESCRIPTORW *dsc; IStream **streams = NULL; UINT stream_count = 0; - SIZE_T hmem_size; - // DWORD remote_format_id = get_remote_format_id(clipboard, instance->m_pFormatEtc[idx].cfFormat); - // FIXME: origin code may be failed here??? - if (cliprdr_send_data_request(instance->m_connID, clipboard, instance->m_pFormatEtc[idx].cfFormat) != 0) + SIZE_T hmem_size = 0; + HANDLE hMem = NULL; + if (cliprdr_send_data_request(instance->m_connID, clipboard, + instance->m_pFormatEtc[idx].cfFormat, &hMem, &hmem_size) != 0) { return E_UNEXPECTED; } - if (!clipboard->hmem) + if (!hMem) { return E_UNEXPECTED; } - pMedium->hGlobal = clipboard->hmem; /* points to a FILEGROUPDESCRIPTOR structure */ + pMedium->hGlobal = hMem; /* points to a FILEGROUPDESCRIPTOR structure */ /* GlobalLock returns a pointer to the first byte of the memory block, * in which is a FILEGROUPDESCRIPTOR structure, whose first UINT member * is the number of FILEDESCRIPTOR's */ - // dsc = (FILEGROUPDESCRIPTOR *)GlobalLock(clipboard->hmem); - dsc = (FILEGROUPDESCRIPTORW *)GlobalLock(clipboard->hmem); + dsc = (FILEGROUPDESCRIPTORW *)GlobalLock(hMem); if (!dsc) { pMedium->hGlobal = NULL; - GlobalFree(clipboard->hmem); - clipboard->hmem = NULL; - clipboard->hmem_data_len = 0; + GlobalFree(hMem); wf_cliprdr_reset_streams(instance); return E_UNEXPECTED; } - hmem_size = clipboard->hmem_data_len; /* cItems is remote-controlled; verify the fixed header exists before reading it. */ if (hmem_size < offsetof(FILEGROUPDESCRIPTORW, fgd)) return wf_cliprdr_fail_locked_file_descriptor_data( - clipboard, pMedium, instance, NULL, 0, E_UNEXPECTED); + hMem, pMedium, instance, NULL, 0, E_UNEXPECTED); stream_count = dsc->cItems; if (!wf_cliprdr_file_group_descriptor_size_valid(hmem_size, stream_count)) return wf_cliprdr_fail_locked_file_descriptor_data( - clipboard, pMedium, instance, NULL, 0, E_UNEXPECTED); + hMem, pMedium, instance, NULL, 0, E_UNEXPECTED); streams = (IStream **)calloc(stream_count, sizeof(IStream *)); if (!streams) return wf_cliprdr_fail_locked_file_descriptor_data( - clipboard, pMedium, instance, NULL, 0, E_OUTOFMEMORY); + hMem, pMedium, instance, NULL, 0, E_OUTOFMEMORY); for (i = 0; i < stream_count; i++) { @@ -1019,11 +1039,11 @@ static HRESULT STDMETHODCALLTYPE CliprdrDataObject_GetData(IDataObject *This, FO if (!streams[i]) { return wf_cliprdr_fail_locked_file_descriptor_data( - clipboard, pMedium, instance, streams, i, E_OUTOFMEMORY); + hMem, pMedium, instance, streams, i, E_OUTOFMEMORY); } } - GlobalUnlock(clipboard->hmem); + GlobalUnlock(hMem); wf_cliprdr_reset_streams(instance); instance->m_pStream = streams; instance->m_nStreams = stream_count; @@ -1870,35 +1890,80 @@ UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BO return ERROR_INTERNAL_ERROR; } -static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UINT32 formatId) +static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UINT32 formatId, + HANDLE *outHmem, SIZE_T *outLen) { UINT rc; UINT32 remoteFormatId; CLIPRDR_FORMAT_DATA_REQUEST formatDataRequest; + DWORD timeoutMillis; + HANDLE hMem = NULL; + SIZE_T len = 0; - if (!clipboard || !clipboard->context || !clipboard->context->ClientFormatDataRequest) + *outHmem = NULL; + *outLen = 0; + + if (!clipboard || !clipboard->context || !clipboard->context->ClientFormatDataRequest || + !clipboard->format_request_mutex) + return ERROR_INTERNAL_ERROR; + + // Serialize the whole request/response cycle so a second request cannot overwrite + // the shared hmem slot while this one is in flight. + timeoutMillis = (clipboard->context->ResponseWaitTimeoutSecs + 1) * 2000; + if (!lock_mutex(clipboard->format_request_mutex, timeoutMillis)) return ERROR_INTERNAL_ERROR; rc = try_reset_event(clipboard->formatDataRespEvent); if (rc != ERROR_SUCCESS) { + ReleaseMutex(clipboard->format_request_mutex); return rc; } - clipboard->formatDataRespReceived = FALSE; remoteFormatId = get_remote_format_id(clipboard, formatId); - formatDataRequest.connID = connID; formatDataRequest.requestedFormatId = remoteFormatId; clipboard->requestedFormatId = formatId; + + // Mark a response as expected under hmem_mutex; the handler drops any response + // that arrives while this flag is clear. + if (!lock_mutex(clipboard->hmem_mutex, INFINITE)) + { + ReleaseMutex(clipboard->format_request_mutex); + return ERROR_INTERNAL_ERROR; + } + clipboard->formatDataRespReceived = FALSE; + clipboard->formatDataRespExpected = TRUE; + ReleaseMutex(clipboard->hmem_mutex); + rc = clipboard->context->ClientFormatDataRequest(clipboard->context, &formatDataRequest); + if (rc == ERROR_SUCCESS) + rc = wait_response_event(connID, clipboard, clipboard->formatDataRespEvent, + &clipboard->formatDataRespReceived, &clipboard->hmem, NULL); + + // Take ownership of the response buffer and stop expecting a response, both under + // hmem_mutex, so a late/duplicate response can no longer touch the slot. + if (lock_mutex(clipboard->hmem_mutex, INFINITE)) + { + hMem = clipboard->hmem; + len = clipboard->hmem_data_len; + clipboard->hmem = NULL; + clipboard->hmem_data_len = 0; + clipboard->formatDataRespExpected = FALSE; + ReleaseMutex(clipboard->hmem_mutex); + } + ReleaseMutex(clipboard->format_request_mutex); + if (rc != ERROR_SUCCESS) { + if (hMem) + GlobalFree(hMem); return rc; } - return wait_response_event(connID, clipboard, clipboard->formatDataRespEvent, - &clipboard->formatDataRespReceived, &clipboard->hmem, NULL); + *outHmem = hMem; + *outLen = len; + return rc; } UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, UINT32 streamId, ULONG index, @@ -2012,11 +2077,13 @@ static LRESULT CALLBACK cliprdr_proc(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM { if (!is_set_by_instance(clipboard)) { - if (clipboard->hmem) - { - GlobalFree(clipboard->hmem); - clipboard->hmem = NULL; - } + // Drop any stale format-data buffer under hmem_mutex so this does not + // race the channel thread's response handler. + HANDLE stale = NULL; + SIZE_T stale_len = 0; + take_hmem(clipboard, &stale, &stale_len); + if (stale) + GlobalFree(stale); cliprdr_send_format_list(clipboard, 0); } @@ -2043,20 +2110,21 @@ static LRESULT CALLBACK cliprdr_proc(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM // https://docs.microsoft.com/en-us/windows/win32/dataxchg/wm-renderformat?redirectedfrom=MSDN // to-do: ensure usage of 0 - if (cliprdr_send_data_request(0, clipboard, (UINT32)wParam) != 0) { - DEBUG_CLIPRDR("error: cliprdr_send_data_request failed."); - break; - } - - if (!SetClipboardData((UINT)wParam, clipboard->hmem)) - { - DEBUG_CLIPRDR("SetClipboardData failed with 0x%x", GetLastError()); - - if (clipboard->hmem) + HANDLE hMem = NULL; + SIZE_T hmemLen = 0; + if (cliprdr_send_data_request(0, clipboard, (UINT32)wParam, &hMem, &hmemLen) != 0) { - GlobalFree(clipboard->hmem); - clipboard->hmem = NULL; + DEBUG_CLIPRDR("error: cliprdr_send_data_request failed."); + break; + } + + if (!SetClipboardData((UINT)wParam, hMem)) + { + DEBUG_CLIPRDR("SetClipboardData failed with 0x%x", GetLastError()); + + if (hMem) + GlobalFree(hMem); } } @@ -2139,22 +2207,22 @@ static LRESULT CALLBACK cliprdr_proc(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM for (UINT32 i = 0; i < format_ids->size; ++i) { - if (cliprdr_send_data_request(format_ids->connID, clipboard, format_ids->formats[i]) != 0) + HANDLE hMem = NULL; + SIZE_T hmemLen = 0; + if (cliprdr_send_data_request(format_ids->connID, clipboard, + format_ids->formats[i], &hMem, &hmemLen) != 0) { DEBUG_CLIPRDR("error: cliprdr_send_data_request failed."); continue; } - if (!SetClipboardData(format_ids->formats[i], clipboard->hmem)) + if (!SetClipboardData(format_ids->formats[i], hMem)) { printf("SetClipboardData failed with 0x%x\n", GetLastError()); DEBUG_CLIPRDR("SetClipboardData failed with 0x%x", GetLastError()); - if (clipboard->hmem) - { - GlobalFree(clipboard->hmem); - clipboard->hmem = NULL; - } + if (hMem) + GlobalFree(hMem); } } @@ -3068,7 +3136,8 @@ wf_cliprdr_server_format_data_response(CliprdrClientContext *context, UINT rc = ERROR_INTERNAL_ERROR; BYTE *data; HANDLE hMem; - wfClipboard *clipboard; + wfClipboard *clipboard = NULL; + BOOL locked = FALSE; do { @@ -3084,13 +3153,33 @@ wf_cliprdr_server_format_data_response(CliprdrClientContext *context, rc = ERROR_INTERNAL_ERROR; break; } + + if (!lock_mutex(clipboard->hmem_mutex, INFINITE)) + { + rc = ERROR_INTERNAL_ERROR; + break; + } + locked = TRUE; + + // Drop a response nobody is waiting for (late/duplicate/unsolicited) so it + // cannot overwrite the slot a consumer is about to take. + if (!clipboard->formatDataRespExpected) + { + ReleaseMutex(clipboard->hmem_mutex); + return CHANNEL_RC_OK; + } + // Consume the expectation so a duplicate response for the same request hits + // the check above instead of overwriting the slot before the consumer takes it. + clipboard->formatDataRespExpected = FALSE; + + // Free any leftover buffer from a prior request that was never consumed. + if (clipboard->hmem) + GlobalFree(clipboard->hmem); clipboard->hmem = NULL; clipboard->hmem_data_len = 0; if (formatDataResponse->msgFlags != CB_RESPONSE_OK) { - // BOOL emptyRes = wf_do_empty_cliprdr((wfClipboard *)context->custom); - // (void)emptyRes; rc = E_FAIL; break; } @@ -3124,7 +3213,10 @@ wf_cliprdr_server_format_data_response(CliprdrClientContext *context, rc = CHANNEL_RC_OK; } while (0); - if (!SetEvent(clipboard->formatDataRespEvent)) + if (locked) + ReleaseMutex(clipboard->hmem_mutex); + + if (clipboard && !SetEvent(clipboard->formatDataRespEvent)) { // If failed to set event, set flag to indicate the event is received. DEBUG_CLIPRDR("wf_cliprdr_server_format_data_response(), SetEvent failed with 0x%x", GetLastError()); @@ -3572,6 +3664,12 @@ BOOL wf_cliprdr_init(wfClipboard *clipboard, CliprdrClientContext *cliprdr) if (!(clipboard->req_f_request_mutex = CreateMutex(NULL, FALSE, NULL))) goto error; + if (!(clipboard->hmem_mutex = CreateMutex(NULL, FALSE, NULL))) + goto error; + + if (!(clipboard->format_request_mutex = CreateMutex(NULL, FALSE, NULL))) + goto error; + if (!(clipboard->thread = CreateThread(NULL, 0, cliprdr_thread_func, clipboard, 0, NULL))) goto error; @@ -3658,6 +3756,21 @@ BOOL wf_cliprdr_uninit(wfClipboard *clipboard, CliprdrClientContext *cliprdr) if (clipboard->req_f_request_mutex) CloseHandle(clipboard->req_f_request_mutex); + if (clipboard->hmem_mutex) + { + // Reclaim any format-data buffer no consumer took, so it is not leaked. + HANDLE leftover = NULL; + SIZE_T leftover_len = 0; + take_hmem(clipboard, &leftover, &leftover_len); + if (leftover) + GlobalFree(leftover); + + CloseHandle(clipboard->hmem_mutex); + } + + if (clipboard->format_request_mutex) + CloseHandle(clipboard->format_request_mutex); + clear_file_array(clipboard); clear_format_map(clipboard); free(clipboard->format_mappings); diff --git a/tests/test_invariant_wf_cliprdr.c b/tests/test_invariant_wf_cliprdr.c index 47253bae4..99e42a887 100644 --- a/tests/test_invariant_wf_cliprdr.c +++ b/tests/test_invariant_wf_cliprdr.c @@ -150,6 +150,34 @@ START_TEST(test_file_contents_response_validates_data) } END_TEST +START_TEST(test_format_data_response_drops_unexpected) +{ + wfClipboard clipboard = { 0 }; + CliprdrClientContext context = { 0 }; + CLIPRDR_FORMAT_DATA_RESPONSE response = { 0 }; + + clipboard.context = &context; + clipboard.hmem_mutex = CreateMutex(NULL, FALSE, NULL); + clipboard.formatDataRespEvent = CreateEvent(NULL, TRUE, FALSE, NULL); + context.Custom = &clipboard; + + ck_assert_ptr_nonnull(clipboard.hmem_mutex); + ck_assert_ptr_nonnull(clipboard.formatDataRespEvent); + + // With no outstanding request, an unsolicited response must be dropped: it must + // not signal the event or write the hmem slot. + clipboard.formatDataRespExpected = FALSE; + response.msgFlags = CB_RESPONSE_OK; + response.dataLen = 0; + ck_assert_int_eq(wf_cliprdr_server_format_data_response(&context, &response), CHANNEL_RC_OK); + ck_assert_int_eq(WaitForSingleObject(clipboard.formatDataRespEvent, 0), WAIT_TIMEOUT); + ck_assert_ptr_null(clipboard.hmem); + + CloseHandle(clipboard.formatDataRespEvent); + CloseHandle(clipboard.hmem_mutex); +} +END_TEST + Suite *wf_cliprdr_invariant_suite(void) { Suite *s; @@ -166,6 +194,7 @@ Suite *wf_cliprdr_invariant_suite(void) tcase_add_test(tc_core, test_descriptor_size_rejects_extreme_count); tcase_add_test(tc_core, test_file_contents_request_initializes_optional_fields); tcase_add_test(tc_core, test_file_contents_response_validates_data); + tcase_add_test(tc_core, test_format_data_response_drops_unexpected); suite_add_tcase(s, tc_core); return s;