[PATCH 1/2] server: Use generic kernel object list to store client device pointer.

[Jacek Caban]

Signed-off-by: Jacek Caban <jacek(a)codeweavers.com>
---
  dlls/ntoskrnl.exe/ntoskrnl.c | 37 +++++++++++--------------
  server/device.c              | 52 ++++++++++++++++++++++++------------
  server/protocol.def          |  7 ++---
  3 files changed, 53 insertions(+), 43 deletions(-)

[Zebediah Figura]

On 3/27/19 11:43 AM, Jacek Caban wrote:
> diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> index 77a610d7db..941ce89ab7 100644
> --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> @@ -1474,8 +1474,6 @@ static const WCHAR device_type_name[] = {'D','e','v','i','c','e',0};
>  static struct _OBJECT_TYPE device_type =
>  {
>      device_type_name,
> -    NULL,
> -    free_kernel_object
>  };
>
> POBJECT_TYPE IoDeviceObjectType = &device_type;
> @@ -1491,7 +1489,6 @@ NTSTATUS WINAPI IoCreateDevice( DRIVER_OBJECT *driver, ULONG ext_size,
>  {
>      NTSTATUS status;
>      DEVICE_OBJECT *device;
> -    HANDLE handle = 0;
>      HANDLE manager = get_device_manager();
>
> TRACE( "(%p, %u, %s, %u, %x, %u, %p)\n",
> @@ -1500,34 +1497,32 @@ NTSTATUS WINAPI IoCreateDevice( DRIVER_OBJECT *driver, ULONG ext_size,
>      if (!(device = alloc_kernel_object( IoDeviceObjectType, NULL, sizeof(DEVICE_OBJECT) + ext_size, 1 )))
>          return STATUS_NO_MEMORY;
>
> +    device->DriverObject    = driver;
> +    device->DeviceExtension = device + 1;
> +    device->DeviceType      = type;
> +    device->StackSize       = 1;
> +
> +    device->NextDevice   = driver->DeviceObject;
> +    driver->DeviceObject = device;
> +
>      SERVER_START_REQ( create_device )
>      {
> -        req->access     = 0;
> -        req->attributes = 0;
>          req->rootdir    = 0;
>          req->manager    = wine_server_obj_handle( manager );
>          req->user_ptr   = wine_server_client_ptr( device );
>          if (name) wine_server_add_data( req, name->Buffer, name->Length );
> -        if (!(status = wine_server_call( req ))) handle = wine_server_ptr_handle( reply->handle );
> +        status = wine_server_call( req );
>      }
>      SERVER_END_REQ;
>
> -    if (status == STATUS_SUCCESS)
> +    if (status)
>      {
> -        device->DriverObject    = driver;
> -        device->DeviceExtension = device + 1;
> -        device->DeviceType      = type;
> -        device->StackSize       = 1;
> -        device->Reserved        = handle;
> -
> -        device->NextDevice   = driver->DeviceObject;
> -        driver->DeviceObject = device;
> -
> -        *ret_device = device;
> +        free_kernel_object( device );
> +        return status;
>      }
> -    else free_kernel_object( device );
>
> -    return status;
> +    *ret_device = device;
> +    return STATUS_SUCCESS;
>  }

What's the reason for moving DEVICE_OBJECT initialization before 
create_device call? In particular this won't work correctly if 
create_device fails, as then driver->DeviceObject will point to an 
invalid device.

[Jacek Caban]

On 4/6/19 7:22 PM, Zebediah Figura wrote:
> On 3/27/19 11:43 AM, Jacek Caban wrote:
> > diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> > index 77a610d7db..941ce89ab7 100644
> > --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> > +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> > @@ -1474,8 +1474,6 @@ static const WCHAR device_type_name[] = 
> > {'D','e','v','i','c','e',0};
> >  static struct _OBJECT_TYPE device_type =
> >  {
> >      device_type_name,
> > -    NULL,
> > -    free_kernel_object
> >  };
> >
> >  POBJECT_TYPE IoDeviceObjectType = &device_type;
> > @@ -1491,7 +1489,6 @@ NTSTATUS WINAPI IoCreateDevice( DRIVER_OBJECT 
> > *driver, ULONG ext_size,
> >  {
> >      NTSTATUS status;
> >      DEVICE_OBJECT *device;
> > -    HANDLE handle = 0;
> >      HANDLE manager = get_device_manager();
> >
> >      TRACE( "(%p, %u, %s, %u, %x, %u, %p)\n",
> > @@ -1500,34 +1497,32 @@ NTSTATUS WINAPI IoCreateDevice( DRIVER_OBJECT 
> > *driver, ULONG ext_size,
> >      if (!(device = alloc_kernel_object( IoDeviceObjectType, NULL, 
> > sizeof(DEVICE_OBJECT) + ext_size, 1 )))
> >          return STATUS_NO_MEMORY;
> >
> > +    device->DriverObject    = driver;
> > +    device->DeviceExtension = device + 1;
> > +    device->DeviceType      = type;
> > +    device->StackSize       = 1;
> > +
> > +    device->NextDevice   = driver->DeviceObject;
> > +    driver->DeviceObject = device;
> > +
> >      SERVER_START_REQ( create_device )
> >      {
> > -        req->access     = 0;
> > -        req->attributes = 0;
> >          req->rootdir    = 0;
> >          req->manager    = wine_server_obj_handle( manager );
> >          req->user_ptr   = wine_server_client_ptr( device );
> >          if (name) wine_server_add_data( req, name->Buffer, 
> > name->Length );
> > -        if (!(status = wine_server_call( req ))) handle = 
> > wine_server_ptr_handle( reply->handle );
> > +        status = wine_server_call( req );
> >      }
> >      SERVER_END_REQ;
> >
> > -    if (status == STATUS_SUCCESS)
> > +    if (status)
> >      {
> > -        device->DriverObject    = driver;
> > -        device->DeviceExtension = device + 1;
> > -        device->DeviceType      = type;
> > -        device->StackSize       = 1;
> > -        device->Reserved        = handle;
> > -
> > -        device->NextDevice   = driver->DeviceObject;
> > -        driver->DeviceObject = device;
> > -
> > -        *ret_device = device;
> > +        free_kernel_object( device );
> > +        return status;
> >      }
> > -    else free_kernel_object( device );
> >
> > -    return status;
> > +    *ret_device = device;
> > +    return STATUS_SUCCESS;
> >  }
>
> What's the reason for moving DEVICE_OBJECT initialization before 
> create_device call?

It was mostly about thread safety, but it's largely a theoretical 
problem here. After create_device server call, other threads may already 
use the object so it's too late to initialize.
> In particular this won't work correctly if create_device fails, as 
> then driver->DeviceObject will point to an invalid device.

You're right, this part is wrong. I will send a fix.


Thanks,

Jacek