[PATCH 3/3] server: Hold a reference to the file in delete_file().

[Zebediah Figura]

On 2/14/20 12:10 PM, Zebediah Figura wrote:
> From: Michael Müller <michael at 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 at 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: server_device_refcount.diff
Type: text/x-patch
Size: 2553 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20200228/d4f16869/attachment.bin>