[ros-diffs] [cgutman] 42387: - Finally get IRP_MJ_CLOSE working properly - Remove handling of IRP_MJ_CLEANUP and move the code to the DispatchClose routine - Remove the hack (holding an extra reference and not closing the handle) which hid these bugs - Fixes some memory and handle leaks too

Author: cgutman
Date: Tue Aug  4 20:56:56 2009
New Revision: 42387

URL: http://svn.reactos.org/svn/reactos?rev=42387&view=rev
Log:
 - Finally get IRP_MJ_CLOSE working properly
 - Remove handling of IRP_MJ_CLEANUP and move the code to the DispatchClose routine
 - Remove the hack (holding an extra reference and not closing the handle) which hid these
bugs
 - Fixes some memory and handle leaks too

Modified:
    trunk/reactos/drivers/network/afd/afd/bind.c
    trunk/reactos/drivers/network/afd/afd/connect.c
    trunk/reactos/drivers/network/afd/afd/main.c
    trunk/reactos/drivers/network/afd/afd/tdi.c
    trunk/reactos/drivers/network/tcpip/include/fileobjs.h
    trunk/reactos/drivers/network/tcpip/tcpip/fileobjs.c
    trunk/reactos/drivers/network/tcpip/tcpip/main.c

Modified: trunk/reactos/drivers/network/afd/afd/bind.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/bi…
==============================================================================
--- trunk/reactos/drivers/network/afd/afd/bind.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/afd/bind.c [iso-8859-1] Tue Aug  4 20:56:56 2009
@@ -32,11 +32,6 @@
                                 FCB->LocalAddress,
                                 &FCB->AddressFile.Handle,
                                 &FCB->AddressFile.Object );
-
-	if (NT_SUCCESS(Status)) 
-	{
-		ObReferenceObject(FCB->AddressFile.Object);
-	}
 
     AFD_DbgPrint(MID_TRACE,("Returning %x\n", Status));
 

Modified: trunk/reactos/drivers/network/afd/afd/connect.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/co…
==============================================================================
--- trunk/reactos/drivers/network/afd/afd/connect.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/afd/connect.c [iso-8859-1] Tue Aug  4 20:56:56 2009
@@ -29,11 +29,6 @@
                                           FCB->Connection.Object );
     }
 
-	if (NT_SUCCESS(Status)) 
-	{
-		ObReferenceObject(FCB->Connection.Object);
-	}
-
     return Status;
 }
 

Modified: trunk/reactos/drivers/network/afd/afd/main.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/ma…
==============================================================================
--- trunk/reactos/drivers/network/afd/afd/main.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/afd/main.c [iso-8859-1] Tue Aug  4 20:56:56 2009
@@ -96,6 +96,8 @@
     FCB->DeviceExt = DeviceExt;
     FCB->Recv.Size = DEFAULT_RECEIVE_WINDOW_SIZE;
     FCB->Send.Size = DEFAULT_SEND_WINDOW_SIZE;
+    FCB->AddressFile.Handle = INVALID_HANDLE_VALUE;
+    FCB->Connection.Handle = INVALID_HANDLE_VALUE;
 
     KeInitializeSpinLock( &FCB->SpinLock );
     ExInitializeFastMutex( &FCB->Mutex );
@@ -167,13 +169,19 @@
     return Status;
 }
 
-VOID CleanupSocket( PAFD_FCB FCB ) {
+static NTSTATUS NTAPI
+AfdCloseSocket(PDEVICE_OBJECT DeviceObject, PIRP Irp,
+	       PIO_STACK_LOCATION IrpSp)
+{
+    PFILE_OBJECT FileObject = IrpSp->FileObject;
+    PAFD_FCB FCB = FileObject->FsContext;
     UINT i;
     PAFD_IN_FLIGHT_REQUEST InFlightRequest[IN_FLIGHT_REQUESTS];
 
-    AFD_DbgPrint(MIN_TRACE,("Called (%x)\n", FCB));
-
-    if( !SocketAcquireStateLock( FCB ) ) return;
+    AFD_DbgPrint(MID_TRACE,
+		 ("AfdClose(DeviceObject %p Irp %p)\n", DeviceObject, Irp));
+
+    if( !SocketAcquireStateLock( FCB ) ) return STATUS_FILE_CLOSED;
 
     FCB->State = SOCKET_STATE_CLOSED;
 
@@ -191,104 +199,53 @@
 	}
     }
 
