[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: [PATCH 1/2] ntoskrnl.exe: Track drivers created with IoCreateDriver
From: Sebastian Lackner <sebastian () fds-team ! de>
Date: 2016-02-29 3:39:51
Message-ID: 56D3BD87.5050404 () fds-team ! de
[Download RAW message or body]
On 29.02.2016 04:38, Aric Stewart wrote:
> Thanks for the review!
>
>
> On 2/28/16 9:30 PM, Sebastian Lackner wrote:
> > On 26.02.2016 21:22, Aric Stewart wrote:
> > > Signed-off-by: Aric Stewart <aric@codeweavers.com>
> > > ---
> > > dlls/ntoskrnl.exe/ntoskrnl.c | 64 +++++++++++++++++++++++++++++++++-----------
> > > 1 file changed, 48 insertions(+), 16 deletions(-)
> > >
> > >
> > >
> > > 0002-ntoskrnl.exe-Track-drivers-created-with-IoCreateDriver.txt
> > >
> > >
> > > diff --git a/dlls/ntoskrnl.exe/ntoskrnl.c b/dlls/ntoskrnl.exe/ntoskrnl.c
> > > index 3bee2bf..49c8cab 100644
> > > --- a/dlls/ntoskrnl.exe/ntoskrnl.c
> > > +++ b/dlls/ntoskrnl.exe/ntoskrnl.c
> > > @@ -72,6 +72,16 @@ static DWORD request_thread;
> > > static DWORD client_tid;
> > > static DWORD client_pid;
> > >
> > > +typedef struct _created_driver {
> > > + struct list entry;
> > > +
> > > + UNICODE_STRING driver_name;
> > > + DRIVER_OBJECT *driver_obj;
> > > + DRIVER_EXTENSION *driver_extension;
> >
> > Isn't it possible to add the DRIVER_OBJECT and DRIVER_EXTENSION struct directly
> > (without pointer), to avoid multiple memory allocations?
> >
>
> Yeah, That would be just fine, to have it part of the base structure instead of a \
> separate pointer.
> > I would also suggest to use a more meaningful name than "created_driver", and
> > you also don't really need the typedef. Something like "struct wine_driver"
> > would make more sense for example, but probably you have a better idea. ;)
> >
> > > +} created_driver;
> > > +
> > > +struct list created_drivers = LIST_INIT(created_drivers);
> > > +
> > > #ifdef __i386__
> > > #define DEFINE_FASTCALL1_ENTRYPOINT( name ) \
> > > __ASM_STDCALL_FUNC( name, 4, \
> > > @@ -818,36 +828,47 @@ PIRP WINAPI IoBuildSynchronousFsdRequest(ULONG majorfunc, \
> > > PDEVICE_OBJECT device,
> > > */
> > > NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, PDRIVER_INITIALIZE init )
> > > {
> > > - DRIVER_OBJECT *driver;
> > > - DRIVER_EXTENSION *extension;
> > > + created_driver *driver;
> > > NTSTATUS status;
> > >
> > > TRACE("(%s, %p)\n", debugstr_us(name), init);
> > >
> > > if (!(driver = RtlAllocateHeap( GetProcessHeap(), HEAP_ZERO_MEMORY,
> > > - sizeof(*driver) + sizeof(*extension) )))
> > > + sizeof(*driver) )))
> > > + return STATUS_NO_MEMORY;
> > > +
> > > + if (!(driver->driver_obj = RtlAllocateHeap( GetProcessHeap(), \
> > > HEAP_ZERO_MEMORY, + \
> > > sizeof(*driver->driver_obj) + sizeof(*driver->driver_extension) ))) + {
> > > + RtlFreeHeap( GetProcessHeap(), 0, driver );
> > > return STATUS_NO_MEMORY;
> > > + }
> > >
> > > - if ((status = RtlDuplicateUnicodeString( 1, name, &driver->DriverName )))
> > > + if ((status = RtlDuplicateUnicodeString( 1, name, &driver->driver_name )))
> > > {
> > > + RtlFreeHeap( GetProcessHeap(), 0, driver->driver_obj );
> > > RtlFreeHeap( GetProcessHeap(), 0, driver );
> > > return status;
> > > }
> > >
> > > - extension = (DRIVER_EXTENSION *)(driver + 1);
> > > - driver->Size = sizeof(*driver);
> > > - driver->DriverInit = init;
> > > - driver->DriverExtension = extension;
> > > - extension->DriverObject = driver;
> > > - extension->ServiceKeyName = driver->DriverName;
> > > + driver->driver_extension = (DRIVER_EXTENSION *)((BYTE*)driver->driver_obj \
> > > + sizeof(*driver->driver_obj)); + driver->driver_obj->DriverName = \
> > > driver->driver_name; + driver->driver_obj->Size = \
> > > sizeof(*driver->driver_obj); + driver->driver_obj->DriverInit = init;
> > > + driver->driver_obj->DriverExtension = driver->driver_extension;
> > > + driver->driver_extension->DriverObject = driver->driver_obj;
> > > + driver->driver_extension->ServiceKeyName = driver->driver_name;
> > >
> > > - status = driver->DriverInit( driver, name );
> > > + status = driver->driver_obj->DriverInit( driver->driver_obj, name );
> > >
> > > if (status)
> > > {
> > > - RtlFreeUnicodeString( &driver->DriverName );
> > > + RtlFreeUnicodeString( &driver->driver_name );
> > > + RtlFreeHeap( GetProcessHeap(), 0, driver->driver_obj );
> > > RtlFreeHeap( GetProcessHeap(), 0, driver );
> > > }
> > > +
> > > + list_add_tail( &created_drivers, &driver->entry );
> >
> > Such a global list will need locking.
> >
>
> You think that a basic critical section would be sufficient or do you think other \
> locking would be better?
A basic critical section should be fine.
>
>
> -aric
>
> > > return status;
> > > }
> > >
> > > @@ -855,12 +876,23 @@ NTSTATUS WINAPI IoCreateDriver( UNICODE_STRING *name, \
> > > PDRIVER_INITIALIZE init ) \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > \
> > > * IoDeleteDriver (NTOSKRNL.EXE.@)
> > > */
> > > -void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver )
> > > +void WINAPI IoDeleteDriver( DRIVER_OBJECT *driver_object )
> > > {
> > > - TRACE("(%p)\n", driver);
> > > + created_driver *driver, *next;
> > > +
> > > + TRACE("(%p)\n", driver_object);
> > > +
> >
> > By putting the driver_object in the same struct, you could use CONTAINING_RECORD \
> > here. Also, a lock will be required while unlinking the driver from the list.
> >
> > > + LIST_FOR_EACH_ENTRY_SAFE(driver, next, &created_drivers, created_driver, \
> > > entry) + {
> > > + if (driver->driver_obj == driver_object)
> > > + {
> > > + list_remove( &driver->entry );
> > > + RtlFreeUnicodeString( &driver->driver_name);
> > > + break;
> > > + }
> > > + }
> > >
> > > - RtlFreeUnicodeString( &driver->DriverName );
> > > - RtlFreeHeap( GetProcessHeap(), 0, driver );
> > > + RtlFreeHeap( GetProcessHeap(), 0, driver_object );
> > > }
> > >
> > >
> > >
> > >
> > >
> >
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic