[ros-diffs] [reactos] 01/01: [UMPNPMGR][USETUP] Fix the way device-install events are queued and dequeued. Fixes CORE-16103.

[Hermès Bélusca-Maïto]

https://git.reactos.org/?p=reactos.git;a=commitdiff;h=ffc96d26ec297a6f9feec…

commit ffc96d26ec297a6f9feec7c1e526efefa08809e0
Author:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
AuthorDate: Mon Sep 28 22:38:55 2020 +0200
Commit:     Hermès Bélusca-Maïto <hermes.belusca-maito(a)reactos.org>
CommitDate: Thu Oct 1 01:52:09 2020 +0200

    [UMPNPMGR][USETUP] Fix the way device-install events are queued and dequeued. Fixes
CORE-16103.
    
    Dedicated to the hard work of Joachim Henze! xD
    
    This reverts part of commit 043a98dd (see also commit b2aeafca).
    
    Contrary to what I assumed in commit 043a98dd (and was also assumed in
    the older commit b2aeafca), we cannot use the singled-linked lists to
    queue and dequeue the PnP device-install events, because:
    
    - the events must be treated from the oldest to the newest ones, for
      consistency, otherwise this creates problems, as shown by e.g. CORE-16103;
    
    - the system singled-linked lists only offer access to the top of the
      list (like a stack) instead of to both the top and the bottom of the
      list, as would be required for a queue. Using the SLISTs would mean
      that only the newest-received events would be treated first, while the
      oldest (which were the first received) events would be treated last,
      and this is wrong.
    
    Therefore one must use e.g. the standard doubly-linked list. Also, using
    locked operations (insertion & removal) on the list of device-install
    events is necessary, because these operations are done concurrently by
    two different threads: PnpEventThread() and DeviceInstallThread().
    Since the interlocked linked list functions are not available in user-mode,
    we need to use instead locking access through e.g. a mutex.
---
 base/services/umpnpmgr/install.c  | 14 ++++++---
 base/services/umpnpmgr/precomp.h  |  8 +++--
 base/services/umpnpmgr/umpnpmgr.c | 38 +++++++++++++++++-------
 base/setup/usetup/devinst.c       | 61 ++++++++++++++++++++++++++++-----------
 4 files changed, 86 insertions(+), 35 deletions(-)

diff --git a/base/services/umpnpmgr/install.c b/base/services/umpnpmgr/install.c
index 4c876e490ef..80ef8258900 100644
--- a/base/services/umpnpmgr/install.c
+++ b/base/services/umpnpmgr/install.c
@@ -22,7 +22,7 @@
  * FILE:             base/services/umpnpmgr/install.c
  * PURPOSE:          Device installer
  * PROGRAMMER:       Eric Kohl (eric.kohl(a)reactos.org)
- *                   Herv� Poussineau (hpoussin(a)reactos.org)
+ *                   Hervé Poussineau (hpoussin(a)reactos.org)
  *                   Colin Finck (colin(a)reactos.org)
  */
 
@@ -40,7 +40,9 @@ HANDLE hUserToken = NULL;
 HANDLE hInstallEvent = NULL;
 HANDLE hNoPendingInstalls = NULL;
 
-SLIST_HEADER DeviceInstallListHead;
+/* Device-install event list */
+HANDLE hDeviceInstallListMutex;
+LIST_ENTRY DeviceInstallListHead;
 HANDLE hDeviceInstallListNotEmpty;
 
 