-    FCB->State = SOCKET_STATE_CREATED;
-
-    if( FCB->EventSelect ) {
+    KillSelectsForFCB( FCB->DeviceExt, FileObject, FALSE );
+
+    SocketStateUnlock( FCB );
+
+    if( FCB->EventSelect )
         ObDereferenceObject( FCB->EventSelect );
-        FCB->EventSelect = NULL;
-    }
-    if( FCB->Context ) {
+
+    if( FCB->Context )
         ExFreePool( FCB->Context );
-        FCB->Context = NULL;
-    }
-    if( FCB->Recv.Window ) {
+
+    if( FCB->Recv.Window )
 	ExFreePool( FCB->Recv.Window );
-        FCB->Recv.Window = NULL;
-    }
-    if( FCB->Send.Window ) {
+
+    if( FCB->Send.Window )
 	ExFreePool( FCB->Send.Window );
-        FCB->Send.Window = NULL;
-    }
-    if( FCB->AddressFrom ) {
+
+    if( FCB->AddressFrom )
 	ExFreePool( FCB->AddressFrom );
-        FCB->AddressFrom = NULL;
-    }
-    if( FCB->LocalAddress ) {
+
+    if( FCB->LocalAddress )
 	ExFreePool( FCB->LocalAddress );
-        FCB->LocalAddress = NULL;
-    }
-    if( FCB->RemoteAddress ) {
+
+    if( FCB->RemoteAddress )
 	ExFreePool( FCB->RemoteAddress );
-        FCB->RemoteAddress = NULL;
-    }
-    if( FCB->Connection.Object ) {
-	ZwClose(FCB->Connection.Handle);
+
+    if( FCB->Connection.Object )
 	ObDereferenceObject(FCB->Connection.Object);
-        FCB->Connection.Object = NULL;
-    }
-    if( FCB->AddressFile.Object ) {
-	ZwClose(FCB->AddressFile.Handle);
+
+    if( FCB->AddressFile.Object )
 	ObDereferenceObject(FCB->AddressFile.Object);
-        FCB->AddressFile.Object = NULL;
-    }
-
-    SocketStateUnlock( FCB );
-}
-
-VOID DestroySocket( PAFD_FCB FCB ) {
+
+    if( FCB->AddressFile.Handle != INVALID_HANDLE_VALUE )
+        ZwClose(FCB->AddressFile.Handle);
+
+    if( FCB->Connection.Handle != INVALID_HANDLE_VALUE )
+        ZwClose(FCB->Connection.Handle);
+
     if( FCB->TdiDeviceName.Buffer )
 	ExFreePool(FCB->TdiDeviceName.Buffer);
 
     ExFreePool(FCB);
-    AFD_DbgPrint(MIN_TRACE,("Deleted (%x)\n", FCB));
-}
-
-static NTSTATUS NTAPI
-AfdCloseSocket(PDEVICE_OBJECT DeviceObject, PIRP Irp,
-	       PIO_STACK_LOCATION IrpSp)
-{
-    PFILE_OBJECT FileObject = IrpSp->FileObject;
-    PAFD_FCB FCB = FileObject->FsContext;
-
-    AFD_DbgPrint(MID_TRACE,
-		 ("AfdClose(DeviceObject %p Irp %p)\n", DeviceObject, Irp));
-
-    if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp );
-
-    AFD_DbgPrint(MID_TRACE,("FCB %x\n", FCB));
-
-    FileObject->FsContext = NULL;
-    SocketStateUnlock( FCB );
-
-    DestroySocket( FCB );
 
     Irp->IoStatus.Status = STATUS_SUCCESS;
     Irp->IoStatus.Information = 0;
     IoCompleteRequest(Irp, IO_NETWORK_INCREMENT);
 
     AFD_DbgPrint(MID_TRACE, ("Returning success.\n"));
