Re: [PATCH] winedevice: Remove static driver_obj variable

[Sebastian Lackner]

On 05.08.2016 14:26, Aric Stewart wrote:
> Signed-off-by: Aric Stewart
>
> programs/winedevice/device.c | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> 0001-winedevice-Remove-static-driver_obj-variable.txt
>
> diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
> index e31318c..2744b9d5 100644
> --- a/programs/winedevice/device.c
> +++ b/programs/winedevice/device.c
> @@ -42,7 +42,16 @@ extern NTSTATUS CDECL wine_ntoskrnl_main_loop( HANDLE stop_event );
>
> static SERVICE_STATUS_HANDLE service_handle;
>  static HANDLE stop_event;
> -static DRIVER_OBJECT *driver_obj;
> -
>
> +static const WCHAR driverW[] = {'\','D','r','i','v','e','r','\',0};
> +extern NTSTATUS WINAPI ObReferenceObjectByName( UNICODE_STRING *ObjectName,
> -
>                                     ULONG Attributes,
> -
>                                     ACCESS_STATE *AccessState,
> -
>                                     ACCESS_MASK DesiredAccess,
> -
>                                     POBJECT_TYPE ObjectType,
> -
>                                     KPROCESSOR_MODE AccessMode,
> -
>                                     void *ParseContext,
> -
>                                     void **Object);

It might be better to add this to a public header file. On Windows its in a
different header file, but Wine already has a couple of ObReference* functions
in include/ddk/wdm.h. You could add it there, too.
> /* find the LDR_MODULE corresponding to the driver module */
>  static LDR_MODULE *find_ldr_module( HMODULE module )
> @@ -225,7 +234,6 @@ static NTSTATUS WINAPI init_driver( DRIVER_OBJECT *driver_object, UNICODE_STRING
>      if (!module)
>          return STATUS_DLL_INIT_FAILED;
> -
>
> driver_obj = driver_object;
> driver_object->DriverSection = find_ldr_module( module );
>
> nt = RtlImageNtHeader( module );
>
> @@ -253,9 +261,26 @@ static NTSTATUS WINAPI init_driver( DRIVER_OBJECT *driver_object, UNICODE_STRING
>  }
>
> /* call the driver unload function */
> -static void unload_driver( DRIVER_OBJECT *driver_obj )
> +static void unload_driver( const WCHAR *driver_name )
>  {
> - LDR_MODULE *ldr = driver_obj->DriverSection;
> - DRIVER_OBJECT *driver_obj;
> - LDR_MODULE *ldr;
> - UNICODE_STRING drv_name;
> - WCHAR *str;
> -
> - str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver_name)*sizeof(WCHAR) );
> - lstrcpyW( str, driverW);
> - lstrcatW( str, driver_name );
> - RtlInitUnicodeString( &drv_name, str );
> -
> - if (ObReferenceObjectByName( &drv_name, OBJ_CASE_INSENSITIVE, NULL,
> -
>    0, NULL, KernelMode, NULL, (void**)&driver_obj ) != STATUS_SUCCESS)

To avoid putting together the string twice, you could already call this function
immediately after loading.

Also, in theory you would have to release the reference obtained by
"ObReferenceObjectByName" using ObDereferenceObject. Since there is no refcounting
yet its fine to skip it, but it might be important at some later point.
> - {
> -
>    ERR("Failed to locate loaded driver (%s)\n", wine_dbgstr_w(driver_name));
> -
>    return;

You are leaking the drv_name variable here and at the end of the function.
> -
>
> }
> -
> -
>
> ldr = driver_obj->DriverSection;
>
> if (driver_obj->DriverUnload)
> {
>
> @@ -276,7 +301,6 @@ static void unload_driver( DRIVER_OBJECT *driver_obj )
>
> static NTSTATUS create_driver(const WCHAR *driver_name)
>  {
> - static const WCHAR driverW[] = {'\','D','r','i','v','e','r','\',0};
> UNICODE_STRING drv_name;
> NTSTATUS status;
> WCHAR *str;
>
> @@ -351,7 +375,7 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
>          SetServiceStatus( service_handle, &status );
>      wine_ntoskrnl_main_loop( stop_event );
> -
>    unload_driver( driver_obj );
> -
>    unload_driver( driver_name );
> }
>    else WINE_ERR( "driver %s failed to load\n", wine_dbgstr_w(driver_name) );

[PATCH] winedevice: Remove static driver_obj variable

[Aric Stewart]

On 8/5/16 10:04 AM, Sebastian Lackner wrote:
> On 05.08.2016 14:26, Aric Stewart wrote:
> > Signed-off-by: Aric Stewart
> >
> > programs/winedevice/device.c | 36 ++++++++++++++++++++++++++++++------
> >  1 file changed, 30 insertions(+), 6 deletions(-)
> >
> > 0001-winedevice-Remove-static-driver_obj-variable.txt
> >
> > diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
> > index e31318c..2744b9d5 100644
> > --- a/programs/winedevice/device.c
> > +++ b/programs/winedevice/device.c
> > @@ -42,7 +42,16 @@ extern NTSTATUS CDECL wine_ntoskrnl_main_loop( HANDLE stop_event );
> >
> > static SERVICE_STATUS_HANDLE service_handle;
> >  static HANDLE stop_event;
> > -static DRIVER_OBJECT *driver_obj;
> > -
> >
> > +static const WCHAR driverW[] = {'\','D','r','i','v','e','r','\',0};
> > +extern NTSTATUS WINAPI ObReferenceObjectByName( UNICODE_STRING *ObjectName,
> > -
> >                                     ULONG Attributes,
> > -
> >                                     ACCESS_STATE *AccessState,
> > -
> >                                     ACCESS_MASK DesiredAccess,
> > -
> >                                     POBJECT_TYPE ObjectType,
> > -
> >                                     KPROCESSOR_MODE AccessMode,
> > -
> >                                     void *ParseContext,
> > -
> >                                     void **Object);
>
> It might be better to add this to a public header file. On Windows its in a
> different header file, but Wine already has a couple of ObReference* functions
> in include/ddk/wdm.h. You could add it there, too.
> > /* find the LDR_MODULE corresponding to the driver module */
> >  static LDR_MODULE *find_ldr_module( HMODULE module )
> > @@ -225,7 +234,6 @@ static NTSTATUS WINAPI init_driver( DRIVER_OBJECT *driver_object, UNICODE_STRING
> >      if (!module)
> >          return STATUS_DLL_INIT_FAILED;
> > -
> >
> > driver_obj = driver_object;
> > driver_object->DriverSection = find_ldr_module( module );
> >
> > nt = RtlImageNtHeader( module );
> >
> > @@ -253,9 +261,26 @@ static NTSTATUS WINAPI init_driver( DRIVER_OBJECT *driver_object, UNICODE_STRING
> >  }
> >
> > /* call the driver unload function */
> > -static void unload_driver( DRIVER_OBJECT *driver_obj )
> > +static void unload_driver( const WCHAR *driver_name )
> >  {
> > - LDR_MODULE *ldr = driver_obj->DriverSection;
> > - DRIVER_OBJECT *driver_obj;
> > - LDR_MODULE *ldr;
> > - UNICODE_STRING drv_name;
> > - WCHAR *str;
> > -
> > - str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver_name)*sizeof(WCHAR) );
> > - lstrcpyW( str, driverW);
> > - lstrcatW( str, driver_name );
> > - RtlInitUnicodeString( &drv_name, str );
> > -
> > - if (ObReferenceObjectByName( &drv_name, OBJ_CASE_INSENSITIVE, NULL,
> > -
> >    0, NULL, KernelMode, NULL, (void**)&driver_obj ) != STATUS_SUCCESS)
>
> To avoid putting together the string twice, you could already call this function
> immediately after loading.

I dont quite understand what you are meaning here...

-aric
> Also, in theory you would have to release the reference obtained by
> "ObReferenceObjectByName" using ObDereferenceObject. Since there is no refcounting
> yet its fine to skip it, but it might be important at some later point.
> > - {
> > -
> >    ERR("Failed to locate loaded driver (%s)\n", wine_dbgstr_w(driver_name));
> > -
> >    return;
>
> You are leaking the drv_name variable here and at the end of the function.
> > -
> >
> > }
> > -
> > -
> >
> > ldr = driver_obj->DriverSection;
> >
> > if (driver_obj->DriverUnload)
> > {
> >
> > @@ -276,7 +301,6 @@ static void unload_driver( DRIVER_OBJECT *driver_obj )
> >
> > static NTSTATUS create_driver(const WCHAR *driver_name)
> >  {
> > - static const WCHAR driverW[] = {'\','D','r','i','v','e','r','\',0};
> > UNICODE_STRING drv_name;
> > NTSTATUS status;
> > WCHAR *str;
> >
> > @@ -351,7 +375,7 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
> >          SetServiceStatus( service_handle, &status );
> >      wine_ntoskrnl_main_loop( stop_event );
> > -
> >    unload_driver( driver_obj );
> > -
> >    unload_driver( driver_name );
> > }
> >    else WINE_ERR( "driver %s failed to load\n", wine_dbgstr_w(driver_name) );

[PATCH] winedevice: Remove static driver_obj variable

[Sebastian Lackner]

On 05.08.2016 21:01, Aric Stewart wrote:
> > > -
> > > - str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver_name)*sizeof(WCHAR) );
> > > - lstrcpyW( str, driverW);
> > > - lstrcatW( str, driver_name );
> > > - RtlInitUnicodeString( &drv_name, str );
> > > -
> > > - if (ObReferenceObjectByName( &drv_name, OBJ_CASE_INSENSITIVE, NULL,
> > > -
> > >    0, NULL, KernelMode, NULL, (void**)&driver_obj ) != STATUS_SUCCESS)
> >
> > To avoid putting together the string twice, you could already call this function
> > immediately after loading.
>
> I dont quite understand what you are meaning here...
>
> -aric

What I mean is that you could move this to create_driver(), where you already have drv_name.
If something went wrong (and the driver is not tracked correctly) you could abort immediately, then.

[PATCH] winedevice: Remove static driver_obj variable

[Aric Stewart]

On 8/5/16 2:36 PM, Sebastian Lackner wrote:
> On 05.08.2016 21:01, Aric Stewart wrote:
> > > > -
> > > > - str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver_name)*sizeof(WCHAR) );
> > > > - lstrcpyW( str, driverW);
> > > > - lstrcatW( str, driver_name );
> > > > - RtlInitUnicodeString( &drv_name, str );
> > > > -
> > > > - if (ObReferenceObjectByName( &drv_name, OBJ_CASE_INSENSITIVE, NULL,
> > > > -
> > > >    0, NULL, KernelMode, NULL, (void**)&driver_obj ) != STATUS_SUCCESS)
> > >
> > > To avoid putting together the string twice, you could already call this function
> > > immediately after loading.
> >
> > I dont quite understand what you are meaning here...
> >
> > -aric
>
> What I mean is that you could move this to create_driver(), where you already have drv_name.
> If something went wrong (and the driver is not tracked correctly) you could abort immediately, then.

And pass out the DRIVER_OBJECT?  It complicates the multiple driver case looking forward but we can solve that problem when we get to if it you feel strongly about that change.

-aric