Re: [PATCH 1/4] ntoskrnl.exe: Implement IoInvalidateDeviceRelations to initialize device driver store

[Sebastian Lackner]

On 29.08.2016 20:49, Aric Stewart wrote:
> We are not handling installing drivers just yet, so we just make use of the
> critical device store. These are drivers that, on Windows, are automatically
> loaded on boot every time.  This database is maintained in
> [CurrentControlSet/Control/CriticalDeviceDatabase] The keys in this registry key
> represent a HardwareID that match a device. We query the IDs from the bus
> device’s BusQueryHardwareIDs and if we find a match (from most specific to most
> general) we look at that registry key. The CriticalDeviceDatabase entry
> specify a [Service] value representing the driver.
>
> Signed-off-by: Aric Stewart <aric(a)codeweavers.com>
> ---
>  dlls/ntoskrnl.exe/Makefile.in       |   3 +-
>  dlls/ntoskrnl.exe/ntoskrnl.exe.spec |   2 +-
>  dlls/ntoskrnl.exe/pnp_manager.c     | 185 ++++++++++++++++++++++++++++++++++++
>  include/ddk/wdm.h                   |   1 +
>  4 files changed, 189 insertions(+), 2 deletions(-)
>  create mode 100644 dlls/ntoskrnl.exe/pnp_manager.c

At some places the patch does not really use a consistent style yet (for example VOID <-> void,
placement of "*" inbetween of type and variable name, "sz" prefix for variables, ...), however
I will focus mainly on the technical aspects in this review.
> 0001-ntoskrnl.exe-Implement-IoInvalidateDeviceRelations-to-.txt
>
> diff --git a/dlls/ntoskrnl.exe/Makefile.in b/dlls/ntoskrnl.exe/Makefile.in
> index b459c54..2fa768b 100644
> --- a/dlls/ntoskrnl.exe/Makefile.in
> +++ b/dlls/ntoskrnl.exe/Makefile.in
> @@ -4,6 +4,7 @@ IMPORTS   = advapi32
>
> C_SRCS = \
>   instr.c \
> -	ntoskrnl.c
> +	ntoskrnl.c \
> +	pnp_manager.c
>
> RC_SRCS = ntoskrnl.rc
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> index 119d406..c94b902 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.exe.spec
> @@ -400,7 +400,7 @@
>  @ stdcall IoInitializeIrp(ptr long long)
>  @ stdcall IoInitializeRemoveLockEx(ptr long long long long)
>  @ stdcall IoInitializeTimer(ptr ptr ptr)
> -@ stub IoInvalidateDeviceRelations
> +@ stdcall IoInvalidateDeviceRelations(ptr long)
>  @ stub IoInvalidateDeviceState
>  @ stub IoIsFileOriginRemote
>  @ stub IoIsOperationSynchronous
> diff --git a/dlls/ntoskrnl.exe/pnp_manager.c b/dlls/ntoskrnl.exe/pnp_manager.c
> new file mode 100644
> index 0000000..5b42a0a
> --- /dev/null
> +++ b/dlls/ntoskrnl.exe/pnp_manager.c
> @@ -0,0 +1,185 @@
> +/*  HID plug and play Manager like functionality
> + *
> + * Copyright 2016 CodeWeavers, Aric Stewart
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> + */
> +
> +#include <stdarg.h>
> +#include "ntstatus.h"
> +#define WIN32_NO_STATUS
> +#include "windef.h"
> +#include "winbase.h"
> +#include "winternl.h"
> +#include "ddk/wdm.h"
> +#include "cfgmgr32.h"
> +#include "winsvc.h"