-
-    return STATUS_SUCCESS;
-}
-
-static NTSTATUS NTAPI
-AfdCleanupSocket(PDEVICE_OBJECT DeviceObject, PIRP Irp,
-                 PIO_STACK_LOCATION IrpSp)
-{
-    PFILE_OBJECT FileObject = IrpSp->FileObject;
-    PAFD_FCB FCB = FileObject->FsContext;
-
-    if( !SocketAcquireStateLock( FCB ) ) return LostSocket( Irp );
-
-    CleanupSocket( FCB );
-
-    KillSelectsForFCB( FCB->DeviceExt, FileObject, FALSE );
-
-    SocketStateUnlock( FCB );
-
-    Irp->IoStatus.Status = STATUS_SUCCESS;
-    Irp->IoStatus.Information = 0;
-    IoCompleteRequest(Irp, IO_NETWORK_INCREMENT);
 
     return STATUS_SUCCESS;
 }
@@ -373,9 +330,6 @@
 	/* Ditto the borrowing */
 	return AfdCloseSocket(DeviceObject, Irp, IrpSp);
 
-    case IRP_MJ_CLEANUP:
-        return AfdCleanupSocket(DeviceObject, Irp, IrpSp);
-
     /* write data */
     case IRP_MJ_WRITE:
 	return AfdConnectedSocketWriteData( DeviceObject, Irp, IrpSp, TRUE );
@@ -550,7 +504,6 @@
     /* register driver routines */
     DriverObject->MajorFunction[IRP_MJ_CLOSE] = AfdDispatch;
     DriverObject->MajorFunction[IRP_MJ_CREATE] = AfdDispatch;
-    DriverObject->MajorFunction[IRP_MJ_CLEANUP] = AfdDispatch;
     DriverObject->MajorFunction[IRP_MJ_WRITE] = AfdDispatch;
     DriverObject->MajorFunction[IRP_MJ_READ] = AfdDispatch;
     DriverObject->MajorFunction[IRP_MJ_DEVICE_CONTROL] = AfdDispatch;

Modified: trunk/reactos/drivers/network/afd/afd/tdi.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/td…
==============================================================================
--- trunk/reactos/drivers/network/afd/afd/tdi.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/afd/tdi.c [iso-8859-1] Tue Aug  4 20:56:56 2009
@@ -146,13 +146,12 @@
     }
 
     if (!NT_SUCCESS(Status)) {
-        *Handle = NULL;
+        *Handle = INVALID_HANDLE_VALUE;
         *Object = NULL;
     }
 
     return Status;
 }
