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>

libs/clipboard/src/windows/wf_cliprdr.c

@@ -281,10 +281,22 @@ struct wf_clipboard
 	LPDATAOBJECT data_obj;
 	HANDLE data_obj_mutex;
 
+	// The req_f* fields are a single-slot rendezvous assuming one outstanding
+	// file-contents request at a time. req_f_mutex guards the req_fdata/req_fsize
+	// hand-off between the channel thread and the explorer-thread consumers; it must
+	// not be held across a blocking wait and does not serialize whole requests.
 	ULONG req_fsize;
 	char *req_fdata;
 	HANDLE req_fevent;
 	BOOL req_f_received;
+	// Distinguishes a successful zero-byte response (EOF) from a failed one; both
+	// leave req_fdata NULL.
+	BOOL req_f_response_ok;
+	// Bytes asked for by the last request; a response claiming more is rejected.
+	ULONG req_f_size_requested;
+	// Stream ID of the last request; responses for another stream are ignored.
+	UINT32 req_f_stream_id_expected;
+	HANDLE req_f_mutex;
 
 	size_t nFiles;
 	size_t file_array_size;
@@ -395,10 +407,30 @@ static ULONG STDMETHODCALLTYPE CliprdrStream_Release(IStream *This)
 	}
 }
 
+// Take ownership of the pending file-contents response buffer, clearing the shared
+// slot under req_f_mutex. The caller must free the returned buffer.
+static void take_req_fdata(wfClipboard *clipboard, char **data, ULONG *size)
+{
+	DWORD wait = WaitForSingleObject(clipboard->req_f_mutex, INFINITE);
+	if (wait != WAIT_OBJECT_0 && wait != WAIT_ABANDONED)
+	{
+		*data = NULL;
+		*size = 0;
+		return;
+	}
+	*data = clipboard->req_fdata;
+	*size = clipboard->req_fsize;
+	clipboard->req_fdata = NULL;
+	clipboard->req_fsize = 0;
+	ReleaseMutex(clipboard->req_f_mutex);
+}
+
 static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULONG cb,
 													ULONG *pcbRead)
 {
 	int ret;
+	ULONG req_size = 0;
+	char *req_data = NULL;
 	CliprdrStream *instance = (CliprdrStream *)This;
 	wfClipboard *clipboard;
 
@@ -415,27 +447,41 @@ static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULO
 											FILECONTENTS_RANGE, instance->m_lOffset.HighPart,
 											instance->m_lOffset.LowPart, cb);
 
-	if (ret < 0)
+	take_req_fdata(clipboard, &req_data, &req_size);
-		return E_FAIL;
 
-	if (clipboard->req_fdata)
+	if (ret != CHANNEL_RC_OK)
 	{
-		CopyMemory(pv, clipboard->req_fdata, clipboard->req_fsize);
+		free(req_data);
-		free(clipboard->req_fdata);
+		return E_FAIL;
-		clipboard->req_fdata = NULL;
 	}
 
-	*pcbRead = clipboard->req_fsize;
+	// A response larger than requested must never overflow `pv`.
+	if (req_size > cb)
+	{
+		free(req_data);
+		return STG_E_READFAULT;
+	}
+
+	// A successful request may legitimately return 0 bytes (EOF).
+	if (req_size > 0)
+	{
+		if (!req_data)
+			return E_FAIL;
+		CopyMemory(pv, req_data, req_size);
+	}
+	free(req_data);
+
+	*pcbRead = req_size;
 	// Check overflow, can not be a real case
-	if ((instance->m_lOffset.QuadPart + clipboard->req_fsize) < instance->m_lOffset.QuadPart) {
+	if ((instance->m_lOffset.QuadPart + req_size) < instance->m_lOffset.QuadPart) {
 		// It's better to crash to release the explorer.exe
 		// This is a critical error, because the explorer is waiting for the data
 		// and the m_lOffset is wrong(overflowed)
 		return S_FALSE;
 	}
-	instance->m_lOffset.QuadPart += clipboard->req_fsize;
+	instance->m_lOffset.QuadPart += req_size;
 
-	if (clipboard->req_fsize < cb)
+	if (req_size < cb)
 		return S_FALSE;
 
 	return S_OK;
@@ -591,6 +637,8 @@ static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData,
 	IStream *iStream = NULL;
 	BOOL success = FALSE;
 	BOOL isDir = FALSE;
+	char *req_data = NULL;
+	ULONG req_sz = 0;
 	CliprdrStream *instance = NULL;
 	wfClipboard *clipboard = (wfClipboard *)pData;
 
@@ -645,12 +693,21 @@ static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData,
 					success = TRUE;
 				}
 