I don't think this include is required anymore.
> +#include "winreg.h"
> +#include "wine/debug.h"
> +#include "wine/list.h"
> +
> +WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> +
> +#define MAX_SERVICE_NAME 260
> +
> +typedef struct _device_driver {
> +    struct list entry;
> +
> +    BOOL loaded;
> +
> +    WCHAR **matching_ids;
> +    INT id_count;
> +
> +    WCHAR *driver_name;
> +} device_driver;
> +
> +struct list device_drivers = LIST_INIT(device_drivers);
> +static BOOL initialized = FALSE;
> +
> +static CRITICAL_SECTION pnp_cs;
> +static CRITICAL_SECTION_DEBUG critsect_debug =
> +{
> +    0, 0, &pnp_cs,
> +    { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
> +      0, 0, { (DWORD_PTR)(__FILE__ ": pnp_cs") }
> +};
> +static CRITICAL_SECTION pnp_cs = { &critsect_debug, -1, 0, 0, 0, 0 };
> +
> +static inline LPWSTR strdupW(LPCWSTR src)
> +{
> +    LPWSTR dest;
> +
> +    if (!src)
> +        return NULL;
> +
> +    dest = HeapAlloc(GetProcessHeap(), 0, (lstrlenW(src) + 1) * sizeof(WCHAR));
> +    if (dest)
> +        lstrcpyW(dest, src);

For new code it would be better to follow the current Wine style guidelines,
which means no LP* types and strlenW/strcpyW without l prefix.
> +
> +    return dest;
> +}
> +
> +static device_driver* register_device_driver(const WCHAR *drivername, const WCHAR* match)
> +{
> +    device_driver *ptr;
> +
> +    LIST_FOR_EACH_ENTRY(ptr, &device_drivers, device_driver, entry)
> +    {
> +        if (lstrcmpW(drivername, ptr->driver_name) == 0)
> +        {
> +            int i;
> +            /* Check matching ids */
> +            for (i = 0; i < ptr->id_count; i++)
> +                if (lstrcmpW(match, ptr->matching_ids[i]) == 0)
> +                    return ptr;
> +            /* Add a new matching id, this should be reletivly rare,
> +               no exponental growth needed */
> +            ptr->matching_ids = HeapReAlloc(GetProcessHeap(), 0, ptr->matching_ids, sizeof(WCHAR*) * (ptr->id_count+1));
> +            ptr->matching_ids[ptr->id_count] = strdupW(match);
> +            ptr->id_count++;

If it is very rare, wouldn't it be better to add them as separate driver entries then?
This would simplify the code and avoid nested loops at some places. The current way of
organizing the driver db also has the disadvantage that the same "matching id" could be
added for multiple drivers.
> +
> +            return ptr;
> +        }
> +    }
> +
> +    ptr = HeapAlloc(GetProcessHeap(), HEAP_ZERO_MEMORY, sizeof(*ptr));
> +    ptr->driver_name = strdupW(drivername);
> +    ptr->loaded = FALSE;
> +    ptr->matching_ids = HeapAlloc(GetProcessHeap(), 0, sizeof(WCHAR*));
> +    ptr->matching_ids[0] = strdupW(match);
> +    ptr->id_count = 1;
> +    list_add_tail(&device_drivers, &ptr->entry);
> +
> +    return ptr;
> +}
> +
> +static void initialize_driver_store(void)
> +{
> +    static const WCHAR criticalW[] =
> +        { 'S','y','s','t','e','m','\\',
> +          'C','u','r','r','e','n','t','C','o','n','t','r','o','l','S','e','t','\\',
> +          'C','o','n','t','r','o','l', '\\',
> +          'C','r','i','t','i','c','a','l','D','e','v','i','c','e','D','a','t','a','b','a','s','e', 0};
> +    static const WCHAR serviceW[] = {'S','e','r','v','i','c','e',0};
> +    HKEY hkey_root;
> +    WCHAR szMatch[MAX_DEVICE_ID_LEN];
> +    int i;
> +
> +    if (RegOpenKeyW(HKEY_LOCAL_MACHINE, criticalW, &hkey_root))
> +    {
> +        TRACE("No drivers found\n");
> +        return;
> +    }
> +
> +    for (i = 0; TRUE; i++)
> +    {
> +        HKEY hkey_service;
> +        DWORD rc;
> +        device_driver *driver;
> +        DWORD len = 0;
> +        WCHAR szService[MAX_SERVICE_NAME];
> +
> +        rc = RegEnumKeyW(hkey_root, i, szMatch, MAX_DEVICE_ID_LEN);
> +        if (rc == ERROR_NO_MORE_ITEMS)
> +            break;
> +
> +        if (rc != 0)
> +        {
> +            ERR("Error %d reading key %d - skipping\n", rc, i);
> +            continue;
> +        }
> +
> +        if (RegOpenKeyW(hkey_root, szMatch, &hkey_service))
> +        {
> +            ERR("Error opening key %s - skipping\n",debugstr_w(szMatch));
> +            continue;
> +        }
> +
> +        len = sizeof(szService);
> +        rc = RegQueryValueExW(hkey_service, serviceW, NULL, NULL, (BYTE*)szService, &len);

It might be useful to check the type of the registry key here.
> +        if (rc != ERROR_SUCCESS)
> +        {
> +            ERR("Error querying service from key %s - skipping\n",debugstr_w(szMatch));
> +            RegCloseKey(hkey_service);
> +            continue;
> +        }
> +        RegCloseKey(hkey_service);
> +
> +        TRACE("Registering service '%s' to match '%s'\n", debugstr_w(szService), debugstr_w(szMatch));
> +
> +        driver = register_device_driver(szService, szMatch);
> +        if (!driver)
> +            ERR("failed to load driver %s\n", debugstr_w(szService));

The message is actually a bit misleading. It will only occur for duplicate entries,
which should not happen anyway.
> +
> +    }
> +
> +    RegCloseKey(hkey_root);
> +}
> +
> +/***********************************************************************
> + *           IoInvalidateDeviceRelations(NTOSKRNL.EXE.@)
> + */
> +VOID WINAPI IoInvalidateDeviceRelations(DEVICE_OBJECT *DeviceObject, DEVICE_RELATION_TYPE Type)
> +{
> +    TRACE("(%p %i)\n", DeviceObject, Type);
> +
> +    EnterCriticalSection(&pnp_cs);
> +    if (!initialized)
> +    {
> +        initialized = TRUE;
> +        initialize_driver_store();
> +    }
> +    LeaveCriticalSection(&pnp_cs);
> +}
> diff --git a/include/ddk/wdm.h b/include/ddk/wdm.h
> index 68694af..bd3b323 100644
> --- a/include/ddk/wdm.h
> +++ b/include/ddk/wdm.h
> @@ -1233,6 +1233,7 @@ NTSTATUS  WINAPI IoGetDeviceProperty(PDEVICE_OBJECT,DEVICE_REGISTRY_PROPERTY,ULO
>  PVOID     WINAPI IoGetDriverObjectExtension(PDRIVER_OBJECT,PVOID);
>  PDEVICE_OBJECT WINAPI IoGetRelatedDeviceObject(PFILE_OBJECT);
>  void      WINAPI IoInitializeIrp(IRP*,USHORT,CCHAR);
> +void      WINAPI IoInvalidateDeviceRelations(PDEVICE_OBJECT,DEVICE_RELATION_TYPE);
>  VOID      WINAPI IoInitializeRemoveLockEx(PIO_REMOVE_LOCK,ULONG,ULONG,ULONG,ULONG);
>  NTSTATUS  WINAPI IoWMIRegistrationControl(PDEVICE_OBJECT,ULONG);