[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 2/4] ntoskrnl.exe: Implement loading plug and play devices
From:       Sebastian Lackner <sebastian () fds-team ! de>
Date:       2016-08-31 7:22:44
Message-ID: 81df3c82-dd6d-0446-0d5a-ab8a42d2f2b8 () fds-team ! de
[Download RAW message or body]

On 29.08.2016 20:49, Aric Stewart wrote:
> Signed-off-by: Aric Stewart <aric@codeweavers.com>
> ---
> dlls/ntoskrnl.exe/pnp_manager.c | 150 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 150 insertions(+)
> 
> 
> 
> 0002-ntoskrnl.exe-Implement-loading-plug-and-play-devices.txt
> 
> 
> diff --git a/dlls/ntoskrnl.exe/pnp_manager.c b/dlls/ntoskrnl.exe/pnp_manager.c
> index 5b42a0a..f1f3109 100644
> --- a/dlls/ntoskrnl.exe/pnp_manager.c
> +++ b/dlls/ntoskrnl.exe/pnp_manager.c
> @@ -17,6 +17,7 @@
> * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301, USA
> */
> 
> +#define NONAMELESSUNION
> #include <stdarg.h>
> #include "ntstatus.h"
> #define WIN32_NO_STATUS
> @@ -27,6 +28,7 @@
> #include "cfgmgr32.h"
> #include "winsvc.h"
> #include "winreg.h"
> +#include "wine/unicode.h"
> #include "wine/debug.h"
> #include "wine/list.h"
> 
> @@ -34,6 +36,8 @@ WINE_DEFAULT_DEBUG_CHANNEL(plugplay);
> 
> #define MAX_SERVICE_NAME 260
> 
> +typedef NTSTATUS WINAPI (*pAddDevice)(DRIVER_OBJECT *DriverObject, DEVICE_OBJECT \
> *PhysicalDeviceObject);

My Win10 header files contain DRIVER_EXTENSION with the appropriate definition, which
would make the casts unnecessary.

