Re: [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) );