Compare commits

...

20 Commits

Author SHA1 Message Date
RustDesk
8aeafd5401 Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-07-06 15:57:14 +08:00
rustdesk
e264283bf2 fix review 2026-07-06 15:46:41 +08:00
rustdesk
1053b53b57 fix copilot false report 2026-07-06 15:31:12 +08:00
rustdesk
233eb49e4d refactor and simplify, remove mutex which is dangeours 2026-07-06 15:17:06 +08:00
rustdesk
c974514710 remove wf-cliprdr invariant tests
Drop tests/test_invariant_wf_cliprdr.c on this branch as requested.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-05 22:27:56 +08:00
rustdesk
52ff648411 remove the dedicated wf-cliprdr CI workflow
Drop .github/workflows/wf-cliprdr-ci.yml on this branch as requested.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-05 22:16:05 +08:00
rustdesk
55ae167394 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 <noreply@anthropic.com>
2026-07-04 18:23:58 +08:00
rustdesk
ba5a535891 key the responder stream cache on connID as well as streamId
Per-stream ids restart from 1 in each peer process, so two connections can
emit the same streamId. The process-static pStreamStc cache keyed only on
streamId could then serve one peer the IStream cached for another peer (a
different file), silently returning wrong-file bytes. Add connID to the key.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-04 18:08:55 +08:00
rustdesk
3ab9d4e46d serialize file-contents request state and poison streams after timeout
- Extract lock_mutex() for the WAIT_OBJECT_0/WAIT_ABANDONED idiom shared by
  take_req_fdata, the request-serialization acquire, and the response handler.
- Collapse the acquire/send/take/release cycle into
  cliprdr_request_filecontents_sync(), used by CliprdrStream_Read and the size
  probe in CliprdrStream_New.
- Publish req_f_stream_id_expected/req_f_size_requested under req_f_mutex in the
  sender and read them under the same lock in the response handler, removing the
  cross-thread data race on those fields.
- Poison a stream (m_failed) after a request fails/times out, so a late response
  carrying a previous offset's bytes cannot satisfy a later same-stream read.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-04 18:07:55 +08:00
rustdesk
bb8ffef7f0 address copilot review findings in wf_cliprdr.c
- Reject a negative FILECONTENTS_SIZE result: m_lSize is unsigned, so a
  negative value became a huge bogus stream size that keeps reads going.
- Use a unique per-stream counter as the CLIPRDR streamId instead of a
  truncated IStream pointer, which could collide or be reused after free
  (and leaked heap addresses to the peer).
- Add req_f_request_mutex to serialize whole file-contents request/response
  cycles, enforcing the previously assumed one-outstanding-request
  invariant when multiple streams are read concurrently. Bounded acquire
  so a wedged request fails the read instead of hanging a consumer.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-04 17:42:32 +08:00
rustdesk
1c2188f80b add invariant tests for file contents request/response hardening
Cover the zeroed optional request fields, stream ID filtering,
oversized/NULL response rejection and the zero-byte EOF path.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-04 16:31:32 +08:00
rustdesk
ff1ca85827 condense hardening comments, fix style in wf_cliprdr.c
Comment-only cleanup of the review-justification comments; also move
the mutex wait result declaration to the top of the block and fix
continuation-line indentation. No behavior change.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2026-07-04 16:31:26 +08:00
rustdesk
f695413ee7 fix review 2026-07-04 15:47:53 +08:00
rustdesk
b0555639a2 fix review 2026-07-04 15:20:23 +08:00
RustDesk
cd4ed15214 Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-07-04 14:55:09 +08:00
RustDesk
96ab03aa65 Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-07-04 14:55:00 +08:00
RustDesk
a30af8a321 Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-07-04 14:54:49 +08:00
RustDesk
ed9a423570 Potential fix for pull request finding
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
2026-07-04 14:54:30 +08:00
rustdesk
5cc7355dd2 fix copilot review 2026-07-04 14:41:16 +08:00
rustdesk
a9eca51ab0 harden wf_cliprdr.c 2026-07-04 14:03:10 +08:00
3 changed files with 54 additions and 191 deletions

View File

