[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 3/3] server: Hold a reference to the file in delete_file().
From:       Zebediah Figura <z.figura12 () gmail ! com>
Date:       2020-02-28 16:33:19
Message-ID: 4bc00dc5-2162-e476-85ee-c853b5256ef9 () gmail ! com
[Download RAW message or body]

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.

["server_device_refcount.diff" (text/x-patch)]

diff --git a/dlls/http.sys/http.c b/dlls/http.sys/http.c
index 4259ca618f..ad7dc33d1c 100644
--- a/dlls/http.sys/http.c
+++ b/dlls/http.sys/http.c
@@ -1373,6 +1373,8 @@ static NTSTATUS WINAPI dispatch_close(DEVICE_OBJECT *device, \
IRP *irp)  IO_STACK_LOCATION *stack = IoGetCurrentIrpStackLocation(irp);
     struct request_queue *queue = stack->FileObject->FsContext;
 
+    *(int *)0 = 0;
+
     TRACE("Closing queue %p.\n", queue);
     close_queue(queue);
 
diff --git a/dlls/httpapi/tests/httpapi.c b/dlls/httpapi/tests/httpapi.c
index a52a3f6c34..bbf5c580f1 100644
--- a/dlls/httpapi/tests/httpapi.c
+++ b/dlls/httpapi/tests/httpapi.c
@@ -1449,6 +1449,29 @@ START_TEST(httpapi)
     ret = HttpInitialize(version, HTTP_INITIALIZE_SERVER, NULL);
     ok(!ret, "Failed to initialize library, ret %u.\n", ret);
 
+    {
+        char DECLSPEC_ALIGN(8) req[2000];
+        unsigned short port;
+        OVERLAPPED ovl;
+        HANDLE queue;
+
+        ret = HttpCreateHttpHandle(&queue, 0);
+        ok(!ret, "Got error %u.\n", ret);
+
+        port = add_url_v1(queue);
+
+        ovl.hEvent = CreateEventA(NULL, TRUE, FALSE, NULL);
+        ret = HttpReceiveHttpRequest(queue, HTTP_NULL_ID, 0, (HTTP_REQUEST *)req, \
sizeof(req), NULL, &ovl); +        ok(ret == ERROR_IO_PENDING, "Got error %u.\n", \
ret); +
+        ret = remove_url_v1(queue, port);
+        ok(!ret, "Got error %u.\n", ret);
+
+        ret = CloseHandle(queue);
+        ok(ret, "Got error %u.\n", GetLastError());
+        return;
+    }
+
     test_v1_server();
     test_v1_completion_port();
     test_v1_multiple_requests();
diff --git a/server/device.c b/server/device.c
index d3e2a84c1e..f32daeb64b 100644
--- a/server/device.c
+++ b/server/device.c
@@ -734,6 +734,10 @@ static void delete_file( struct device_file *file )
         list_remove( &irp->mgr_entry );
         set_irp_result( irp, STATUS_FILE_DELETED, NULL, 0, 0 );
     }
+
+fprintf(stderr, "Dumping file %p:\n", file);
+    file->obj.ops->dump( &file->obj, 1 );
+fprintf(stderr, "Dumped file %p.\n", file);
 }
 
 static void delete_device( struct device *device )
@@ -745,6 +749,10 @@ static void delete_device( struct device *device )
     LIST_FOR_EACH_ENTRY_SAFE( file, next, &device->files, struct device_file, entry \
)  delete_file( file );
 
+fprintf(stderr, "Dumping device %p:\n", device);
+    device->obj.ops->dump( &file->obj, 1 );
+fprintf(stderr, "Dumped device %p.\n", device);
+
     unlink_named_object( &device->obj );
     list_remove( &device->entry );
     device->manager = NULL;



[prev in list] [next in list] [prev in thread] [next in thread]