Fix/wf cliprdr c bugs (#9253)

* fix: ResetEvent() after WaitForSingleObject() Signed-off-by: fufesou <linlong1266@gmail.com> * fix: check and free mem Signed-off-by: fufesou <linlong1266@gmail.com> --------- Signed-off-by: fufesou <linlong1266@gmail.com>
2024-11-27 14:59:02 +08:00 · 2024-09-04 17:04:48 +08:00 · 2024-09-04 17:04:48 +08:00 · e40243b55d
commit e40243b55d
parent dbbbd08934
1 changed files with 190 additions and 51 deletions
--- a/libs/clipboard/src/windows/wf_cliprdr.c
+++ b/libs/clipboard/src/windows/wf_cliprdr.c
@ -289,6 +289,9 @@ static BOOL try_open_clipboard(HWND hwnd)
 static HRESULT STDMETHODCALLTYPE CliprdrStream_QueryInterface(IStream *This, REFIID riid,
 															  void **ppvObject)
 {
+	if (ppvObject == NULL)
+		return E_INVALIDARG;
+
 	if (IsEqualIID(riid, &IID_IStream) || IsEqualIID(riid, &IID_IUnknown))
 	{
 		IStream_AddRef(This);
@ -364,6 +367,13 @@ static HRESULT STDMETHODCALLTYPE CliprdrStream_Read(IStream *This, void *pv, ULO
 	}

 	*pcbRead = clipboard->req_fsize;
+	// Check overflow, can not be a real case
+	if ((instance->m_lOffset.QuadPart + clipboard->req_fsize) < 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;

 	if (clipboard->req_fsize < cb)
@ -519,11 +529,17 @@ static HRESULT STDMETHODCALLTYPE CliprdrStream_Clone(IStream *This, IStream **pp

 static CliprdrStream *CliprdrStream_New(UINT32 connID, ULONG index, void *pData, const FILEDESCRIPTORW *dsc)
 {
-	IStream *iStream;
+	IStream *iStream = NULL;
 	BOOL success = FALSE;
 	BOOL isDir = FALSE;
-	CliprdrStream *instance;
+	CliprdrStream *instance = NULL;
 	wfClipboard *clipboard = (wfClipboard *)pData;
+
+	if (!(pData && dsc))
+	{
+		return NULL;
+	}
+
 	instance = (CliprdrStream *)calloc(1, sizeof(CliprdrStream));

 	if (instance)
@ -876,14 +892,18 @@ static HRESULT STDMETHODCALLTYPE CliprdrDataObject_EnumDAdvise(IDataObject *This
 static CliprdrDataObject *CliprdrDataObject_New(UINT32 connID, FORMATETC *fmtetc, STGMEDIUM *stgmed, ULONG count,
 												void *data)
 {
-	CliprdrDataObject *instance;
-	IDataObject *iDataObject;
+	CliprdrDataObject *instance = NULL;
+	IDataObject *iDataObject = NULL;
 	instance = (CliprdrDataObject *)calloc(1, sizeof(CliprdrDataObject));

 	if (!instance)
 		goto error;

+	instance->m_pFormatEtc = NULL;
+	instance->m_pStgMedium = NULL;
+
 	iDataObject = &instance->iDataObject;
+	iDataObject->lpVtbl = NULL;
 	iDataObject->lpVtbl = (IDataObjectVtbl *)calloc(1, sizeof(IDataObjectVtbl));

 	if (!iDataObject->lpVtbl)
@ -931,7 +951,24 @@ static CliprdrDataObject *CliprdrDataObject_New(UINT32 connID, FORMATETC *fmtetc

 	return instance;
 error:
-	CliprdrDataObject_Delete(instance);
+	if (iDataObject && iDataObject->lpVtbl)
+	{
+		free(iDataObject->lpVtbl);
+	}
+	if (instance)
+	{
+		if (instance->m_pFormatEtc)
+		{
+			free(instance->m_pFormatEtc);
+		}
+
+		if (instance->m_pStgMedium)
+		{
+			free(instance->m_pStgMedium);
+		}
+
+		CliprdrDataObject_Delete(instance);
+	}
 	return NULL;
 }

@ -1012,6 +1049,8 @@ static HRESULT STDMETHODCALLTYPE CliprdrEnumFORMATETC_QueryInterface(IEnumFORMAT
 																	 REFIID riid, void **ppvObject)
 {
 	(void)This;
+	if (!ppvObject)
+		return E_INVALIDARG;

 	if (IsEqualIID(riid, &IID_IEnumFORMATETC) || IsEqualIID(riid, &IID_IUnknown))
 	{
@ -1200,6 +1239,7 @@ static UINT32 get_local_format_id_by_name(wfClipboard *clipboard, const TCHAR *f
 	WCHAR *unicode_name;
 #if !defined(UNICODE)
 	size_t size;
+	int towchar_count;
 #endif

 	if (!clipboard || !format_name)
@ -1207,6 +1247,8 @@ static UINT32 get_local_format_id_by_name(wfClipboard *clipboard, const TCHAR *f

 #if defined(UNICODE)
 	unicode_name = _wcsdup(format_name);
+	if (!unicode_name)
+		return 0;
 #else
 	size = _tcslen(format_name);
 	unicode_name = calloc(size + 1, sizeof(WCHAR));
@ -1214,11 +1256,13 @@ static UINT32 get_local_format_id_by_name(wfClipboard *clipboard, const TCHAR *f
 	if (!unicode_name)
 		return 0;

-	MultiByteToWideChar(CP_OEMCP, 0, format_name, strlen(format_name), unicode_name, size);
-#endif
-
-	if (!unicode_name)
+	towchar_count = MultiByteToWideChar(CP_OEMCP, 0, format_name, strlen(format_name), NULL, 0);
+	if (towchar_count <= 0 || towchar_count > size)
 		return 0;
+	towchar_count = MultiByteToWideChar(CP_OEMCP, 0, format_name, strlen(format_name), unicode_name, size);
+	if (towchar_count <= 0)
+		return 0;
+#endif

 	for (i = 0; i < clipboard->map_size; i++)
 	{
@ -1314,6 +1358,9 @@ static UINT cliprdr_send_tempdir(wfClipboard *clipboard)
 	if (!clipboard)
 		return -1;

+	// to-do:
+	// Directly use the environment variable `TEMP` is not safe.
+	// But this function is not used for now.
 	if (GetEnvironmentVariableA("TEMP", tempDirectory.szTempDir, sizeof(tempDirectory.szTempDir)) ==
 		0)
 		return -1;
@ -1446,6 +1493,36 @@ static UINT cliprdr_send_format_list(wfClipboard *clipboard, UINT32 connID)
 	return rc;
 }

+// Ensure the event is not signaled, and reset it if it is.
+UINT try_reset_event(HANDLE event)
+{
+	if (!event)
+	{
+		return ERROR_INTERNAL_ERROR;
+	}
+
+	DWORD result = WaitForSingleObject(event, 0);
+	if (result == WAIT_OBJECT_0)
+	{
+		if (!ResetEvent(event))
+		{
+			return GetLastError();
+		}
+		else
+		{
+			return ERROR_SUCCESS;
+		}
+	}
+	else if (result == WAIT_TIMEOUT)
+	{
+		return ERROR_SUCCESS;
+	}
+	else
+	{
+		return ERROR_INTERNAL_ERROR;
+	}
+}
+
 UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BOOL* recvedFlag, void **data)
 {
 	UINT rc = ERROR_SUCCESS;
@ -1470,6 +1547,11 @@ UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BO
 			}
 		}

+		if (!ResetEvent(event))
+		{
+			// NOTE: critical error here, crash may be better
+		}
+
 		if (clipboard->context->IsStopped == TRUE)
 		{
 			wf_do_empty_cliprdr(clipboard);
@ -1481,12 +1563,6 @@ UINT wait_response_event(UINT32 connID, wfClipboard *clipboard, HANDLE event, BO
 			return ERROR_INTERNAL_ERROR;
 		}

-		if (!ResetEvent(event))
-		{
-			// NOTE: critical error here, crash may be better
-			rc = ERROR_INTERNAL_ERROR;
-		}
-
 		if ((*data) == NULL)
 		{
 			rc = ERROR_INTERNAL_ERROR;
@ -1530,12 +1606,18 @@ static UINT cliprdr_send_data_request(UINT32 connID, wfClipboard *clipboard, UIN
 	if (!clipboard || !clipboard->context || !clipboard->context->ClientFormatDataRequest)
 		return ERROR_INTERNAL_ERROR;

+	rc = try_reset_event(clipboard->formatDataRespEvent);
+	if (rc != ERROR_SUCCESS)
+	{
+		return rc;
+	}
+	clipboard->formatDataRespReceived = FALSE;
+
 	remoteFormatId = get_remote_format_id(clipboard, formatId);

 	formatDataRequest.connID = connID;
 	formatDataRequest.requestedFormatId = remoteFormatId;
 	clipboard->requestedFormatId = formatId;
-	clipboard->formatDataRespReceived = FALSE;
 	rc = clipboard->context->ClientFormatDataRequest(clipboard->context, &formatDataRequest);
 	if (rc != ERROR_SUCCESS)
 	{
@ -1555,7 +1637,17 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co
 	if (!clipboard || !clipboard->context || !clipboard->context->ClientFileContentsRequest)
 		return ERROR_INTERNAL_ERROR;

+	rc = try_reset_event(clipboard->req_fevent);
+	if (rc != ERROR_SUCCESS)
+	{
+		return rc;
+	}
+	clipboard->req_f_received = FALSE;
+
 	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.listIndex = index;
 	fileContentsRequest.dwFlags = flag;
@ -1564,7 +1656,6 @@ UINT cliprdr_send_request_filecontents(wfClipboard *clipboard, UINT32 connID, co
 	fileContentsRequest.cbRequested = nreq;
 	fileContentsRequest.clipDataId = 0;
 	fileContentsRequest.msgFlags = 0;
-	clipboard->req_f_received = FALSE;
 	rc = clipboard->context->ClientFileContentsRequest(clipboard->context, &fileContentsRequest);
 	if (rc != ERROR_SUCCESS)
 	{
@ -1801,6 +1892,7 @@ static LRESULT CALLBACK cliprdr_proc(HWND hWnd, UINT Msg, WPARAM wParam, LPARAM
 		break;

 	case WM_DESTROYCLIPBOARD:
+		// to-do: clear clipboard data?
 	case WM_ASKCBFORMATNAME:
 	case WM_HSCROLLCLIPBOARD:
 	case WM_PAINTCLIPBOARD:
@ -1917,7 +2009,7 @@ static BOOL wf_cliprdr_get_file_contents(WCHAR *file_name, BYTE *buffer, LONG po
 										 LONG positionHigh, DWORD nRequested, DWORD *puSize)
 {
 	BOOL res = FALSE;
-	HANDLE hFile;
+	HANDLE hFile = NULL;
 	DWORD nGet, rc;

 	if (!file_name || !buffer || !puSize)
@ -1945,9 +2037,11 @@ static BOOL wf_cliprdr_get_file_contents(WCHAR *file_name, BYTE *buffer, LONG po

 	res = TRUE;
 error:
-
-	if (!CloseHandle(hFile))
-		res = FALSE;
+	if (hFile)
+	{
+		if (!CloseHandle(hFile))
+			res = FALSE;
+	}

 	if (res)
 		*puSize = nGet;
@ -1958,8 +2052,8 @@ error:
 /* path_name has a '\' at the end. e.g. c:\newfolder\, file_name is c:\newfolder\new.txt */
 static FILEDESCRIPTORW *wf_cliprdr_get_file_descriptor(WCHAR *file_name, size_t pathLen)
 {
-	HANDLE hFile;
-	FILEDESCRIPTORW *fd;
+	HANDLE hFile = NULL;
+	FILEDESCRIPTORW *fd = NULL;
 	fd = (FILEDESCRIPTORW *)calloc(1, sizeof(FILEDESCRIPTORW));

 	if (!fd)
@ -1988,7 +2082,16 @@ static FILEDESCRIPTORW *wf_cliprdr_get_file_descriptor(WCHAR *file_name, size_t
 	}

 	fd->nFileSizeLow = GetFileSize(hFile, &fd->nFileSizeHigh);
-	wcscpy_s(fd->cFileName, sizeof(fd->cFileName) / 2, file_name + pathLen);
+	if ((wcslen(file_name + pathLen) + 1) > sizeof(fd->cFileName) / sizeof(fd->cFileName[0]))
+	{
+		// The file name is too long, which is not a normal case.
+		// So we just return NULL.
+		CloseHandle(hFile);
+		free(fd);
+		return NULL;
+	}
+
+	wcsncpy_s(fd->cFileName, sizeof(fd->cFileName) / sizeof(fd->cFileName[0]), file_name + pathLen, wcslen(file_name + pathLen) + 1);
 	CloseHandle(hFile);

 	return fd;
@ -2037,7 +2140,12 @@ static BOOL wf_cliprdr_add_to_file_arrays(wfClipboard *clipboard, WCHAR *full_fi
 	if (!clipboard->file_names[clipboard->nFiles])
 		return FALSE;

-	wcscpy_s(clipboard->file_names[clipboard->nFiles], MAX_PATH, full_file_name);
+	// `MAX_PATH` is long enough for the file name.
+	// So we just return FALSE if the file name is too long, which is not a normal case.
+	if ((wcslen(full_file_name) + 1) > MAX_PATH)
+		return FALSE;
+
+	wcsncpy_s(clipboard->file_names[clipboard->nFiles], MAX_PATH, full_file_name, wcslen(full_file_name) + 1);
 	/* add to descriptor array */
 	clipboard->fileDescriptor[clipboard->nFiles] =
 		wf_cliprdr_get_file_descriptor(full_file_name, pathLen);
@ -2061,8 +2169,8 @@ static BOOL wf_cliprdr_traverse_directory(wfClipboard *clipboard, WCHAR *Dir, si
 	if (!clipboard || !Dir)
 		return FALSE;

-	// StringCchCopy(DirSpec, MAX_PATH, Dir);
-	// StringCchCat(DirSpec, MAX_PATH, TEXT("\\*"));
+	if (wcslen(Dir) + 3 > MAX_PATH)
+		return FALSE;
 	StringCchCopyW(DirSpec, MAX_PATH, Dir);
 	StringCchCatW(DirSpec, MAX_PATH, L"\\*");

@ -2091,9 +2199,8 @@ static BOOL wf_cliprdr_traverse_directory(wfClipboard *clipboard, WCHAR *Dir, si
 		if ((FindFileData.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) != 0)
 		{
 			WCHAR DirAdd[MAX_PATH];
-			// StringCchCopy(DirAdd, MAX_PATH, Dir);
-			// StringCchCat(DirAdd, MAX_PATH, _T("\\"));
-			// StringCchCat(DirAdd, MAX_PATH, FindFileData.cFileName);
+			if (wcslen(Dir) + wcslen(FindFileData.cFileName) + 2 > MAX_PATH)
+				return FALSE;
 			StringCchCopyW(DirAdd, MAX_PATH, Dir);
 			StringCchCatW(DirAdd, MAX_PATH, L"\\");
 			StringCchCatW(DirAdd, MAX_PATH, FindFileData.cFileName);
@ -2107,10 +2214,8 @@ static BOOL wf_cliprdr_traverse_directory(wfClipboard *clipboard, WCHAR *Dir, si
 		else
 		{
 			WCHAR fileName[MAX_PATH];
-			// StringCchCopy(fileName, MAX_PATH, Dir);
-			// StringCchCat(fileName, MAX_PATH, _T("\\"));
-			// StringCchCat(fileName, MAX_PATH, FindFileData.cFileName);
-
+			if (wcslen(Dir) + wcslen(FindFileData.cFileName) + 2 > MAX_PATH)
+				return FALSE;
 			StringCchCopyW(fileName, MAX_PATH, Dir);
 			StringCchCatW(fileName, MAX_PATH, L"\\");
 			StringCchCatW(fileName, MAX_PATH, FindFileData.cFileName);
@ -2255,9 +2360,11 @@ static UINT wf_cliprdr_server_format_list(CliprdrClientContext *context,
 		if (context->EnableFiles)
 		{
 			UINT32 *p_conn_id = (UINT32 *)calloc(1, sizeof(UINT32));
-			*p_conn_id = formatList->connID;
-			if (PostMessage(clipboard->hwnd, WM_CLIPRDR_MESSAGE, OLE_SETCLIPBOARD, p_conn_id))
-				rc = CHANNEL_RC_OK;
+			if (p_conn_id) {
+				*p_conn_id = formatList->connID;
+				if (PostMessage(clipboard->hwnd, WM_CLIPRDR_MESSAGE, OLE_SETCLIPBOARD, p_conn_id))
+					rc = CHANNEL_RC_OK;
+			}
 		}
 		else
 		{
@ -2278,16 +2385,30 @@ static UINT wf_cliprdr_server_format_list(CliprdrClientContext *context,
 				//    SetClipboardData(clipboard->format_mappings[i].local_format_id, NULL);

 				FORMAT_IDS *format_ids = (FORMAT_IDS *)calloc(1, sizeof(FORMAT_IDS));
-				format_ids->connID = formatList->connID;
-				format_ids->size = (UINT32)clipboard->map_size;
-				format_ids->formats = (UINT32 *)calloc(format_ids->size, sizeof(UINT32));
-				for (i = 0; i < format_ids->size; ++i)
+				if (format_ids)
 				{
-					format_ids->formats[i] = clipboard->format_mappings[i].local_format_id;
-				}
-				if (PostMessage(clipboard->hwnd, WM_CLIPRDR_MESSAGE, DELAYED_RENDERING, format_ids))
-				{
-					rc = CHANNEL_RC_OK;
+					format_ids->connID = formatList->connID;
+					format_ids->size = (UINT32)clipboard->map_size;
+					format_ids->formats = (UINT32 *)calloc(format_ids->size, sizeof(UINT32));
+					if (format_ids->formats)
+					{
+						for (i = 0; i < format_ids->size; ++i)
+						{
+							format_ids->formats[i] = clipboard->format_mappings[i].local_format_id;
+						}
+						if (PostMessage(clipboard->hwnd, WM_CLIPRDR_MESSAGE, DELAYED_RENDERING, format_ids))
+						{
+							rc = CHANNEL_RC_OK;
+						}
+						else
+						{
+							rc = ERROR_INTERNAL_ERROR;
+						}
+					}
+					else
+					{
+						rc = ERROR_INTERNAL_ERROR;
+					}
 				}
 				else
 				{
@ -2482,17 +2603,28 @@ wf_cliprdr_server_format_data_request(CliprdrClientContext *context,
 				 p += len + 1, clipboard->nFiles++)
 			{
 				int cchWideChar;
-				WCHAR *wFileName;
 				cchWideChar = MultiByteToWideChar(CP_ACP, MB_COMPOSITE, p, len, NULL, 0);
 				wFileName = (LPWSTR)calloc(cchWideChar, sizeof(WCHAR));
-				MultiByteToWideChar(CP_ACP, MB_COMPOSITE, p, len, wFileName, cchWideChar);
-				wf_cliprdr_process_filename(clipboard, wFileName, cchWideChar);
+				if (wFileName)
+				{
+					MultiByteToWideChar(CP_ACP, MB_COMPOSITE, p, len, wFileName, cchWideChar);
+					wf_cliprdr_process_filename(clipboard, wFileName, cchWideChar);
+					free(wFileName);
+				}
+				else
+				{
+					rc = ERROR_INTERNAL_ERROR;
+					GlobalUnlock(stg_medium.hGlobal);
+					ReleaseStgMedium(&stg_medium);
+					goto exit;
+				}
 			}
 		}

 		GlobalUnlock(stg_medium.hGlobal);
 		ReleaseStgMedium(&stg_medium);
 	resp:
+		// size will not overflow, because size type is size_t (unsigned __int64)
 		size = 4 + clipboard->nFiles * sizeof(FILEDESCRIPTORW);
 		groupDsc = (FILEGROUPDESCRIPTORW *)malloc(size);

@ -2532,10 +2664,17 @@ wf_cliprdr_server_format_data_request(CliprdrClientContext *context,
 				globlemem = (char *)GlobalLock(hClipdata);
 				size = (int)GlobalSize(hClipdata);
 				buff = malloc(size);
-				CopyMemory(buff, globlemem, size);
+				if (buff)
+				{
+					CopyMemory(buff, globlemem, size);
+					rc = ERROR_SUCCESS;
+				}
+				else
+				{
+					rc = ERROR_INTERNAL_ERROR;
+				}
 				GlobalUnlock(hClipdata);
 				CloseClipboard();
-				rc = ERROR_SUCCESS;
 			}
 		}
 		else