@@ -1,85 +0,0 @@
name: wf-cliprdr CI
on:
workflow_dispatch:
pull_request:
paths:
- "libs/clipboard/src/windows/**"
- "tests/test_invariant_wf_cliprdr.c"
- ".github/workflows/wf-cliprdr-ci.yml"
push:
branches:
- master
paths:
- "libs/clipboard/src/windows/**"
- "tests/test_invariant_wf_cliprdr.c"
- ".github/workflows/wf-cliprdr-ci.yml"
permissions:
contents: read
concurrency:
group: ${{ github.workflow }}-${{ github.ref }}
cancel-in-progress: true
jobs:
test:
name: wf_cliprdr invariant test
runs-on: windows-2022
steps:
- name: Checkout source code
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4
with:
persist-credentials: false
- name: Set up MSVC
uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756
with:
arch: x64
- name: Setup vcpkg with GitHub Actions binary cache
uses: lukka/run-vcpkg@b1a0dd252f06b9e25b3c022a9a03bd7a427fb6a2 # v11
with:
vcpkgDirectory: C:\vcpkg
doNotCache: false
- name: Install vcpkg dependency
shell: pwsh
run: |
& "$env:VCPKG_ROOT\vcpkg.exe" install check:x64-windows --classic --x-install-root="$env:VCPKG_ROOT\installed"
- name: Build test
shell: pwsh
run: |
$testRoot = Join-Path $env:GITHUB_WORKSPACE 'build\wf-cliprdr'
New-Item -ItemType Directory -Force $testRoot | Out-Null
$testSource = (($env:GITHUB_WORKSPACE -replace '\\', '/') + '/tests/test_invariant_wf_cliprdr.c')
$cmakeLists = @(
'cmake_minimum_required(VERSION 3.20)'
'project(test_invariant_wf_cliprdr C)'
''
'set(CMAKE_C_STANDARD 11)'
'set(CMAKE_C_STANDARD_REQUIRED ON)'
'set(CMAKE_C_EXTENSIONS OFF)'
''
'find_package(check CONFIG REQUIRED)'
''
'add_executable(test_invariant_wf_cliprdr'
' "TEST_SOURCE"'
')'
''
'target_link_libraries(test_invariant_wf_cliprdr PRIVATE'
' $<$<TARGET_EXISTS:Check::check>:Check::check>'
' $<$<NOT:$<TARGET_EXISTS:Check::check>>:Check::checkShared>'
')'
) -join [Environment]::NewLine
$cmakeLists.Replace('TEST_SOURCE', $testSource) | Set-Content -NoNewline (Join-Path $testRoot 'CMakeLists.txt')
cmake -S $testRoot -B (Join-Path $testRoot 'out') -G "Visual Studio 17 2022" -A x64 -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_ROOT\scripts\buildsystems\vcpkg.cmake" -DVCPKG_TARGET_TRIPLET=x64-windows
cmake --build (Join-Path $testRoot 'out') --config Release
- name: Run test
shell: pwsh
run: .\build\wf-cliprdr\out\Release\test_invariant_wf_cliprdr.exe

View File

@@ -232,6 +232,7 @@ struct _CliprdrStream
FILEDESCRIPTORW m_Dsc;
void *m_pData;
UINT32 m_connID;
UINT32 m_streamId; // unique CLIPRDR streamId; avoids leaking a heap pointer
};
typedef struct _CliprdrStream CliprdrStream;
@@ -285,6 +286,9 @@ struct wf_clipboard
char *req_fdata;
HANDLE req_fevent;
BOOL req_f_received;
UINT32 req_f_conn_id_expected; // connID of the outstanding request
UINT32 req_f_stream_id_expected; // streamId of the outstanding request; responses for another are dropped
LONG req_f_stream_id_seq; // source of unique per-stream ids
size_t nFiles;
size_t file_array_size;
@@ -315,7 +319,7 @@ 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_lock(wfClipboard *clipboard);
static UINT cliprdr_send_unlock(wfClipboard *clipboard);
static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, const void *streamid,
static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, UINT32 streamId,
ULONG index, UINT32 flag, DWORD positionhigh,
DWORD positionlow, ULONG request);
@@ -398,7 +402,7 @@ static ULONG STDMETHODCALLTYPE CliprdrStream_Release(IStream *This)
static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULONG cb,
ULONG *pcbRead)
{
int ret;
UINT ret;
CliprdrStream *instance = (CliprdrStream *)This;
wfClipboard *clipboard;
@@ -411,12 +415,23 @@ static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULO
if (instance->m_lOffset.QuadPart >= instance->m_lSize.QuadPart)
return S_FALSE;
ret = cliprdr_send_request_filecontents(clipboard, instance->m_connID, (void *)This, instance->m_lIndex,
ret = cliprdr_send_request_filecontents(clipboard, instance->m_connID, instance->m_streamId, instance->m_lIndex,
FILECONTENTS_RANGE, instance->m_lOffset.HighPart,
instance->m_lOffset.LowPart, cb);
if (ret < 0)
if (ret != CHANNEL_RC_OK)
{
free(clipboard->req_fdata);
clipboard->req_fdata = NULL;
return E_FAIL;
}
if (clipboard->req_fsize > cb)
{
free(clipboard->req_fdata);
clipboard->req_fdata = NULL;
return STG_E_READFAULT;
}
if (clipboard->req_fdata)
{
@@ -628,6 +643,7 @@ static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData,
instance->m_pData = pData;
instance->m_lOffset.QuadPart = 0;
instance->m_connID = connID;
instance->m_streamId = (UINT32)InterlockedIncrement(&clipboard->req_f_stream_id_seq);
if (instance->m_Dsc.dwFlags & FD_ATTRIBUTES)
{
@@ -638,16 +654,28 @@ static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData,
if (((instance->m_Dsc.dwFlags & FD_FILESIZE) == 0) && !isDir)
{
/* get content size of this stream */
if (cliprdr_send_request_filecontents(clipboard, instance->m_connID, (void *)instance,
if (cliprdr_send_request_filecontents(clipboard, instance->m_connID, instance->m_streamId,
instance->m_lIndex, FILECONTENTS_SIZE, 0, 0,
8) == CHANNEL_RC_OK)
{
success = TRUE;
}
if (clipboard->req_fdata != NULL)
if (clipboard->req_fdata != NULL && clipboard->req_fsize >= sizeof(LONGLONG))
{
LONGLONG sz = 0;
CopyMemory(&sz, clipboard->req_fdata, sizeof(sz));
if (sz < 0)
success = FALSE;
else
instance->m_lSize.QuadPart = sz;
}
else
{
success = FALSE;
}
if (clipboard->req_fdata)
{
instance->m_lSize.QuadPart = *((LONGLONG *)clipboard->req_fdata);
free(clipboard->req_fdata);
clipboard->req_fdata = NULL;
}
@@ -1773,12 +1801,12 @@ static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UIN
return wait_response_event(connID, clipboard, clipboard->formatDataRespEvent, &clipboard->formatDataRespReceived, &clipboard->hmem);
}
UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, const void *streamid, ULONG index,
static UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, UINT32 streamId, ULONG index,
UINT32 flag, DWORD positionhigh, DWORD positionlow,
ULONG nreq)
{
UINT rc;
CLIPRDR_FILE_CONTENTS_REQUEST fileContentsRequest;
CLIPRDR_FILE_CONTENTS_REQUEST fileContentsRequest = { 0 };
if (!clipboard || !clipboard->context || !clipboard->context->ClientFileContentsRequest)
return ERROR_INTERNAL_ERROR;
@@ -1789,12 +1817,11 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co
return rc;
}
clipboard->req_f_received = FALSE;
clipboard->req_f_conn_id_expected = connID;
clipboard->req_f_stream_id_expected = streamId;
fileContentsRequest.connID = connID;
// streamId is `IStream*` pointer, though it is not very good on a 64-bit system.
// But it is OK, because it is only used to check if the stream is the same in
// `wf_cliprdr_server_file_contents_request()` function.
fileContentsRequest.streamId = (UINT32)(ULONG_PTR)streamid;
fileContentsRequest.streamId = streamId;
fileContentsRequest.listIndex = index;
fileContentsRequest.dwFlags = flag;
fileContentsRequest.nPositionLow = positionlow;
@@ -3015,6 +3042,7 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context,
BOOL bIsStreamFile = TRUE;
static LPSTREAM pStreamStc = NULL;
static UINT32 uStreamIdStc = 0;
static UINT32 uConnIdStc = 0;
wfClipboard *clipboard;
UINT rc = ERROR_INTERNAL_ERROR;
UINT sRc;
@@ -3088,7 +3116,8 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context,
vFormatEtc.lindex = fileContentsRequest->listIndex;
vFormatEtc.ptd = NULL;
if ((uStreamIdStc != fileContentsRequest->streamId) || !pStreamStc)
if ((uStreamIdStc != fileContentsRequest->streamId) ||
(uConnIdStc != fileContentsRequest->connID) || !pStreamStc)
{
LPENUMFORMATETC pEnumFormatEtc;
ULONG CeltFetched;
@@ -3119,6 +3148,7 @@ wf_cliprdr_server_file_contents_request(CliprdrClientContext *context,
{
pStreamStc = vStgMedium.pstm;
uStreamIdStc = fileContentsRequest->streamId;
uConnIdStc = fileContentsRequest->connID;
bIsStreamFile = TRUE;
}
@@ -3282,6 +3312,9 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context,
rc = ERROR_INTERNAL_ERROR;
break;
}
if (fileContentsResponse->connID != clipboard->req_f_conn_id_expected ||
fileContentsResponse->streamId != clipboard->req_f_stream_id_expected)
return CHANNEL_RC_OK;
clipboard->req_fsize = 0;
clipboard->req_fdata = NULL;
@@ -3292,6 +3325,13 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context,
}
clipboard->req_fsize = fileContentsResponse->cbRequested;
/*
* Keep the zero-size allocation: supported Windows builds use the Microsoft
* CRT, where malloc(0) returns a valid pointer. wait_response_event() also
* uses a non-NULL req_fdata to recognize a successful zero-byte response.
* The Rust FFI derives requestedData and cbRequested from the same Vec, so a
* nonzero length cannot have a NULL data pointer on the normal call path.
*/
clipboard->req_fdata = (char *)malloc(fileContentsResponse->cbRequested);
if (!clipboard->req_fdata)
{

View File

@@ -1,92 +0,0 @@
#include <check.h>
#include <stdlib.h>
#include <stddef.h>
#include "../libs/clipboard/src/windows/wf_cliprdr.c"
static SIZE_T descriptor_size(UINT count)
{
return offsetof(FILEGROUPDESCRIPTORW, fgd) + (SIZE_T)count * sizeof(FILEDESCRIPTORW);
}
START_TEST(test_descriptor_size_rejects_buffer_smaller_than_header)
{
ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(0, 1), FALSE);
ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(
offsetof(FILEGROUPDESCRIPTORW, fgd) - 1, 1),
FALSE);
}
END_TEST
START_TEST(test_descriptor_size_rejects_zero_items)
{
ck_assert_int_eq(
wf_cliprdr_file_group_descriptor_size_valid(offsetof(FILEGROUPDESCRIPTORW, fgd), 0),
FALSE);
}
END_TEST
START_TEST(test_descriptor_size_accepts_max_stream_count)
{
ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(
descriptor_size(WF_CLIPRDR_MAX_STREAMS), WF_CLIPRDR_MAX_STREAMS),
TRUE);
}
END_TEST
START_TEST(test_descriptor_size_rejects_stream_count_above_limit)
{
ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(
descriptor_size(WF_CLIPRDR_MAX_STREAMS), WF_CLIPRDR_MAX_STREAMS + 1),
FALSE);
}
END_TEST
START_TEST(test_descriptor_size_rejects_truncated_descriptor_array)
{
ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid(descriptor_size(2) - 1, 2),
FALSE);
}
END_TEST
START_TEST(test_descriptor_size_rejects_extreme_count)
{
ck_assert_int_eq(wf_cliprdr_file_group_descriptor_size_valid((SIZE_T)-1, (UINT)-1),
FALSE);
}
END_TEST
Suite *wf_cliprdr_invariant_suite(void)
{
Suite *s;
TCase *tc_core;
s = suite_create("wf_cliprdr_invariants");
tc_core = tcase_create("descriptor_size");
tcase_add_test(tc_core, test_descriptor_size_rejects_buffer_smaller_than_header);
tcase_add_test(tc_core, test_descriptor_size_rejects_zero_items);
tcase_add_test(tc_core, test_descriptor_size_accepts_max_stream_count);
tcase_add_test(tc_core, test_descriptor_size_rejects_stream_count_above_limit);
tcase_add_test(tc_core, test_descriptor_size_rejects_truncated_descriptor_array);
tcase_add_test(tc_core, test_descriptor_size_rejects_extreme_count);
suite_add_tcase(s, tc_core);
return s;
}
int main(void)
{
int number_failed;
Suite *s;
SRunner *sr;
s = wf_cliprdr_invariant_suite();
sr = srunner_create(s);
srunner_run_all(sr, CK_NORMAL);
number_failed = srunner_ntests_failed(sr);
srunner_free(sr);
return (number_failed == 0) ? EXIT_SUCCESS : EXIT_FAILURE;
}