@@ -354,7 +356,7 @@ DWORD
 WINAPI
 DeviceInstallThread(LPVOID lpParameter)
 {
-    PSLIST_ENTRY ListEntry;
+    PLIST_ENTRY ListEntry;
     DeviceInstallParams* Params;
     BOOL showWizard;
 
@@ -366,7 +368,11 @@ DeviceInstallThread(LPVOID lpParameter)
 
     while (TRUE)
     {
-        ListEntry = InterlockedPopEntrySList(&DeviceInstallListHead);
+        /* Dequeue the next oldest device-install event */
+        WaitForSingleObject(hDeviceInstallListMutex, INFINITE);
+        ListEntry = (IsListEmpty(&DeviceInstallListHead)
+                        ? NULL : RemoveHeadList(&DeviceInstallListHead));
+        ReleaseMutex(hDeviceInstallListMutex);
 
         if (ListEntry == NULL)
         {
diff --git a/base/services/umpnpmgr/precomp.h b/base/services/umpnpmgr/precomp.h
index f7d7b4f9f72..d24a760884a 100644
--- a/base/services/umpnpmgr/precomp.h
+++ b/base/services/umpnpmgr/precomp.h
@@ -35,8 +35,8 @@
 
 typedef struct
 {
-    SLIST_ENTRY ListEntry;
-    WCHAR DeviceIds[1];
+    LIST_ENTRY ListEntry;
+    WCHAR DeviceIds[ANYSIZE_ARRAY];
 } DeviceInstallParams;
 
 /* install.c */
@@ -45,7 +45,9 @@ extern HANDLE hUserToken;
 extern HANDLE hInstallEvent;
 extern HANDLE hNoPendingInstalls;
 
-extern SLIST_HEADER DeviceInstallListHead;
+/* Device-install event list */
+extern HANDLE hDeviceInstallListMutex;
+extern LIST_ENTRY DeviceInstallListHead;
 extern HANDLE hDeviceInstallListNotEmpty;
 
 BOOL
diff --git a/base/services/umpnpmgr/umpnpmgr.c b/base/services/umpnpmgr/umpnpmgr.c
index 0195f73813c..826efbde66b 100644
--- a/base/services/umpnpmgr/umpnpmgr.c
+++ b/base/services/umpnpmgr/umpnpmgr.c
@@ -104,13 +104,18 @@ PnpEventThread(LPVOID lpParameter)
             DeviceIdLength = lstrlenW(PnpEvent->TargetDevice.DeviceIds);
             if (DeviceIdLength)
             {
-                /* Queue device install (will be dequeued by DeviceInstallThread) */
+                /* Allocate a new device-install event */
                 len = FIELD_OFFSET(DeviceInstallParams, DeviceIds) + (DeviceIdLength + 1)
* sizeof(WCHAR);
                 Params = HeapAlloc(GetProcessHeap(), 0, len);
                 if (Params)
                 {
                     wcscpy(Params->DeviceIds, PnpEvent->TargetDevice.DeviceIds);
-                    InterlockedPushEntrySList(&DeviceInstallListHead,
&Params->ListEntry);
+
+                    /* Queue the event (will be dequeued by DeviceInstallThread) */
+                    WaitForSingleObject(hDeviceInstallListMutex, INFINITE);
+                    InsertTailList(&DeviceInstallListHead,
&Params->ListEntry);
+                    ReleaseMutex(hDeviceInstallListMutex);
+
                     SetEvent(hDeviceInstallListNotEmpty);
                 }
             }
@@ -413,26 +418,37 @@ InitializePnPManager(VOID)
         return dwError;
     }
 
+    hNoPendingInstalls = CreateEventW(NULL,
+                                      TRUE,
+                                      FALSE,
+                                     
L"Global\\PnP_No_Pending_Install_Events");
+    if (hNoPendingInstalls == NULL)
+    {
+        dwError = GetLastError();
+        DPRINT1("Could not create the Pending-Install Event! (Error %lu)\n",
dwError);
+        return dwError;
+    }
+
+    /*
+     * Initialize the device-install event list
+     */
+
     hDeviceInstallListNotEmpty = CreateEventW(NULL, FALSE, FALSE, NULL);
     if (hDeviceInstallListNotEmpty == NULL)
     {
         dwError = GetLastError();
-        DPRINT1("Could not create the Event! (Error %lu)\n", dwError);
+        DPRINT1("Could not create the List Event! (Error %lu)\n", dwError);
         return dwError;
     }
 
-    hNoPendingInstalls = CreateEventW(NULL,
-                                      TRUE,
-                                      FALSE,
-                                     
L"Global\\PnP_No_Pending_Install_Events");
-    if (hNoPendingInstalls == NULL)
+    hDeviceInstallListMutex = CreateMutexW(NULL, FALSE, NULL);
+    if (hDeviceInstallListMutex == NULL)
     {
         dwError = GetLastError();
-        DPRINT1("Could not create the Event! (Error %lu)\n", dwError);
+        DPRINT1("Could not create the List Mutex! (Error %lu)\n", dwError);
         return dwError;
     }
-
-    InitializeSListHead(&DeviceInstallListHead);
+    InitializeListHead(&DeviceInstallListHead);
 
     /* Query the SuppressUI registry value and cache it for our whole lifetime */
     GetBooleanRegValue(HKEY_LOCAL_MACHINE,
diff --git a/base/setup/usetup/devinst.c b/base/setup/usetup/devinst.c
index d9fc5c2a576..c9ca64dc949 100644
--- a/base/setup/usetup/devinst.c
+++ b/base/setup/usetup/devinst.c
@@ -25,12 +25,14 @@ static HANDLE hNoPendingInstalls = NULL;
 static HANDLE hPnpThread = NULL;
 static HANDLE hDeviceInstallThread = NULL;
 
-static SLIST_HEADER DeviceInstallListHead;
+/* Device-install event list */
+static HANDLE hDeviceInstallListMutex = NULL;
+static LIST_ENTRY DeviceInstallListHead;
 static HANDLE hDeviceInstallListNotEmpty = NULL;
 
 typedef struct
 {
-    SLIST_ENTRY ListEntry;
+    LIST_ENTRY ListEntry;
     WCHAR DeviceIds[ANYSIZE_ARRAY];
 } DeviceInstallParams;
 
@@ -363,13 +365,17 @@ static ULONG NTAPI
 DeviceInstallThread(IN PVOID Parameter)
 {
     HINF hSetupInf = *(HINF*)Parameter;
-    PSLIST_ENTRY ListEntry;
+    PLIST_ENTRY ListEntry;
     DeviceInstallParams* Params;
     LARGE_INTEGER Timeout;
 
     for (;;)
     {
-        ListEntry = RtlInterlockedPopEntrySList(&DeviceInstallListHead);
+        /* Dequeue the next oldest device-install event */
+        NtWaitForSingleObject(hDeviceInstallListMutex, FALSE, NULL);
+        ListEntry = (IsListEmpty(&DeviceInstallListHead)
+                        ? NULL : RemoveHeadList(&DeviceInstallListHead));
+        NtReleaseMutant(hDeviceInstallListMutex, NULL);
 
         if (ListEntry == NULL)
         {
@@ -454,18 +460,23 @@ PnpEventThread(IN PVOID Parameter)
             ULONG len;
             ULONG DeviceIdLength;
 
-            DPRINT("Device enumerated event: %S\n",
PnpEvent->TargetDevice.DeviceIds);
+            DPRINT("Device enumerated: %S\n",
PnpEvent->TargetDevice.DeviceIds);
 
             DeviceIdLength = wcslen(PnpEvent->TargetDevice.DeviceIds);
             if (DeviceIdLength)
             {
-                /* Queue device install (will be dequeued by DeviceInstallThread) */
+                /* Allocate a new device-install event */
                 len = FIELD_OFFSET(DeviceInstallParams, DeviceIds) + (DeviceIdLength + 1)
* sizeof(WCHAR);
                 Params = RtlAllocateHeap(ProcessHeap, 0, len);
                 if (Params)
                 {
                     wcscpy(Params->DeviceIds, PnpEvent->TargetDevice.DeviceIds);
-                    RtlInterlockedPushEntrySList(&DeviceInstallListHead,
&Params->ListEntry);
+
+                    /* Queue the event (will be dequeued by DeviceInstallThread) */
+                    NtWaitForSingleObject(hDeviceInstallListMutex, FALSE, NULL);
+                    InsertTailList(&DeviceInstallListHead,
&Params->ListEntry);
+                    NtReleaseMutant(hDeviceInstallListMutex, NULL);
+
                     NtSetEvent(hDeviceInstallListNotEmpty, NULL);
                 }
                 else
@@ -559,29 +570,41 @@ InitializeUserModePnpManager(
     UNICODE_STRING EnumU =
RTL_CONSTANT_STRING(L"\\Registry\\Machine\\SYSTEM\\CurrentControlSet\\Enum");
     UNICODE_STRING ServicesU =
RTL_CONSTANT_STRING(L"\\Registry\\Machine\\SYSTEM\\CurrentControlSet\\Services");
 
-    Status = NtCreateEvent(&hDeviceInstallListNotEmpty,
+    Status = NtCreateEvent(&hNoPendingInstalls,
                            EVENT_ALL_ACCESS,
                            NULL,
-                           SynchronizationEvent,
+                           NotificationEvent,
                            FALSE);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("Could not create the event! (Status 0x%08lx)\n", Status);
+        DPRINT1("Could not create the Pending-Install Event! (Status
0x%08lx)\n", Status);
         goto Failure;
     }
 
-    Status = NtCreateEvent(&hNoPendingInstalls,
+    /*
+     * Initialize the device-install event list
+     */
+
+    Status = NtCreateEvent(&hDeviceInstallListNotEmpty,
                            EVENT_ALL_ACCESS,
                            NULL,
-                           NotificationEvent,
+                           SynchronizationEvent,
                            FALSE);
     if (!NT_SUCCESS(Status))
     {
-        DPRINT1("Could not create the event! (Status 0x%08lx)\n", Status);
+        DPRINT1("Could not create the List Event! (Status 0x%08lx)\n",
Status);
         goto Failure;
     }
 
-    RtlInitializeSListHead(&DeviceInstallListHead);
+    Status = NtCreateMutant(&hDeviceInstallListMutex,
+                            MUTANT_ALL_ACCESS,
+                            NULL, FALSE);
+    if (!NT_SUCCESS(Status))
+    {
+        DPRINT1("Could not create the List Mutex! (Status 0x%08lx)\n",
Status);
+        goto Failure;
+    }
+    InitializeListHead(&DeviceInstallListHead);
 
     InitializeObjectAttributes(&ObjectAttributes, &EnumU, OBJ_CASE_INSENSITIVE,
NULL, NULL);
     Status = NtOpenKey(&hEnumKey, KEY_QUERY_VALUE, &ObjectAttributes);
@@ -653,14 +676,18 @@ Failure:
         NtClose(hEnumKey);
     hEnumKey = NULL;
 
-    if (hNoPendingInstalls)
-        NtClose(hNoPendingInstalls);
-    hNoPendingInstalls = NULL;
+    if (hDeviceInstallListMutex)
+        NtClose(hDeviceInstallListMutex);
+    hDeviceInstallListMutex = NULL;
 
     if (hDeviceInstallListNotEmpty)
         NtClose(hDeviceInstallListNotEmpty);
     hDeviceInstallListNotEmpty = NULL;
 
+    if (hNoPendingInstalls)
+        NtClose(hNoPendingInstalls);
+    hNoPendingInstalls = NULL;
+
     return Status;
 }