-				if (clipboard->req_fdata != NULL)
+				take_req_fdata(clipboard, &req_data, &req_sz);
+
+				if (req_data != NULL && req_sz >= sizeof(LONGLONG))
 				{
-					instance->m_lSize.QuadPart = *((LONGLONG *)clipboard->req_fdata);
+					LONGLONG sz = 0;
-					free(clipboard->req_fdata);
+					CopyMemory(&sz, req_data, sizeof(sz));
-					clipboard->req_fdata = NULL;
+					instance->m_lSize.QuadPart = sz;
 				}
+				else
+				{
+					// The size probe must return at least sizeof(LONGLONG) bytes.
+					success = FALSE;
+				}
+
+				free(req_data);
 			}
 			else {
 				instance->m_lSize.QuadPart =
@@ -1669,7 +1726,8 @@ UINT try_reset_event(HANDLE event)
 	}
 }
 
-UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BOOL* recvedFlag, void **data)
+UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BOOL* recvedFlag,
+						 void **data, const BOOL *responseSucceeded)
 {
 	UINT rc = ERROR_SUCCESS;
 	clipboard->context->IsStopped = FALSE;
@@ -1709,7 +1767,8 @@ UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BO
 			return ERROR_INTERNAL_ERROR;
 		}
 
-		if ((*data) == NULL)
+		if ((responseSucceeded && !*responseSucceeded) ||
+			(!responseSucceeded && (*data) == NULL))
 		{
 			rc = ERROR_INTERNAL_ERROR;
 		}
@@ -1770,7 +1829,8 @@ static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UIN
 		return rc;
 	}
 
-	return wait_response_event(connID, clipboard, clipboard->formatDataRespEvent, &clipboard->formatDataRespReceived, &clipboard->hmem);
+	return wait_response_event(connID, clipboard, clipboard->formatDataRespEvent,
+							   &clipboard->formatDataRespReceived, &clipboard->hmem, NULL);
 }
 
 UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, const void *streamid, ULONG index,
@@ -1778,7 +1838,7 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co
 									   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 +1849,15 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co
 		return rc;
 	}
 	clipboard->req_f_received = FALSE;
+	clipboard->req_f_response_ok = FALSE;
+	clipboard->req_f_size_requested = nreq;
 
 	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;
+	clipboard->req_f_stream_id_expected = fileContentsRequest.streamId;
 	fileContentsRequest.listIndex = index;
 	fileContentsRequest.dwFlags = flag;
 	fileContentsRequest.nPositionLow = positionlow;
@@ -1808,7 +1871,9 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co
 		return rc;
 	}
 
