[PATCH 2/3] winedevice: Make driver (un)loading synchronous.
Zebediah Figura
z.figura12 at gmail.com
Fri Aug 24 22:42:57 CDT 2018
This essentially reverts 440482d2ef31333d1bc3ce15b0aad4ceec60466c.
440482d was aimed towards making it possible to load multiple drivers
asynchronously, as well as to allow reentrancy. Unfortunately, asynchronicity
is incorrect, as demonstrated by bug 38836, and some trivial testing shows
that the SCM database lock is held for the entirety of the driver entry and
exit routines, and that StartService() and ControlService() block until they
complete. 5726824 and dd2624a nullified the effects of 440482d, making driver
loading all but synchronous (with the exception of the added 30 second
timeout, but this is actually incorrect: drivers can block indefinitely).
This patch therefore does not change any behaviour, but rather removes the
use of threadpools and "async" functions, essentially reverting back to the
implementation prior to 440482d. The incidental change to unload_driver()
made by that patch (viz. never to unload a driver without a DriverUnload()
routine) is kept.
Signed-off-by: Zebediah Figura <z.figura12 at gmail.com>
---
(Please feel free to keep as much or as little of the above as is appropriate,
if any.)
programs/winedevice/device.c | 223 ++++++++++---------------------------------
1 file changed, 51 insertions(+), 172 deletions(-)
diff --git a/programs/winedevice/device.c b/programs/winedevice/device.c
index fa8b268..9ee094a 100644
--- a/programs/winedevice/device.c
+++ b/programs/winedevice/device.c
@@ -45,20 +45,14 @@ extern NTSTATUS CDECL wine_ntoskrnl_main_loop( HANDLE stop_event );
static const WCHAR winedeviceW[] = {'w','i','n','e','d','e','v','i','c','e',0};
static SERVICE_STATUS_HANDLE service_handle;
-static PTP_CLEANUP_GROUP cleanup_group;
static SC_HANDLE manager_handle;
-static BOOL shutdown_in_progress;
static HANDLE stop_event;
-#define EVENT_STARTED 0
-#define EVENT_ERROR 1
-
struct wine_driver
{
struct wine_rb_entry entry;
SERVICE_STATUS_HANDLE handle;
- HANDLE events[2];
DRIVER_OBJECT *driver_obj;
WCHAR name[1];
};
@@ -71,15 +65,6 @@ static int wine_drivers_rb_compare( const void *key, const struct wine_rb_entry
static struct wine_rb_tree wine_drivers = { wine_drivers_rb_compare };
-static CRITICAL_SECTION drivers_cs;
-static CRITICAL_SECTION_DEBUG critsect_debug =
-{
- 0, 0, &drivers_cs,
- { &critsect_debug.ProcessLocksList, &critsect_debug.ProcessLocksList },
- 0, 0, { (DWORD_PTR)(__FILE__ ": drivers_cs") }
-};
-static CRITICAL_SECTION drivers_cs = { &critsect_debug, -1, 0, 0, 0, 0 };
-
/* find the LDR_MODULE corresponding to the driver module */
static LDR_MODULE *find_ldr_module( HMODULE module )
{
@@ -299,11 +284,21 @@ static void set_service_status( SERVICE_STATUS_HANDLE handle, DWORD state, DWORD
SetServiceStatus( handle, &status );
}
-static void WINAPI async_unload_driver( PTP_CALLBACK_INSTANCE instance, void *context )
+/* call the driver unload function */
+static void unload_driver( struct wine_rb_entry *entry, void *context )
{
- struct wine_driver *driver = context;
+ struct wine_driver *driver = WINE_RB_ENTRY_VALUE( entry, struct wine_driver, entry );
DRIVER_OBJECT *driver_obj = driver->driver_obj;
- LDR_MODULE *ldr;
+ LDR_MODULE *ldr = driver_obj->DriverSection;
+
+ if (!driver_obj->DriverUnload)
+ {
+ TRACE( "driver %s does not support unloading\n", wine_dbgstr_w(driver->name) );
+ return;
+ }
+
+ TRACE( "stopping driver %s\n", wine_dbgstr_w(driver->name) );
+ set_service_status( driver->handle, SERVICE_STOP_PENDING, 0 );
TRACE_(relay)( "\1Call driver unload %p (obj=%p)\n", driver_obj->DriverUnload, driver_obj );
@@ -311,7 +306,6 @@ static void WINAPI async_unload_driver( PTP_CALLBACK_INSTANCE instance, void *co
TRACE_(relay)( "\1Ret driver unload %p (obj=%p)\n", driver_obj->DriverUnload, driver_obj );
- ldr = driver_obj->DriverSection;
FreeLibrary( ldr->BaseAddress );
IoDeleteDriver( driver_obj );
ObDereferenceObject( driver_obj );
@@ -321,106 +315,15 @@ static void WINAPI async_unload_driver( PTP_CALLBACK_INSTANCE instance, void *co
HeapFree( GetProcessHeap(), 0, driver );
}
-/* call the driver unload function */
-static NTSTATUS unload_driver( struct wine_rb_entry *entry, BOOL destroy )
-{
- TP_CALLBACK_ENVIRON environment;
- struct wine_driver *driver = WINE_RB_ENTRY_VALUE( entry, struct wine_driver, entry );
- DRIVER_OBJECT *driver_obj = driver->driver_obj;
-
- if (!driver_obj)
- {
- TRACE( "driver %s has not finished loading yet\n", wine_dbgstr_w(driver->name) );
- return STATUS_UNSUCCESSFUL;
- }
- if (!driver_obj->DriverUnload)
- {
- TRACE( "driver %s does not support unloading\n", wine_dbgstr_w(driver->name) );
- return STATUS_UNSUCCESSFUL;
- }
-
- TRACE( "stopping driver %s\n", wine_dbgstr_w(driver->name) );
- set_service_status( driver->handle, SERVICE_STOP_PENDING, 0 );
-
- if (destroy)
- {
- async_unload_driver( NULL, driver );
- return STATUS_SUCCESS;
- }
-
- wine_rb_remove( &wine_drivers, &driver->entry );
-
- memset( &environment, 0, sizeof(environment) );
- environment.Version = 1;
- environment.CleanupGroup = cleanup_group;
-
- /* don't block the service control handler */
- if (!TrySubmitThreadpoolCallback( async_unload_driver, driver, &environment ))
- async_unload_driver( NULL, driver );
-
- return STATUS_SUCCESS;
-}
-
-static void WINAPI async_create_driver( PTP_CALLBACK_INSTANCE instance, void *context )
-{
- static const WCHAR driverW[] = {'\\','D','r','i','v','e','r','\\',0};
- struct wine_driver *driver = context;
- DRIVER_OBJECT *driver_obj;
- UNICODE_STRING drv_name;
- NTSTATUS status;
- WCHAR *str;
-
- if (!(str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver->name)*sizeof(WCHAR) )))
- goto error;
-
- lstrcpyW( str, driverW);
- lstrcatW( str, driver->name );
- RtlInitUnicodeString( &drv_name, str );
-
- status = IoCreateDriver( &drv_name, init_driver );
- if (status != STATUS_SUCCESS)
- {
- ERR( "failed to create driver %s: %08x\n", debugstr_w(driver->name), status );
- RtlFreeUnicodeString( &drv_name );
- goto error;
- }
-
- status = ObReferenceObjectByName( &drv_name, OBJ_CASE_INSENSITIVE, NULL,
- 0, NULL, KernelMode, NULL, (void **)&driver_obj );
- RtlFreeUnicodeString( &drv_name );
- if (status != STATUS_SUCCESS)
- {
- ERR( "failed to locate driver %s: %08x\n", debugstr_w(driver->name), status );
- goto error;
- }
-
- SetEvent(driver->events[EVENT_STARTED]);
-
- EnterCriticalSection( &drivers_cs );
- driver->driver_obj = driver_obj;
- set_service_status( driver->handle, SERVICE_RUNNING,
- SERVICE_ACCEPT_STOP | SERVICE_ACCEPT_SHUTDOWN );
- LeaveCriticalSection( &drivers_cs );
- return;
-
-error:
- SetEvent(driver->events[EVENT_ERROR]);
- EnterCriticalSection( &drivers_cs );
- wine_rb_remove( &wine_drivers, &driver->entry );
- LeaveCriticalSection( &drivers_cs );
-
- set_service_status( driver->handle, SERVICE_STOPPED, 0 );
- CloseServiceHandle( (void *)driver->handle );
- HeapFree( GetProcessHeap(), 0, driver );
-}
-
/* load a driver and notify services.exe about the status change */
static NTSTATUS create_driver( const WCHAR *driver_name )
{
- TP_CALLBACK_ENVIRON environment;
+ static const WCHAR driverW[] = {'\\','D','r','i','v','e','r','\\',0};
struct wine_driver *driver;
+ UNICODE_STRING drv_name;
+ NTSTATUS status;
DWORD length;
- DWORD ret;
+ WCHAR *str;
length = FIELD_OFFSET( struct wine_driver, name[strlenW(driver_name) + 1] );
if (!(driver = HeapAlloc( GetProcessHeap(), 0, length )))
@@ -442,63 +345,46 @@ static NTSTATUS create_driver( const WCHAR *driver_name )
return STATUS_UNSUCCESSFUL;
}
+ if (!(str = HeapAlloc( GetProcessHeap(), 0, sizeof(driverW) + strlenW(driver->name)*sizeof(WCHAR) )))
+ {
+ status = STATUS_NO_MEMORY;
+ goto error;
+ }
+
TRACE( "starting driver %s\n", wine_dbgstr_w(driver_name) );
set_service_status( driver->handle, SERVICE_START_PENDING, 0 );
- memset( &environment, 0, sizeof(environment) );
- environment.Version = 1;
- environment.CleanupGroup = cleanup_group;
+ lstrcpyW( str, driverW);
+ lstrcatW( str, driver->name );
+ RtlInitUnicodeString( &drv_name, str );
- driver->events[EVENT_STARTED] = CreateEventW(NULL, TRUE, FALSE, NULL);
- driver->events[EVENT_ERROR] = CreateEventW(NULL, TRUE, FALSE, NULL);
-
- /* don't block the service control handler */
- if (!TrySubmitThreadpoolCallback( async_create_driver, driver, &environment ))
- async_create_driver( NULL, driver );
-
- /* Windows wait 30 Seconds */
- ret = WaitForMultipleObjects(2, driver->events, FALSE, 30000);
- if(ret == WAIT_OBJECT_0 + EVENT_ERROR)
- return STATUS_UNSUCCESSFUL;
- else if(ret == WAIT_TIMEOUT)
- return ERROR_SERVICE_REQUEST_TIMEOUT;
-
- return STATUS_SUCCESS;
-}
-
-static void wine_drivers_rb_destroy( struct wine_rb_entry *entry, void *context )
-{
- if (unload_driver( entry, TRUE ) != STATUS_SUCCESS)
+ status = IoCreateDriver( &drv_name, init_driver );
+ if (status != STATUS_SUCCESS)
{
- struct wine_driver *driver = WINE_RB_ENTRY_VALUE( entry, struct wine_driver, entry );
- CloseHandle(driver->events[EVENT_STARTED]);
- CloseHandle(driver->events[EVENT_ERROR]);
- ObDereferenceObject( driver->driver_obj );
- CloseServiceHandle( (void *)driver->handle );
- HeapFree( GetProcessHeap(), 0, driver );
+ ERR( "failed to create driver %s: %08x\n", debugstr_w(driver->name), status );
+ RtlFreeUnicodeString( &drv_name );
+ goto error;
}
-}
-static void WINAPI async_shutdown_drivers( PTP_CALLBACK_INSTANCE instance, void *context )
-{
- CloseThreadpoolCleanupGroupMembers( cleanup_group, FALSE, NULL );
+ status = ObReferenceObjectByName( &drv_name, OBJ_CASE_INSENSITIVE, NULL,
+ 0, NULL, KernelMode, NULL, (void **)&driver->driver_obj );
+ RtlFreeUnicodeString( &drv_name );
+ if (status != STATUS_SUCCESS)
+ {
+ ERR( "failed to locate driver %s: %08x\n", debugstr_w(driver->name), status );
+ goto error;
+ }
- EnterCriticalSection( &drivers_cs );
- wine_rb_destroy( &wine_drivers, wine_drivers_rb_destroy, NULL );
- LeaveCriticalSection( &drivers_cs );
+ set_service_status( driver->handle, SERVICE_RUNNING,
+ SERVICE_ACCEPT_STOP | SERVICE_ACCEPT_SHUTDOWN );
+ return STATUS_SUCCESS;
- SetEvent( stop_event );
-}
-
-static void shutdown_drivers( void )
-{
- if (shutdown_in_progress) return;
-
- /* don't block the service control handler */
- if (!TrySubmitThreadpoolCallback( async_shutdown_drivers, NULL, NULL ))
- async_shutdown_drivers( NULL, NULL );
-
- shutdown_in_progress = TRUE;
+error:
+ set_service_status( driver->handle, SERVICE_STOPPED, 0 );
+ CloseServiceHandle( (void *)driver->handle );
+ wine_rb_remove( &wine_drivers, &driver->entry );
+ HeapFree( GetProcessHeap(), 0, driver );
+ return status;
}
static DWORD device_handler( DWORD ctrl, const WCHAR *driver_name )
@@ -506,10 +392,6 @@ static DWORD device_handler( DWORD ctrl, const WCHAR *driver_name )
struct wine_rb_entry *entry;
DWORD result = NO_ERROR;
- if (shutdown_in_progress)
- return ERROR_SERVICE_CANNOT_ACCEPT_CTRL;
-
- EnterCriticalSection( &drivers_cs );
entry = wine_rb_get( &wine_drivers, driver_name );
switch (ctrl)
@@ -521,14 +403,13 @@ static DWORD device_handler( DWORD ctrl, const WCHAR *driver_name )
case SERVICE_CONTROL_STOP:
if (!entry) break;
- result = RtlNtStatusToDosError(unload_driver( entry, FALSE ));
+ unload_driver( entry, NULL );
break;
default:
FIXME( "got driver ctrl %x for %s\n", ctrl, wine_dbgstr_w(driver_name) );
break;
}
- LeaveCriticalSection( &drivers_cs );
return result;
}
@@ -548,7 +429,7 @@ static DWORD WINAPI service_handler( DWORD ctrl, DWORD event_type, LPVOID event_
case SERVICE_CONTROL_SHUTDOWN:
TRACE( "shutting down %s\n", wine_dbgstr_w(service_group) );
set_service_status( service_handle, SERVICE_STOP_PENDING, 0 );
- shutdown_drivers();
+ SetEvent( stop_event );
return NO_ERROR;
default:
FIXME( "got service ctrl %x for %s\n", ctrl, wine_dbgstr_w(service_group) );
@@ -564,8 +445,6 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
if (!(stop_event = CreateEventW( NULL, TRUE, FALSE, NULL )))
return;
- if (!(cleanup_group = CreateThreadpoolCleanupGroup()))
- return;
if (!(manager_handle = OpenSCManagerW( NULL, NULL, SC_MANAGER_CONNECT )))
return;
if (!(service_handle = RegisterServiceCtrlHandlerExW( winedeviceW, service_handler, (void *)service_group )))
@@ -578,9 +457,9 @@ static void WINAPI ServiceMain( DWORD argc, LPWSTR *argv )
wine_ntoskrnl_main_loop( stop_event );
TRACE( "service group %s stopped\n", wine_dbgstr_w(service_group) );
+ wine_rb_destroy( &wine_drivers, unload_driver, NULL );
set_service_status( service_handle, SERVICE_STOPPED, 0 );
CloseServiceHandle( manager_handle );
- CloseThreadpoolCleanupGroup( cleanup_group );
CloseHandle( stop_event );
}
--
2.7.4
More information about the wine-devel
mailing list