diff --git a/.github/workflows/wf-cliprdr-ci.yml b/.github/workflows/wf-cliprdr-ci.yml deleted file mode 100644 index bc65d22e8..000000000 --- a/.github/workflows/wf-cliprdr-ci.yml +++ /dev/null @@ -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' - ' $<$: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 diff --git a/libs/clipboard/src/windows/wf_cliprdr.c b/libs/clipboard/src/windows/wf_cliprdr.c index 78968cee2..b535b2ec7 100644 --- a/libs/clipboard/src/windows/wf_cliprdr.c +++ b/libs/clipboard/src/windows/wf_cliprdr.c @@ -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) { diff --git a/tests/test_invariant_wf_cliprdr.c b/tests/test_invariant_wf_cliprdr.c deleted file mode 100644 index 3a75a5f3e..000000000 --- a/tests/test_invariant_wf_cliprdr.c +++ /dev/null @@ -1,92 +0,0 @@ -#include -#include -#include - -#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; -}