Signed-off-by: Zebediah Figura z.figura12@gmail.com --- server/object.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/server/object.c b/server/object.c index 048da504ad..aeb30684a8 100644 --- a/server/object.c +++ b/server/object.c @@ -215,7 +215,7 @@ void *alloc_object( const struct object_ops *ops ) }
/* free an object once it has been destroyed */ -void free_object( struct object *obj ) +static void free_object( struct object *obj ) { free( obj->sd ); #ifdef DEBUG_OBJECTS
Otherwise, the only thing holding a reference to a device may be a device_file. If this is released in delete_device(), the subsequent call to unlink_named_object() will crash. This can occur if a device driver crashes with pending IRPs outstanding.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- This seems to be what's meant to be helped by this Staging patch:
https://raw.githubusercontent.com/wine-staging/wine-staging/master/patches/s...
When testing it, I couldn't reproduce a server crash just by crashing during an IRP_MJ_CREATE request, but I encountered this much later debugging bug 48530.
I think it makes more sense to formalize the reference to the device as belonging to the manager, rather than to add a temporary reference around deletion.
server/device.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/server/device.c b/server/device.c index d3e2a84c1e..b02d965e33 100644 --- a/server/device.c +++ b/server/device.c @@ -700,6 +700,7 @@ static struct device *create_device( struct object *root, const struct unicode_s { device->unix_path = NULL; device->manager = manager; + grab_object( device ); list_add_tail( &manager->devices, &device->entry ); list_init( &device->kernel_object ); list_init( &device->files ); @@ -748,6 +749,7 @@ static void delete_device( struct device *device ) unlink_named_object( &device->obj ); list_remove( &device->entry ); device->manager = NULL; + release_object( device ); }
From: Michael Müller michael@fds-team.de
Otherwise, we may attempt to access freed memory trawling the device list. This can occur if a device driver crashes during an IRP_CALL_CLOSE request.
Signed-off-by: Zebediah Figura z.figura12@gmail.com --- server/device.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/server/device.c b/server/device.c index b02d965e33..12208ec8a2 100644 --- a/server/device.c +++ b/server/device.c @@ -729,12 +729,18 @@ static void delete_file( struct device_file *file ) { struct irp_call *irp, *next;
+ /* The pending requests may be the only thing holding a reference to the + * file. */ + grab_object( file ); + /* terminate all pending requests */ LIST_FOR_EACH_ENTRY_SAFE( irp, next, &file->requests, struct irp_call, dev_entry ) { list_remove( &irp->mgr_entry ); set_irp_result( irp, STATUS_FILE_DELETED, NULL, 0, 0 ); } + + release_object( file ); }
static void delete_device( struct device *device )
On 2/14/20 12:10 PM, Zebediah Figura wrote:
From: Michael Müller michael@fds-team.de
Otherwise, we may attempt to access freed memory trawling the device list. This can occur if a device driver crashes during an IRP_CALL_CLOSE request.
Signed-off-by: Zebediah Figura z.figura12@gmail.com
server/device.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/server/device.c b/server/device.c index b02d965e33..12208ec8a2 100644 --- a/server/device.c +++ b/server/device.c @@ -729,12 +729,18 @@ static void delete_file( struct device_file *file ) { struct irp_call *irp, *next;
/* The pending requests may be the only thing holding a reference to the
* file. */grab_object( file );
/* terminate all pending requests */ LIST_FOR_EACH_ENTRY_SAFE( irp, next, &file->requests, struct irp_call, dev_entry ) { list_remove( &irp->mgr_entry ); set_irp_result( irp, STATUS_FILE_DELETED, NULL, 0, 0 ); }release_object( file ); }
static void delete_device( struct device *device )
I'm attaching a diff here which reproduces this issue. The test case is added to httpapi/tests, and intentionally causes a crash in http.sys' IRP_MJ_CLOSE handler (which is artificial, but as I understand it should not be allowed to crash the server). It essentially opens a file on the device, makes one overlapped ioctl which is never completed, and then closes the file. I've also added traces to server/device.c to clearly demonstrate the crash.
The server accesses freed memory for me without both of these patches. (In the case of delete_file(), my understanding is that LIST_FOR_EACH_ENTRY_SAFE still dereferences the list on every iteration to determine whether there are more elements.) This doesn't always manifest as a crash, but usually does for me.
-
Zebediah Figura