[cgutman] 52086: [IP/TCPIP] - Add a Next member to CONNECTION_ENDPOINT to allow multiple connections to be associated with a single address file while not overwriting pointers, dereferencing other ...

Author: cgutman
Date: Sun Jun  5 02:16:45 2011
New Revision: 52086

URL: http://svn.reactos.org/svn/reactos?rev=52086&view=rev
Log:
[IP/TCPIP]
- Add a Next member to CONNECTION_ENDPOINT to allow multiple connections to be associated
with a single address file while not overwriting pointers, dereferencing other
people's connection objects, and causing horrific amounts of memory corruption
- Add several sanity checks to prevent this from happening again
- Don't try dereference address files and connection endpoints in the free functions;
there should be none associated since r52083 (sanity checks ensure this)
- Don't hold an extra reference to the address file when creating a listener; this
reference is implicit
- This should greatly increase reliability of activities that open lots of sockets such as
web browsing and running servers on ROS
- This also fixes most issues of not releasing a server port when the listener is closed

Modified:
    trunk/reactos/drivers/network/tcpip/include/titypes.h
    trunk/reactos/drivers/network/tcpip/tcpip/dispatch.c
    trunk/reactos/drivers/network/tcpip/tcpip/fileobjs.c
    trunk/reactos/lib/drivers/ip/transport/tcp/tcp.c

Modified: trunk/reactos/drivers/network/tcpip/include/titypes.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/incl…
==============================================================================
--- trunk/reactos/drivers/network/tcpip/include/titypes.h [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/include/titypes.h [iso-8859-1] Sun Jun  5 02:16:45
2011
@@ -270,6 +270,8 @@
 
     /* Signals */
     UINT    SignalState;       /* Active signals from oskit */
+
+    struct _CONNECTION_ENDPOINT *Next; /* Next connection in address file list */
 } CONNECTION_ENDPOINT, *PCONNECTION_ENDPOINT;
 
 

Modified: trunk/reactos/drivers/network/tcpip/tcpip/dispatch.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/tcpi…
==============================================================================
--- trunk/reactos/drivers/network/tcpip/tcpip/dispatch.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/tcpip/dispatch.c [iso-8859-1] Sun Jun  5 02:16:45
2011
@@ -259,7 +259,7 @@
   PTDI_REQUEST_KERNEL_ASSOCIATE Parameters;
   PTRANSPORT_CONTEXT TranContext;
   PIO_STACK_LOCATION IrpSp;
-  PCONNECTION_ENDPOINT Connection;
+  PCONNECTION_ENDPOINT Connection, LastConnection;
   PFILE_OBJECT FileObject;
   PADDRESS_FILE AddrFile = NULL;
   NTSTATUS Status;
@@ -340,15 +340,22 @@
 
   /* Add connection endpoint to the address file */
   ReferenceObject(Connection);
-  AddrFile->Connection = Connection;
-
-  /* FIXME: Maybe do this in DispTdiDisassociateAddress() instead? */
+  if (AddrFile->Connection == NULL)
+      AddrFile->Connection = Connection;
+  else
+  {
+      LastConnection = AddrFile->Connection;
+      while (LastConnection->Next != NULL)
+         LastConnection = LastConnection->Next;
+      LastConnection->Next = Connection;
+  }
+
   ObDereferenceObject(FileObject);
 
   UnlockObjectFromDpcLevel(AddrFile);
   UnlockObject(Connection, OldIrql);
 
-  return Status;
+  return STATUS_SUCCESS;
 }
 
 