-
 
 NTSTATUS TdiOpenAddressFile(
     PUNICODE_STRING DeviceName,

Modified: trunk/reactos/drivers/network/tcpip/include/fileobjs.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/incl…
==============================================================================
--- trunk/reactos/drivers/network/tcpip/include/fileobjs.h [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/include/fileobjs.h [iso-8859-1] Tue Aug  4
20:56:56 2009
@@ -23,9 +23,6 @@
 NTSTATUS FileCloseAddress(
   PTDI_REQUEST Request);
 
-NTSTATUS FileFreeAddress(
-  PTDI_REQUEST Request);
-
 NTSTATUS FileOpenConnection(
   PTDI_REQUEST Request,
   PVOID ClientContext);
@@ -35,13 +32,10 @@
 NTSTATUS FileCloseConnection(
   PTDI_REQUEST Request);
 
-NTSTATUS FileFreeConnection(
-  PTDI_REQUEST Request);
-
 NTSTATUS FileOpenControlChannel(
   PTDI_REQUEST Request);
 
-NTSTATUS FileFreeControlChannel(
+NTSTATUS FileCloseControlChannel(
   PTDI_REQUEST Request);
 
 #endif /* __FILEOBJS_H */

Modified: trunk/reactos/drivers/network/tcpip/tcpip/fileobjs.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/tcpi…
==============================================================================
--- trunk/reactos/drivers/network/tcpip/tcpip/fileobjs.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/tcpip/fileobjs.c [iso-8859-1] Tue Aug  4 20:56:56
2009
@@ -284,17 +284,21 @@
 NTSTATUS FileCloseAddress(
   PTDI_REQUEST Request)
 {
-  KIRQL OldIrql;
   PADDRESS_FILE AddrFile;
   NTSTATUS Status = STATUS_SUCCESS;
+  KIRQL OldIrql;
+  PDATAGRAM_RECEIVE_REQUEST ReceiveRequest;
   PDATAGRAM_SEND_REQUEST SendRequest;
-  PDATAGRAM_RECEIVE_REQUEST ReceiveRequest;
-  PLIST_ENTRY CurrentEntry;
-  PLIST_ENTRY NextEntry;
+  PLIST_ENTRY CurrentEntry, NextEntry;
+
+  AddrFile = Request->Handle.AddressHandle;
 
   TI_DbgPrint(MID_TRACE, ("Called.\n"));
 
-  AddrFile = Request->Handle.AddressHandle;
+  /* Remove address file from the global list */
+  TcpipAcquireSpinLock(&AddressFileListLock, &OldIrql);
+  RemoveEntryList(&AddrFile->ListEntry);
+  TcpipReleaseSpinLock(&AddressFileListLock, OldIrql);
 
   TcpipAcquireSpinLock(&AddrFile->Lock, &OldIrql);
 
@@ -311,7 +315,7 @@
     ReceiveRequest = CONTAINING_RECORD(CurrentEntry, DATAGRAM_RECEIVE_REQUEST,
ListEntry);
     /* Abort the request and free its resources */
     TcpipReleaseSpinLock(&AddrFile->Lock, OldIrql);
-    (*ReceiveRequest->Complete)(ReceiveRequest->Context, STATUS_ADDRESS_CLOSED,
0);
+    (*ReceiveRequest->Complete)(ReceiveRequest->Context, STATUS_CANCELLED, 0);
     TcpipAcquireSpinLock(&AddrFile->Lock, &OldIrql);
     CurrentEntry = NextEntry;
   }
@@ -326,42 +330,13 @@
 				    DATAGRAM_SEND_REQUEST, ListEntry);
     /* Abort the request and free its resources */
     TcpipReleaseSpinLock(&AddrFile->Lock, OldIrql);
-    (*SendRequest->Complete)(SendRequest->Context, STATUS_ADDRESS_CLOSED, 0);
+    (*SendRequest->Complete)(SendRequest->Context, STATUS_CANCELLED, 0);
     exFreePool(SendRequest);
     TcpipAcquireSpinLock(&AddrFile->Lock, &OldIrql);
     CurrentEntry = NextEntry;
   }
 
   TcpipReleaseSpinLock(&AddrFile->Lock, OldIrql);
-
-  TI_DbgPrint(MAX_TRACE, ("Leaving.\n"));
-
-  return Status;
-}
-
-
-/*
- * FUNCTION: Closes an address file object
- * ARGUMENTS:
- *     Request = Pointer to TDI request structure for this request
- * RETURNS:
- *     Status of operation
- */
-NTSTATUS FileFreeAddress(
-  PTDI_REQUEST Request)
-{
-  PADDRESS_FILE AddrFile;
-  NTSTATUS Status = STATUS_SUCCESS;
-  KIRQL OldIrql;
-
-  AddrFile = Request->Handle.AddressHandle;
-
-  TI_DbgPrint(MID_TRACE, ("Called.\n"));
-
-  /* Remove address file from the global list */
-  TcpipAcquireSpinLock(&AddressFileListLock, &OldIrql);
-  RemoveEntryList(&AddrFile->ListEntry);
-  TcpipReleaseSpinLock(&AddressFileListLock, OldIrql);
 
   /* Protocol specific handling */
   switch (AddrFile->Protocol) {
@@ -474,42 +449,20 @@
   PTDI_REQUEST Request)
 {
   PCONNECTION_ENDPOINT Connection;
+  KIRQL OldIrql;
 
   TI_DbgPrint(MID_TRACE, ("Called.\n"));
 
   Connection = Request->Handle.ConnectionContext;
+
+  TcpipAcquireSpinLock(&ConnectionEndpointListLock, &OldIrql);
+  RemoveEntryList(&Connection->ListEntry);
+  TcpipReleaseSpinLock(&ConnectionEndpointListLock, OldIrql);
 
   TcpipRecursiveMutexEnter( &TCPLock, TRUE );
   TCPClose( Connection );
   TcpipRecursiveMutexLeave( &TCPLock );
 
-  TI_DbgPrint(MAX_TRACE, ("Leaving.\n"));
-
-  return STATUS_SUCCESS;
-}
-
-
-/*
- * FUNCTION: Frees an connection file object
- * ARGUMENTS:
- *     Request = Pointer to TDI request structure for this request
- * RETURNS:
- *     Status of operation
- */
-NTSTATUS FileFreeConnection(
-  PTDI_REQUEST Request)
-{
-  KIRQL OldIrql;
-  PCONNECTION_ENDPOINT Connection;
-
-  TI_DbgPrint(MID_TRACE, ("Called.\n"));
-
-  Connection = Request->Handle.ConnectionContext;
-
-  TcpipAcquireSpinLock(&ConnectionEndpointListLock, &OldIrql);
-  RemoveEntryList(&Connection->ListEntry);
-  TcpipReleaseSpinLock(&ConnectionEndpointListLock, OldIrql);
-
   TCPFreeConnectionEndpoint(Connection);
 
   TI_DbgPrint(MAX_TRACE, ("Leaving.\n"));
@@ -565,7 +518,7 @@
  * RETURNS:
  *     Status of operation
  */
-NTSTATUS FileFreeControlChannel(
+NTSTATUS FileCloseControlChannel(
   PTDI_REQUEST Request)
 {
   PCONTROL_CHANNEL ControlChannel = Request->Handle.ControlChannel;

Modified: trunk/reactos/drivers/network/tcpip/tcpip/main.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/tcpi…
==============================================================================
--- trunk/reactos/drivers/network/tcpip/tcpip/main.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/tcpip/main.c [iso-8859-1] Tue Aug  4 20:56:56
2009
@@ -252,74 +252,6 @@
   return Status;
 }
 
-/*
- * FUNCTION: Prepares a file object for close
- * ARGUMENTS:
- *     DeviceObject = Pointer to a device object for this driver
- *     Irp          = Pointer to a I/O request packet
- * RETURNS:
- *     Status of the operation
- * NOTES:
- *     This function does not pend
- */
-NTSTATUS TiCleanupFileObject(
-  PDEVICE_OBJECT DeviceObject,
-  PIRP Irp)
-{
-  PIO_STACK_LOCATION IrpSp;
-  PTRANSPORT_CONTEXT Context;
-  TDI_REQUEST Request;
-  NTSTATUS Status;
-  KIRQL OldIrql;
-
-  IrpSp   = IoGetCurrentIrpStackLocation(Irp);
-  Context = IrpSp->FileObject->FsContext;
-  if (!Context) {
-    TI_DbgPrint(MIN_TRACE, ("Parameters are invalid.\n"));
-    return STATUS_INVALID_PARAMETER;
-  }
-
-  IoAcquireCancelSpinLock(&OldIrql);
-
-  Context->CancelIrps = TRUE;
-
-  IoReleaseCancelSpinLock(OldIrql);
-
-  switch ((ULONG_PTR)IrpSp->FileObject->FsContext2) {
-  case TDI_TRANSPORT_ADDRESS_FILE:
-    Request.Handle.AddressHandle = Context->Handle.AddressHandle;
-    Status = FileCloseAddress(&Request);
-    break;
-
-  case TDI_CONNECTION_FILE:
-    Request.Handle.ConnectionContext = Context->Handle.ConnectionContext;
-    Status = FileCloseConnection(&Request);
-    break;
-
-  case TDI_CONTROL_CHANNEL_FILE:
-    Request.Handle.ControlChannel = Context->Handle.ControlChannel;
-	/* Nothing to do to close */
-	Status = STATUS_SUCCESS;
-    break;
-
-  default:
-    /* This should never happen */
-
-    TI_DbgPrint(MIN_TRACE, ("Unknown transport context.\n"));
-
-    IoAcquireCancelSpinLock(&OldIrql);
-    Context->CancelIrps = FALSE;
-    IoReleaseCancelSpinLock(OldIrql);
-
-    Status = STATUS_INVALID_PARAMETER;
-
-  }
-
-  Irp->IoStatus.Status = Status;
-
-  return Irp->IoStatus.Status;
-}
-
 
 /*
  * FUNCTION: Releases resources used by a file object
@@ -339,7 +271,6 @@
   PTRANSPORT_CONTEXT Context;
   TDI_REQUEST Request;
   NTSTATUS Status;
-  KIRQL OldIrql;
 
   IrpSp   = IoGetCurrentIrpStackLocation(Irp);
   Context = IrpSp->FileObject->FsContext;
@@ -348,39 +279,26 @@
     return STATUS_INVALID_PARAMETER;
   }
 
-  IoAcquireCancelSpinLock(&OldIrql);
-
-  Context->CancelIrps = TRUE;
-
-  IoReleaseCancelSpinLock(OldIrql);
-
   switch ((ULONG_PTR)IrpSp->FileObject->FsContext2) {
   case TDI_TRANSPORT_ADDRESS_FILE:
     Request.Handle.AddressHandle = Context->Handle.AddressHandle;
-    Status = FileFreeAddress(&Request);
+    Status = FileCloseAddress(&Request);
     break;
 
   case TDI_CONNECTION_FILE:
     Request.Handle.ConnectionContext = Context->Handle.ConnectionContext;
-    Status = FileFreeConnection(&Request);
+    Status = FileCloseConnection(&Request);
     break;
 
   case TDI_CONTROL_CHANNEL_FILE:
     Request.Handle.ControlChannel = Context->Handle.ControlChannel;
-    Status = FileFreeControlChannel(&Request);
+    Status = FileCloseControlChannel(&Request);
     break;
 
   default:
-    /* This should never happen */
-
-    TI_DbgPrint(MIN_TRACE, ("Unknown transport context.\n"));
-
-    IoAcquireCancelSpinLock(&OldIrql);
-    Context->CancelIrps = FALSE;
-    IoReleaseCancelSpinLock(OldIrql);
-
+    DbgPrint("Unknown type %d\n",
(ULONG_PTR)IrpSp->FileObject->FsContext2);
     Status = STATUS_INVALID_PARAMETER;
-
+    break;
   }
 
   Irp->IoStatus.Status = Status;
@@ -422,12 +340,6 @@
   case IRP_MJ_CLOSE:
     Context = (PTRANSPORT_CONTEXT)IrpSp->FileObject->FsContext;
 	Status = TiCloseFileObject(DeviceObject, Irp);
-    break;
-
-  /* Release resources bound to an address file, connection endpoint,
-     or control connection */
-  case IRP_MJ_CLEANUP:
-    Status = TiCleanupFileObject(DeviceObject, Irp);
     break;
 
   default:
@@ -912,7 +824,6 @@
   /* Initialize the driver object with this driver's entry points */
   DriverObject->MajorFunction[IRP_MJ_CREATE]  = TiDispatchOpenClose;
   DriverObject->MajorFunction[IRP_MJ_CLOSE]   = TiDispatchOpenClose;
-  DriverObject->MajorFunction[IRP_MJ_CLEANUP] = TiDispatchOpenClose;
   DriverObject->MajorFunction[IRP_MJ_INTERNAL_DEVICE_CONTROL] = TiDispatchInternal;
   DriverObject->MajorFunction[IRP_MJ_DEVICE_CONTROL] = TiDispatch;