From e36b89addba874fc9aa898a42e894ef37465cd30 Mon Sep 17 00:00:00 2001 From: Ged Murphy Date: Tue, 5 Dec 2017 22:13:02 +0000 Subject: [PATCH] [SERVMAN] - Avoid a potential race whereby the current service selection can change before the propsheet thread starts up - Cleanup the depends data, it doesn't need to be passed around the propsheet --- .../mscutils/servman/dependencies_tv1.c | 29 ++++--- .../mscutils/servman/dependencies_tv2.c | 24 +++--- base/applications/mscutils/servman/precomp.h | 17 +++-- .../applications/mscutils/servman/propsheet.c | 75 ++++++++++--------- .../mscutils/servman/propsheet_depends.c | 54 ++++++------- .../mscutils/servman/propsheet_recovery.c | 3 +- 6 files changed, 107 insertions(+), 95 deletions(-) diff --git a/base/applications/mscutils/servman/dependencies_tv1.c b/base/applications/mscutils/servman/dependencies_tv1.c index 5e764e51dbe..27d376ea544 100644 --- a/base/applications/mscutils/servman/dependencies_tv1.c +++ b/base/applications/mscutils/servman/dependencies_tv1.c @@ -10,8 +10,7 @@ #include "precomp.h" LPWSTR -TV1_GetDependants(PSERVICEPROPSHEET pDlgInfo, - SC_HANDLE hService) +TV1_GetDependants(SC_HANDLE hService) { LPQUERY_SERVICE_CONFIG lpServiceConfig; LPWSTR lpStr = NULL; @@ -80,7 +79,7 @@ TV1_GetDependants(PSERVICEPROPSHEET pDlgInfo, } VOID -TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, +TV1_AddDependantsToTree(PDEPENDDATA pDependData, HTREEITEM hParent, LPWSTR lpServiceName) { @@ -103,7 +102,7 @@ TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, if (hService) { /* Get a list of service dependents */ - lpDependants = TV1_GetDependants(pDlgInfo, hService); + lpDependants = TV1_GetDependants(hService); if (lpDependants) { lpStr = lpDependants; @@ -127,7 +126,7 @@ TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, } /* Add it */ - AddItemToTreeView(pDlgInfo->hDependsTreeView1, + AddItemToTreeView(pDependData->hDependsTreeView1, hParent, lpServiceConfig->lpDisplayName, lpStr, @@ -156,7 +155,7 @@ TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, /* Load the 'No dependencies' string */ AllocAndLoadString(&lpNoDepends, hInstance, IDS_NO_DEPENDS); - AddItemToTreeView(pDlgInfo->hDependsTreeView1, + AddItemToTreeView(pDependData->hDependsTreeView1, NULL, lpNoDepends, NULL, @@ -166,7 +165,7 @@ TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, LocalFree(lpNoDepends); /* Disable the window */ - EnableWindow(pDlgInfo->hDependsTreeView1, FALSE); + EnableWindow(pDependData->hDependsTreeView1, FALSE); } } @@ -178,25 +177,25 @@ TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, } BOOL -TV1_Initialize(PSERVICEPROPSHEET pDlgInfo, +TV1_Initialize(PDEPENDDATA pDependData, LPWSTR lpServiceName) { BOOL bRet = FALSE; /* Associate the imagelist with TV1 */ - pDlgInfo->hDependsTreeView1 = GetDlgItem(pDlgInfo->hDependsWnd, IDC_DEPEND_TREE1); - if (!pDlgInfo->hDependsTreeView1) + pDependData->hDependsTreeView1 = GetDlgItem(pDependData->hDependsWnd, IDC_DEPEND_TREE1); + if (!pDependData->hDependsTreeView1) { - ImageList_Destroy(pDlgInfo->hDependsImageList); - pDlgInfo->hDependsImageList = NULL; + ImageList_Destroy(pDependData->hDependsImageList); + pDependData->hDependsImageList = NULL; return FALSE; } - (void)TreeView_SetImageList(pDlgInfo->hDependsTreeView1, - pDlgInfo->hDependsImageList, + (void)TreeView_SetImageList(pDependData->hDependsTreeView1, + pDependData->hDependsImageList, TVSIL_NORMAL); /* Set the first items in the control */ - TV1_AddDependantsToTree(pDlgInfo, NULL, lpServiceName); + TV1_AddDependantsToTree(pDependData, NULL, lpServiceName); return bRet; } diff --git a/base/applications/mscutils/servman/dependencies_tv2.c b/base/applications/mscutils/servman/dependencies_tv2.c index a0f37b07c12..6f23197c0a5 100644 --- a/base/applications/mscutils/servman/dependencies_tv2.c +++ b/base/applications/mscutils/servman/dependencies_tv2.c @@ -118,7 +118,7 @@ TV2_GetDependants(LPWSTR lpServiceName, } VOID -TV2_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, +TV2_AddDependantsToTree(PDEPENDDATA pDependData, HTREEITEM hParent, LPWSTR lpServiceName) { @@ -138,7 +138,7 @@ TV2_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, bHasChildren = TV2_HasDependantServices(lpServiceStatus[i].lpServiceName); /* Add it */ - AddItemToTreeView(pDlgInfo->hDependsTreeView2, + AddItemToTreeView(pDependData->hDependsTreeView2, hParent, lpServiceStatus[i].lpDisplayName, lpServiceStatus[i].lpServiceName, @@ -158,7 +158,7 @@ TV2_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, /* Load the 'No dependencies' string */ AllocAndLoadString(&lpNoDepends, hInstance, IDS_NO_DEPENDS); - AddItemToTreeView(pDlgInfo->hDependsTreeView2, + AddItemToTreeView(pDependData->hDependsTreeView2, NULL, lpNoDepends, NULL, @@ -168,31 +168,31 @@ TV2_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, LocalFree(lpNoDepends); /* Disable the window */ - EnableWindow(pDlgInfo->hDependsTreeView2, FALSE); + EnableWindow(pDependData->hDependsTreeView2, FALSE); } } } BOOL -TV2_Initialize(PSERVICEPROPSHEET pDlgInfo, +TV2_Initialize(PDEPENDDATA pDependData, LPWSTR lpServiceName) { BOOL bRet = FALSE; /* Associate the imagelist with TV2 */ - pDlgInfo->hDependsTreeView2 = GetDlgItem(pDlgInfo->hDependsWnd, IDC_DEPEND_TREE2); - if (!pDlgInfo->hDependsTreeView2) + pDependData->hDependsTreeView2 = GetDlgItem(pDependData->hDependsWnd, IDC_DEPEND_TREE2); + if (!pDependData->hDependsTreeView2) { - ImageList_Destroy(pDlgInfo->hDependsImageList); - pDlgInfo->hDependsImageList = NULL; + ImageList_Destroy(pDependData->hDependsImageList); + pDependData->hDependsImageList = NULL; return FALSE; } - (void)TreeView_SetImageList(pDlgInfo->hDependsTreeView2, - pDlgInfo->hDependsImageList, + (void)TreeView_SetImageList(pDependData->hDependsTreeView2, + pDependData->hDependsImageList, TVSIL_NORMAL); /* Set the first items in the control */ - TV2_AddDependantsToTree(pDlgInfo, NULL, lpServiceName); + TV2_AddDependantsToTree(pDependData, NULL, lpServiceName); return bRet; } diff --git a/base/applications/mscutils/servman/precomp.h b/base/applications/mscutils/servman/precomp.h index 051634dc954..5ec27109f61 100644 --- a/base/applications/mscutils/servman/precomp.h +++ b/base/applications/mscutils/servman/precomp.h @@ -128,11 +128,18 @@ typedef struct _SERVICEPROPSHEET { PMAIN_WND_INFO Info; ENUM_SERVICE_STATUS_PROCESS *pService; + +} SERVICEPROPSHEET, *PSERVICEPROPSHEET; + +typedef struct _DEPENDDATA +{ + PSERVICEPROPSHEET pDlgInfo; HIMAGELIST hDependsImageList; HWND hDependsWnd; HWND hDependsTreeView1; HWND hDependsTreeView2; -} SERVICEPROPSHEET, *PSERVICEPROPSHEET; + +} DEPENDDATA, *PDEPENDDATA; HTREEITEM AddItemToTreeView(HWND hTreeView, HTREEITEM hRoot, LPWSTR lpDisplayName, LPWSTR lpServiceName, ULONG serviceType, BOOL bHasChildren); @@ -147,12 +154,12 @@ LPWSTR DisplayName, LPWSTR ServiceList); /* tv1_dependencies */ -BOOL TV1_Initialize(PSERVICEPROPSHEET pDlgInfo, LPWSTR lpServiceName); -VOID TV1_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, HTREEITEM hParent, LPWSTR lpServiceName); +BOOL TV1_Initialize(PDEPENDDATA pDependData, LPWSTR lpServiceName); +VOID TV1_AddDependantsToTree(PDEPENDDATA pDependData, HTREEITEM hParent, LPWSTR lpServiceName); /* tv2_dependencies */ -BOOL TV2_Initialize(PSERVICEPROPSHEET pDlgInfo, LPWSTR lpServiceName); -VOID TV2_AddDependantsToTree(PSERVICEPROPSHEET pDlgInfo, HTREEITEM hParent, LPWSTR lpServiceName); +BOOL TV2_Initialize(PDEPENDDATA pDependData, LPWSTR lpServiceName); +VOID TV2_AddDependantsToTree(PDEPENDDATA pDependData, HTREEITEM hParent, LPWSTR lpServiceName); BOOL TV2_HasDependantServices(LPWSTR lpServiceName); LPENUM_SERVICE_STATUS TV2_GetDependants(LPWSTR lpServiceName, LPDWORD lpdwCount); diff --git a/base/applications/mscutils/servman/propsheet.c b/base/applications/mscutils/servman/propsheet.c index 9557420ebbc..bd0ec245102 100644 --- a/base/applications/mscutils/servman/propsheet.c +++ b/base/applications/mscutils/servman/propsheet.c @@ -3,7 +3,7 @@ * LICENSE: GPL - See COPYING in the top level directory * FILE: base/applications/mscutils/servman/propsheet.c * PURPOSE: Property dialog box message handler - * COPYRIGHT: Copyright 2006-2007 Ged Murphy + * COPYRIGHT: Copyright 2006-2017 Ged Murphy * */ @@ -29,8 +29,20 @@ InitPropSheetPage(PROPSHEETPAGE *psp, VOID OpenPropSheet(PMAIN_WND_INFO Info) { + PSERVICEPROPSHEET pServicePropSheet; HANDLE hThread; - hThread = (HANDLE)_beginthreadex(NULL, 0, &PropSheetThread, Info, 0, NULL); + + pServicePropSheet = HeapAlloc(ProcessHeap, + 0, + sizeof(*pServicePropSheet)); + if (!pServicePropSheet) return; + + /* Set the current service in this calling thread to avoid + * it being updated before the thread is up */ + pServicePropSheet->pService = Info->pCurrentService; + pServicePropSheet->Info = Info; + + hThread = (HANDLE)_beginthreadex(NULL, 0, &PropSheetThread, pServicePropSheet, 0, NULL); if (hThread) { CloseHandle(hThread); @@ -40,65 +52,54 @@ OpenPropSheet(PMAIN_WND_INFO Info) unsigned int __stdcall PropSheetThread(void* Param) { + PSERVICEPROPSHEET pServicePropSheet; PROPSHEETHEADER psh; PROPSHEETPAGE psp[4]; - PSERVICEPROPSHEET pServicePropSheet; HWND hDlg = NULL; MSG Msg; - PMAIN_WND_INFO Info = (PMAIN_WND_INFO)Param; + pServicePropSheet = (PSERVICEPROPSHEET)Param; ZeroMemory(&psh, sizeof(PROPSHEETHEADER)); psh.dwSize = sizeof(PROPSHEETHEADER); psh.dwFlags = PSH_PROPSHEETPAGE | PSH_PROPTITLE | PSH_MODELESS; - psh.hwndParent = Info->hMainWnd; + psh.hwndParent = pServicePropSheet->Info->hMainWnd; psh.hInstance = hInstance; psh.hIcon = LoadIcon(hInstance, MAKEINTRESOURCE(IDI_SM_ICON)); - psh.pszCaption = Info->pCurrentService->lpDisplayName; + psh.pszCaption = pServicePropSheet->Info->pCurrentService->lpDisplayName; psh.nPages = sizeof(psp) / sizeof(PROPSHEETPAGE); psh.nStartPage = 0; psh.ppsp = psp; + /* Initialize the tabs */ + InitPropSheetPage(&psp[0], pServicePropSheet, IDD_DLG_GENERAL, GeneralPageProc); + InitPropSheetPage(&psp[1], pServicePropSheet, IDD_LOGON, LogonPageProc); + InitPropSheetPage(&psp[2], pServicePropSheet, IDD_RECOVERY, RecoveryPageProc); + InitPropSheetPage(&psp[3], pServicePropSheet, IDD_DLG_DEPEND, DependenciesPageProc); - pServicePropSheet = HeapAlloc(ProcessHeap, - 0, - sizeof(*pServicePropSheet)); - if (pServicePropSheet) + hDlg = (HWND)PropertySheetW(&psh); + if (hDlg) { - /* Save current service, as it could change while the dialog is open */ - pServicePropSheet->pService = Info->pCurrentService; - pServicePropSheet->Info = Info; - - InitPropSheetPage(&psp[0], pServicePropSheet, IDD_DLG_GENERAL, GeneralPageProc); - InitPropSheetPage(&psp[1], pServicePropSheet, IDD_LOGON, LogonPageProc); - InitPropSheetPage(&psp[2], pServicePropSheet, IDD_RECOVERY, RecoveryPageProc); - InitPropSheetPage(&psp[3], pServicePropSheet, IDD_DLG_DEPEND, DependenciesPageProc); - - hDlg = (HWND)PropertySheetW(&psh); - if (hDlg) + /* Pump the message queue */ + while (GetMessageW(&Msg, NULL, 0, 0)) { - /* Pump the message queue */ - while (GetMessageW(&Msg, NULL, 0, 0)) + if (PropSheet_GetCurrentPageHwnd(hDlg) == NULL) { + /* The user hit the ok / cancel button, pull it down */ + EnableWindow(pServicePropSheet->Info->hMainWnd, TRUE); + DestroyWindow(hDlg); + } - if (PropSheet_GetCurrentPageHwnd(hDlg) == NULL) - { - /* The user hit the ok / cancel button, pull it down */ - EnableWindow(Info->hMainWnd, TRUE); - DestroyWindow(hDlg); - } - - if (PropSheet_IsDialogMessage(hDlg, &Msg) != 0) - { - TranslateMessage(&Msg); - DispatchMessageW(&Msg); - } + if (PropSheet_IsDialogMessage(hDlg, &Msg) != 0) + { + TranslateMessage(&Msg); + DispatchMessageW(&Msg); } } - - HeapFree(GetProcessHeap(), 0, pServicePropSheet); } + HeapFree(GetProcessHeap(), 0, pServicePropSheet); + return (hDlg != NULL); } diff --git a/base/applications/mscutils/servman/propsheet_depends.c b/base/applications/mscutils/servman/propsheet_depends.c index b32111700e4..de1d6a55737 100644 --- a/base/applications/mscutils/servman/propsheet_depends.c +++ b/base/applications/mscutils/servman/propsheet_depends.c @@ -159,20 +159,20 @@ TreeView_GetItemText(HWND hTreeView, */ static VOID -InitDependPage(PSERVICEPROPSHEET pDlgInfo) +InitDependPage(PDEPENDDATA pDependData) { /* Initialize the image list */ - pDlgInfo->hDependsImageList = InitImageList(IDI_NODEPENDS, - IDI_DRIVER, - GetSystemMetrics(SM_CXSMICON), - GetSystemMetrics(SM_CXSMICON), - IMAGE_ICON); + pDependData->hDependsImageList = InitImageList(IDI_NODEPENDS, + IDI_DRIVER, + GetSystemMetrics(SM_CXSMICON), + GetSystemMetrics(SM_CXSMICON), + IMAGE_ICON); /* Set the first tree view */ - TV1_Initialize(pDlgInfo, pDlgInfo->pService->lpServiceName); + TV1_Initialize(pDependData, pDependData->pDlgInfo->pService->lpServiceName); /* Set the second tree view */ - TV2_Initialize(pDlgInfo, pDlgInfo->pService->lpServiceName); + TV2_Initialize(pDependData, pDependData->pDlgInfo->pService->lpServiceName); } /* @@ -185,12 +185,13 @@ DependenciesPageProc(HWND hwndDlg, WPARAM wParam, LPARAM lParam) { - PSERVICEPROPSHEET pDlgInfo; + + PDEPENDDATA pDependData; /* Get the window context */ - pDlgInfo = (PSERVICEPROPSHEET)GetWindowLongPtr(hwndDlg, - GWLP_USERDATA); - if (pDlgInfo == NULL && uMsg != WM_INITDIALOG) + pDependData = (PDEPENDDATA)GetWindowLongPtr(hwndDlg, + GWLP_USERDATA); + if (pDependData == NULL && uMsg != WM_INITDIALOG) { return FALSE; } @@ -199,16 +200,17 @@ DependenciesPageProc(HWND hwndDlg, { case WM_INITDIALOG: { - pDlgInfo = (PSERVICEPROPSHEET)(((LPPROPSHEETPAGE)lParam)->lParam); - if (pDlgInfo != NULL) + pDependData = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(DEPENDDATA)); + if (pDependData != NULL) { SetWindowLongPtr(hwndDlg, GWLP_USERDATA, - (LONG_PTR)pDlgInfo); + (LONG_PTR)pDependData); - pDlgInfo->hDependsWnd = hwndDlg; + pDependData->pDlgInfo = (PSERVICEPROPSHEET)(((LPPROPSHEETPAGE)lParam)->lParam); + pDependData->hDependsWnd = hwndDlg; - InitDependPage(pDlgInfo); + InitDependPage(pDependData); } } break; @@ -226,19 +228,19 @@ DependenciesPageProc(HWND hwndDlg, if (lpnmtv->hdr.idFrom == IDC_DEPEND_TREE1) { /* Has this node been expanded before */ - if (!TreeView_GetChild(pDlgInfo->hDependsTreeView1, lpnmtv->itemNew.hItem)) + if (!TreeView_GetChild(pDependData->hDependsTreeView1, lpnmtv->itemNew.hItem)) { /* It's not, add the children */ - TV1_AddDependantsToTree(pDlgInfo, lpnmtv->itemNew.hItem, (LPWSTR)lpnmtv->itemNew.lParam); + TV1_AddDependantsToTree(pDependData, lpnmtv->itemNew.hItem, (LPWSTR)lpnmtv->itemNew.lParam); } } else if (lpnmtv->hdr.idFrom == IDC_DEPEND_TREE2) { /* Has this node been expanded before */ - if (!TreeView_GetChild(pDlgInfo->hDependsTreeView2, lpnmtv->itemNew.hItem)) + if (!TreeView_GetChild(pDependData->hDependsTreeView2, lpnmtv->itemNew.hItem)) { /* It's not, add the children */ - TV2_AddDependantsToTree(pDlgInfo, lpnmtv->itemNew.hItem, (LPWSTR)lpnmtv->itemNew.lParam); + TV2_AddDependantsToTree(pDependData, lpnmtv->itemNew.hItem, (LPWSTR)lpnmtv->itemNew.lParam); } } } @@ -256,11 +258,13 @@ DependenciesPageProc(HWND hwndDlg, break; case WM_DESTROY: - DestroyTreeView(pDlgInfo->hDependsTreeView1); - DestroyTreeView(pDlgInfo->hDependsTreeView2); + DestroyTreeView(pDependData->hDependsTreeView1); + DestroyTreeView(pDependData->hDependsTreeView2); - if (pDlgInfo->hDependsImageList) - ImageList_Destroy(pDlgInfo->hDependsImageList); + if (pDependData->hDependsImageList) + ImageList_Destroy(pDependData->hDependsImageList); + + HeapFree(GetProcessHeap(), 0, pDependData); } return FALSE; diff --git a/base/applications/mscutils/servman/propsheet_recovery.c b/base/applications/mscutils/servman/propsheet_recovery.c index 8e161221515..59205f24c01 100644 --- a/base/applications/mscutils/servman/propsheet_recovery.c +++ b/base/applications/mscutils/servman/propsheet_recovery.c @@ -170,7 +170,8 @@ ShowFailureActions( { WCHAR szBuffer[256]; PWSTR startPtr, endPtr; - INT i, index, id, length; + INT index, id, length; + DWORD i; for (i = 0; i < min(pRecoveryData->pServiceFailure->cActions, 3); i++) {