@@ -425,10 +432,11 @@
  *     Status of operation
  */
 {
-  PCONNECTION_ENDPOINT Connection;
+  PCONNECTION_ENDPOINT Connection, LastConnection;
   PTRANSPORT_CONTEXT TranContext;
   PIO_STACK_LOCATION IrpSp;
   KIRQL OldIrql;
+  NTSTATUS Status;
 
   TI_DbgPrint(DEBUG_IRP, ("Called.\n"));
 
@@ -458,19 +466,42 @@
 
   LockObjectAtDpcLevel(Connection->AddressFile);
 
-  /* Remove this connection from the address file */
-  DereferenceObject(Connection->AddressFile->Connection);
-  Connection->AddressFile->Connection = NULL;
+  /* Unlink this connection from the address file */
+  if (Connection->AddressFile->Connection == Connection)
+  {
+      Connection->AddressFile->Connection = Connection->Next;
+      DereferenceObject(Connection);
+      Status = STATUS_SUCCESS;
+  }
+  else
+  {
+      LastConnection = Connection->AddressFile->Connection;
+      while (LastConnection->Next != Connection && LastConnection->Next !=
NULL)
+         LastConnection = LastConnection->Next;
+      if (LastConnection->Next == Connection)
+      {
+          LastConnection->Next = Connection->Next;
+          DereferenceObject(Connection);
+          Status = STATUS_SUCCESS;
+      }
+      else
+      {
+          Status = STATUS_INVALID_PARAMETER;
+      }
+  }
 
   UnlockObjectFromDpcLevel(Connection->AddressFile);
 
-  /* Remove the address file from this connection */
-  DereferenceObject(Connection->AddressFile);
-  Connection->AddressFile = NULL;
+  if (Status == STATUS_SUCCESS)
+  {
+      /* Remove the address file from this connection */
+      DereferenceObject(Connection->AddressFile);
+      Connection->AddressFile = NULL;
+  }
 
   UnlockObject(Connection, OldIrql);
 
-  return STATUS_SUCCESS;
+  return Status;
 }
 
 
@@ -602,7 +633,6 @@
 	  Status = STATUS_NO_MEMORY;
 
       if( NT_SUCCESS(Status) ) {
-          ReferenceObject(Connection->AddressFile);
 	  Connection->AddressFile->Listener->AddressFile =
 	      Connection->AddressFile;
 

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] Sun Jun  5 02:16:45
2011
@@ -161,6 +161,9 @@
 
   TI_DbgPrint(MID_TRACE, ("Called.\n"));
 
+  /* We should not be associated with a connection here */
+  ASSERT(!AddrFile->Connection);
+
   /* Remove address file from the global list */
   TcpipAcquireSpinLock(&AddressFileListLock, &OldIrql);
   RemoveEntryList(&AddrFile->ListEntry);
@@ -377,17 +380,14 @@
   if (!Request->Handle.AddressHandle) return STATUS_INVALID_PARAMETER;
 
   LockObject(AddrFile, &OldIrql);
-  /* We have to close this connection because we started it */
+
+  /* We have to close this listener because we started it */
   if( AddrFile->Listener )
   {
       AddrFile->Listener->AddressFile = NULL;
       TCPClose( AddrFile->Listener );
   }
-  if( AddrFile->Connection )
-  {
-      AddrFile->Connection->AddressFile = NULL;
-      DereferenceObject( AddrFile->Connection );
-  }
+
   UnlockObject(AddrFile, OldIrql);
 
   DereferenceObject(AddrFile);

Modified: trunk/reactos/lib/drivers/ip/transport/tcp/tcp.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/lib/drivers/ip/transport/t…
==============================================================================
--- trunk/reactos/lib/drivers/ip/transport/tcp/tcp.c [iso-8859-1] (original)
+++ trunk/reactos/lib/drivers/ip/transport/tcp/tcp.c [iso-8859-1] Sun Jun  5 02:16:45
2011
@@ -666,12 +666,13 @@
     KIRQL OldIrql;
     NTSTATUS Status;
     PVOID Socket;
-    PADDRESS_FILE AddressFile = NULL;
-    PCONNECTION_ENDPOINT AddressConnection = NULL;
 
     LockObject(Connection, &OldIrql);
     Socket = Connection->SocketContext;
     Connection->SocketContext = NULL;
+
+    /* We should not be associated to an address file at this point */
+    ASSERT(!Connection->AddressFile);
 
     /* Don't try to close again if the other side closed us already */
     if (Socket)
@@ -693,27 +694,9 @@
        Status = STATUS_SUCCESS;
     }
 
-    if (Connection->AddressFile)
-    {
-        LockObjectAtDpcLevel(Connection->AddressFile);
-        if (Connection->AddressFile->Connection == Connection)
-        {
-            AddressConnection = Connection->AddressFile->Connection;
-            Connection->AddressFile->Connection = NULL;
-        }
-        UnlockObjectFromDpcLevel(Connection->AddressFile);
-
-        AddressFile = Connection->AddressFile;
-        Connection->AddressFile = NULL;
-    }
-
     UnlockObject(Connection, OldIrql);
 
     DereferenceObject(Connection);
-    if (AddressConnection)
-        DereferenceObject(AddressConnection);
-    if (AddressFile)
-        DereferenceObject(AddressFile);
 
     return Status;
 }