From b92d45b8530b9b1a0ead21462720e517590387b4 Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Sun, 5 Jun 2011 02:16:45 +0000 Subject: [PATCH] [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 svn path=/trunk/; revision=52086 --- .../drivers/network/tcpip/include/titypes.h | 2 + .../drivers/network/tcpip/tcpip/dispatch.c | 56 ++++++++++++++----- .../drivers/network/tcpip/tcpip/fileobjs.c | 12 ++-- reactos/lib/drivers/ip/transport/tcp/tcp.c | 23 +------- 4 files changed, 54 insertions(+), 39 deletions(-) diff --git a/reactos/drivers/network/tcpip/include/titypes.h b/reactos/drivers/network/tcpip/include/titypes.h index 5400010db5e..726ea002bbd 100644 --- a/reactos/drivers/network/tcpip/include/titypes.h +++ b/reactos/drivers/network/tcpip/include/titypes.h @@ -270,6 +270,8 @@ typedef struct _CONNECTION_ENDPOINT { /* Signals */ UINT SignalState; /* Active signals from oskit */ + + struct _CONNECTION_ENDPOINT *Next; /* Next connection in address file list */ } CONNECTION_ENDPOINT, *PCONNECTION_ENDPOINT; diff --git a/reactos/drivers/network/tcpip/tcpip/dispatch.c b/reactos/drivers/network/tcpip/tcpip/dispatch.c index f216ed5f678..2e108b8db33 100644 --- a/reactos/drivers/network/tcpip/tcpip/dispatch.c +++ b/reactos/drivers/network/tcpip/tcpip/dispatch.c @@ -259,7 +259,7 @@ NTSTATUS DispTdiAssociateAddress( 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 @@ NTSTATUS DispTdiAssociateAddress( /* Add connection endpoint to the address file */ ReferenceObject(Connection); - AddrFile->Connection = Connection; + if (AddrFile->Connection == NULL) + AddrFile->Connection = Connection; + else + { + LastConnection = AddrFile->Connection; + while (LastConnection->Next != NULL) + LastConnection = LastConnection->Next; + LastConnection->Next = Connection; + } - /* FIXME: Maybe do this in DispTdiDisassociateAddress() instead? */ ObDereferenceObject(FileObject); UnlockObjectFromDpcLevel(AddrFile); UnlockObject(Connection, OldIrql); - return Status; + return STATUS_SUCCESS; } @@ -425,10 +432,11 @@ NTSTATUS DispTdiDisassociateAddress( * 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 @@ NTSTATUS DispTdiDisassociateAddress( 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 @@ NTSTATUS DispTdiListen( Status = STATUS_NO_MEMORY; if( NT_SUCCESS(Status) ) { - ReferenceObject(Connection->AddressFile); Connection->AddressFile->Listener->AddressFile = Connection->AddressFile; diff --git a/reactos/drivers/network/tcpip/tcpip/fileobjs.c b/reactos/drivers/network/tcpip/tcpip/fileobjs.c index 98cc4062d0d..4d4285add70 100644 --- a/reactos/drivers/network/tcpip/tcpip/fileobjs.c +++ b/reactos/drivers/network/tcpip/tcpip/fileobjs.c @@ -161,6 +161,9 @@ VOID AddrFileFree( 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 @@ NTSTATUS FileCloseAddress( 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); diff --git a/reactos/lib/drivers/ip/transport/tcp/tcp.c b/reactos/lib/drivers/ip/transport/tcp/tcp.c index e980f6b1e15..3f3889fd2e7 100644 --- a/reactos/lib/drivers/ip/transport/tcp/tcp.c +++ b/reactos/lib/drivers/ip/transport/tcp/tcp.c @@ -666,13 +666,14 @@ NTSTATUS TCPClose 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 @@ NTSTATUS TCPClose 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; }