> +
> typedef struct _device_driver {
> struct list entry;
> 
> @@ -105,6 +109,143 @@ static device_driver* register_device_driver(const WCHAR \
> *drivername, const WCHA return ptr;
> }
> 
> +static NTSTATUS WINAPI internalComplete(DEVICE_OBJECT *deviceObject, IRP *irp,
> +    void *context )
> +{
> +    SetEvent(irp->UserEvent);
> +    return STATUS_MORE_PROCESSING_REQUIRED;
> +}
> +
> +static NTSTATUS get_device_id(DEVICE_OBJECT *device, BUS_QUERY_ID_TYPE type, WCHAR \
> **id) +{
> +    NTSTATUS status;
> +    IO_STACK_LOCATION *irpsp;
> +    IO_STATUS_BLOCK irp_status;
> +    IRP *irp;
> +    HANDLE event = CreateEventA(NULL, FALSE, FALSE, NULL);
> +
> +    irp = IoBuildSynchronousFsdRequest(IRP_MJ_PNP, device, NULL, 0, NULL, NULL, \
> &irp_status); +    if (irp == NULL)
> +        return STATUS_NO_MEMORY;

The return path here will leak the event handle. The same issue was recently fixed in \
hidclass.sys. Also I'm wondering a bit, do we need to keep this identical code at \
both places?

> +
> +    irp->UserEvent = event;
> +    irpsp = IoGetNextIrpStackLocation(irp);
> +    irpsp->MinorFunction = IRP_MN_QUERY_ID;
> +    irpsp->Parameters.QueryId.IdType = type;
> +    irpsp->CompletionRoutine = internalComplete;
> +    irpsp->Control = SL_INVOKE_ON_SUCCESS | SL_INVOKE_ON_ERROR;
> +
> +    IoCallDriver(device, irp);
> +
> +    if (irp->IoStatus.u.Status == STATUS_PENDING)
> +        WaitForSingleObject(event, INFINITE);
> +
> +    *id = (WCHAR*)irp->IoStatus.Information;
> +    status = irp->IoStatus.u.Status;
> +    IoCompleteRequest(irp, IO_NO_INCREMENT );
> +    CloseHandle(event);
> +
> +    return status;
> +}
> +
> +static void handle_bus_relations(DEVICE_OBJECT *device)
> +{
> +    static const WCHAR szDriverW[] = {'\\','D','r','i','v','e','r','\\',0};
> +
> +    NTSTATUS status;
> +    WCHAR *id, *idptr;
> +    device_driver *driver = NULL;
> +    WCHAR fullname[MAX_PATH];
> +    DRIVER_OBJECT *driver_obj;
> +    UNICODE_STRING driverName;
> +
> +    TRACE("Device %p\n", device);
> +
> +    /* We could(should?) do a full IRP_MN_QUERY_DEVICE_RELATIONS query,
> +     * but we dont have to, We have the DEVICE_OBJECT of the new device
> +     * so we can simply handle the process here */
> +
> +    status = get_device_id(device, BusQueryCompatibleIDs, &id);
> +    if (status != ERROR_SUCCESS || !id)
> +    {
> +        ERR("Failed to get device IDs\n");
> +        return;
> +    }
> +
> +    idptr = id;
> +    do {
> +        device_driver *ptr;
> +
> +        TRACE("Checking for id %s\n",debugstr_w(idptr));
> +
> +        /* Check loaded drivers */
> +        LIST_FOR_EACH_ENTRY(ptr, &device_drivers, device_driver, entry)
> +        {
> +            int i;
> +            for (i = 0; i < ptr->id_count; i++)
> +            {
> +                if (lstrcmpW(idptr, ptr->matching_ids[i]) == 0)
> +                {
> +                    driver = ptr;
> +                    break;

Is it intentional that this does not abort the outer loop?

> +                }
> +            }
> +        }
> +        idptr += (lstrlenW(idptr) + 1);
> +    } while (!driver && *idptr != 0);
> +
> +    HeapFree(GetProcessHeap(), 0, id);
> +
> +    if (!driver)
> +    {
> +        ERR("No matching driver found for device\n");
> +        return;
> +    }
> +
> +    strcpyW(fullname, szDriverW);
> +    strcatW(fullname, driver->driver_name);
> +    RtlInitUnicodeString(&driverName, fullname);
> +
> +    if (!driver->loaded)
> +    {
> +        static const WCHAR servicesW[] = {'\\','R','e','g','i','s','t','r','y',
> +                                  '\\','M','a','c','h','i','n','e',
> +                                  '\\','S','y','s','t','e','m',
> +                                  \
> '\\','C','u','r','r','e','n','t','C','o','n','t','r','o','l','S','e','t', +         \
> '\\','S','e','r','v','i','c','e','s', +                                  '\\',0};
> +        WCHAR driverKey[MAX_PATH];
> +        UNICODE_STRING driverPath;
> +
> +        strcpyW(driverKey, servicesW);
> +        strcatW(driverKey, driver->driver_name);
> +        RtlInitUnicodeString(&driverPath, driverKey);
> +        if (ZwLoadDriver(&driverPath) != STATUS_SUCCESS)
> +        {
> +            ERR("Failed to load driver %s\n",debugstr_w(driver->driver_name));
> +            return;
> +        }
> +        driver->loaded = TRUE;
> +    }
> +
> +    if (ObReferenceObjectByName(&driverName, OBJ_CASE_INSENSITIVE, NULL,
> +        0, NULL, KernelMode, NULL, (void**)&driver_obj) != STATUS_SUCCESS)
> +    {
> +        ERR("Failed to locate loaded driver\n");
> +        return;
> +    }
> +
> +    status = ((pAddDevice)driver_obj->DriverExtension->AddDevice)(driver_obj, \
> device);

It would be better to handle the lack of a AddDevice routine.

> +
> +    ObDereferenceObject(driver_obj);
> +
> +    if (status != STATUS_SUCCESS)
> +    {
> +        ERR("AddDevice failed\n");
> +        return;
> +    }
> +}
> +
> static void initialize_driver_store(void)
> {
> static const WCHAR criticalW[] =
> @@ -181,5 +322,14 @@ VOID WINAPI IoInvalidateDeviceRelations(DEVICE_OBJECT \
> *DeviceObject, DEVICE_RELA initialized = TRUE;
> initialize_driver_store();
> }
> +
> +    switch (Type)
> +    {
> +        case BusRelations:
> +            handle_bus_relations(DeviceObject);
> +            break;
> +        default:
> +            FIXME("Unhandled Relation %i\n", Type);
> +    }
> LeaveCriticalSection(&pnp_cs);
> }
> 
> 
> 


[prev in list] [next in list] [prev in thread] [next in thread]