-	return wait_response_event(connID, clipboard, clipboard->req_fevent, &clipboard->req_f_received, (void **)&clipboard->req_fdata);
+	return wait_response_event(connID, clipboard, clipboard->req_fevent,
+							   &clipboard->req_f_received, (void **)&clipboard->req_fdata,
+							   &clipboard->req_f_response_ok);
 }
 
 static UINT cliprdr_send_response_filecontents(
@@ -3265,8 +3330,10 @@ static UINT
 wf_cliprdr_server_file_contents_response(CliprdrClientContext *context,
 										 const CLIPRDR_FILE_CONTENTS_RESPONSE *fileContentsResponse)
 {
-	wfClipboard *clipboard;
+	wfClipboard *clipboard = NULL;
 	UINT rc = ERROR_INTERNAL_ERROR;
+	BOOL locked = FALSE;
+	DWORD wait;
 
 	do
 	{
@@ -3282,8 +3349,26 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context,
 			rc = ERROR_INTERNAL_ERROR;
 			break;
 		}
+
+		// A response for another stream is ignored without waking the waiter.
+		if (fileContentsResponse->streamId != clipboard->req_f_stream_id_expected)
+			return CHANNEL_RC_OK;
+
+		// WAIT_ABANDONED still means the mutex is held; WAIT_FAILED means it is not,
+		// so it must not be released.
+		wait = WaitForSingleObject(clipboard->req_f_mutex, INFINITE);
+		locked = (wait == WAIT_OBJECT_0 || wait == WAIT_ABANDONED);
+		if (!locked)
+		{
+			rc = ERROR_INTERNAL_ERROR;
+			break;
+		}
+
+		// Free any leftover buffer from a prior request that was never consumed.
+		free(clipboard->req_fdata);
 		clipboard->req_fsize = 0;
 		clipboard->req_fdata = NULL;
+		clipboard->req_f_response_ok = FALSE;
 
 		if (fileContentsResponse->msgFlags != CB_RESPONSE_OK)
 		{
@@ -3291,10 +3376,31 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context,
 			break;
 		}
 
+		// Reject a response claiming more data than was requested.
+		if (fileContentsResponse->cbRequested > clipboard->req_f_size_requested)
+		{
+			rc = ERROR_INTERNAL_ERROR;
+			break;
+		}
+
+		if (fileContentsResponse->cbRequested == 0)
+		{
+			clipboard->req_f_response_ok = TRUE;
+			rc = CHANNEL_RC_OK;
+			break;
+		}
+
+		if (!fileContentsResponse->requestedData)
+		{
+			rc = ERROR_INTERNAL_ERROR;
+			break;
+		}
+
 		clipboard->req_fsize = fileContentsResponse->cbRequested;
 		clipboard->req_fdata = (char *)malloc(fileContentsResponse->cbRequested);
 		if (!clipboard->req_fdata)
 		{
+			clipboard->req_fsize = 0;
 			rc = ERROR_INTERNAL_ERROR;
 			break;
 		}
@@ -3302,10 +3408,14 @@ wf_cliprdr_server_file_contents_response(CliprdrClientContext *context,
 		CopyMemory(clipboard->req_fdata, fileContentsResponse->requestedData,
 				   fileContentsResponse->cbRequested);
 
+		clipboard->req_f_response_ok = TRUE;
 		rc = CHANNEL_RC_OK;
 	} while (0);
 
-	if (!SetEvent(clipboard->req_fevent))
+	if (locked)
+		ReleaseMutex(clipboard->req_f_mutex);
+
+	if (clipboard && !SetEvent(clipboard->req_fevent))
 	{
 		// If failed to set event, set flag to indicate the event is received.
 		DEBUG_CLIPRDR("wf_cliprdr_server_file_contents_response(), SetEvent failed with 0x%x", GetLastError());
@@ -3377,6 +3487,9 @@ BOOL wf_cliprdr_init(wfClipboard *clipboard, CliprdrClientContext *cliprdr)
 		goto error;
 	clipboard->req_f_received = FALSE;
 
+	if (!(clipboard->req_f_mutex = CreateMutex(NULL, FALSE, NULL)))
+		goto error;
+
 	if (!(clipboard->thread = CreateThread(NULL, 0, cliprdr_thread_func, clipboard, 0, NULL)))
 		goto error;
 
@@ -3449,6 +3562,17 @@ BOOL wf_cliprdr_uninit(wfClipboard *clipboard, CliprdrClientContext *cliprdr)
 	if (clipboard->req_fevent)
 		CloseHandle(clipboard->req_fevent);
 
+	if (clipboard->req_f_mutex)
+	{
+		// Reclaim any response buffer that no consumer ever took, so it is not leaked.
+		char *leftover = NULL;
+		ULONG leftover_sz = 0;
+		take_req_fdata(clipboard, &leftover, &leftover_sz);
+		free(leftover);
+
+		CloseHandle(clipboard->req_f_mutex);
+	}
+
 	clear_file_array(clipboard);
 	clear_format_map(clipboard);
 	free(clipboard->format_mappings);

tests/test_invariant_wf_cliprdr.c

@@ -56,6 +56,101 @@ START_TEST(test_descriptor_size_rejects_extreme_count)
 }
 END_TEST
 
+static UINT test_client_file_contents_request(
+	CliprdrClientContext *context,
+	const CLIPRDR_FILE_CONTENTS_REQUEST *request)
+{
+	CLIPRDR_FILE_CONTENTS_RESPONSE response = { 0 };
+
+	ck_assert_int_eq(request->haveClipDataId, FALSE);
+	ck_assert_int_eq(request->clipDataId, 0);
+	response.msgFlags = CB_RESPONSE_OK;
+	response.streamId = request->streamId;
+	return wf_cliprdr_server_file_contents_response(context, &response);
+}
+
+START_TEST(test_file_contents_request_initializes_optional_fields)
+{
+	wfClipboard clipboard = { 0 };
+	CliprdrClientContext context = { 0 };
+	UINT rc;
+
+	clipboard.context = &context;
+	clipboard.req_f_mutex = CreateMutex(NULL, FALSE, NULL);
+	clipboard.req_fevent = CreateEvent(NULL, TRUE, FALSE, NULL);
+	context.Custom = &clipboard;
+	context.ResponseWaitTimeoutSecs = 1;
+	context.ClientFileContentsRequest = test_client_file_contents_request;
+
+	ck_assert_ptr_nonnull(clipboard.req_f_mutex);
+	ck_assert_ptr_nonnull(clipboard.req_fevent);
+	rc = cliprdr_send_request_filecontents(&clipboard, 1, (const void *)(ULONG_PTR)7,
+	                                      0, FILECONTENTS_SIZE, 0, 0, 0);
+	ck_assert_int_eq(rc, CHANNEL_RC_OK);
+	ck_assert_int_eq(clipboard.req_f_stream_id_expected, 7);
+
+	CloseHandle(clipboard.req_fevent);
+	CloseHandle(clipboard.req_f_mutex);
+}
+END_TEST
+
+START_TEST(test_file_contents_response_validates_data)
+{
+	wfClipboard clipboard = { 0 };
+	CliprdrClientContext context = { 0 };
+	CLIPRDR_FILE_CONTENTS_RESPONSE response = { 0 };
+	UINT rc;
+
+	clipboard.context = &context;
+	clipboard.req_f_mutex = CreateMutex(NULL, FALSE, NULL);
+	clipboard.req_fevent = CreateEvent(NULL, TRUE, FALSE, NULL);
+	context.Custom = &clipboard;
+	context.ResponseWaitTimeoutSecs = 1;
+	response.msgFlags = CB_RESPONSE_OK;
+	response.cbRequested = 0;
+	clipboard.req_f_stream_id_expected = 7;
+
+	ck_assert_ptr_nonnull(clipboard.req_f_mutex);
+	ck_assert_ptr_nonnull(clipboard.req_fevent);
+
+	response.streamId = 8;
+	ck_assert_int_eq(wf_cliprdr_server_file_contents_response(&context, &response),
+	                 CHANNEL_RC_OK);
+	ck_assert_int_eq(WaitForSingleObject(clipboard.req_fevent, 0), WAIT_TIMEOUT);
+	ck_assert_ptr_null(clipboard.req_fdata);
+	ck_assert_int_eq(clipboard.req_fsize, 0);
+	ck_assert_int_eq(clipboard.req_f_response_ok, FALSE);
+
+	response.streamId = 7;
+	ck_assert_int_eq(wf_cliprdr_server_file_contents_response(&context, &response),
+	                 CHANNEL_RC_OK);
+	ck_assert_ptr_null(clipboard.req_fdata);
+	ck_assert_int_eq(clipboard.req_fsize, 0);
+	ck_assert_int_eq(clipboard.req_f_response_ok, TRUE);
+
+	rc = wait_response_event(0, &clipboard, clipboard.req_fevent,
+	                         &clipboard.req_f_received, (void **)&clipboard.req_fdata,
+	                         &clipboard.req_f_response_ok);
+	ck_assert_int_eq(rc, CHANNEL_RC_OK);
+
+	response.cbRequested = 1;
+	clipboard.req_f_size_requested = 1;
+	ck_assert_int_eq(wf_cliprdr_server_file_contents_response(&context, &response),
+	                 ERROR_INTERNAL_ERROR);
+	ck_assert_ptr_null(clipboard.req_fdata);
+	ck_assert_int_eq(clipboard.req_fsize, 0);
+	ck_assert_int_eq(clipboard.req_f_response_ok, FALSE);
+
+	rc = wait_response_event(0, &clipboard, clipboard.req_fevent,
+	                         &clipboard.req_f_received, (void **)&clipboard.req_fdata,
+	                         &clipboard.req_f_response_ok);
+	ck_assert_int_eq(rc, ERROR_INTERNAL_ERROR);
+
+	CloseHandle(clipboard.req_fevent);
+	CloseHandle(clipboard.req_f_mutex);
+}
+END_TEST
+
 Suite *wf_cliprdr_invariant_suite(void)
 {
 	Suite *s;
@@ -70,6 +165,8 @@ Suite *wf_cliprdr_invariant_suite(void)
 	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);
+	tcase_add_test(tc_core, test_file_contents_request_initializes_optional_fields);
+	tcase_add_test(tc_core, test_file_contents_response_validates_data);
 
 	suite_add_tcase(s, tc_core);
 	return s;