[ros-diffs] [cgutman] 53083: [TCPIP] - Fix socket closure from a hacked mess that just aborts every socket into something that works properly [LWIP] - Add code to indicate when a socket is closed from LAST_ACK...

Author: cgutman
Date: Fri Aug  5 10:28:11 2011
New Revision: 53083

URL: http://svn.reactos.org/svn/reactos?rev=53083&view=rev
Log:
[TCPIP]
- Fix socket closure from a hacked mess that just aborts every socket into something that works properly
[LWIP]
- Add code to indicate when a socket is closed from LAST_ACK state
- The problem was that after we call tcp_close from a CLOSE_WAIT state, we never get any indication when the connection is actually closed which causes problems because we're stuck guessing when we can release the port
- I will bring this issue up on the lwIP mailing lists to see if this is a patch they want to integrate or if they have any other ideas

Modified:
    branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/dispatch.c
    branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/fileobjs.c
    branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/event.c
    branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/tcp.c
    branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/core/tcp_in.c
    branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/include/rosip.h
    branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/rostcp.c

Modified: branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/dispatch.c
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/drivers/ne...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/dispatch.c [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/dispatch.c [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -444,11 +444,9 @@
  *     Status of operation
  */
 {
-  PCONNECTION_ENDPOINT Connection, LastConnection;
+  PCONNECTION_ENDPOINT Connection;
   PTRANSPORT_CONTEXT TranContext;
   PIO_STACK_LOCATION IrpSp;
-  KIRQL OldIrql;
-  NTSTATUS Status;

TI_DbgPrint(DEBUG_IRP, ("Called.\n"));

@@ -468,52 +466,9 @@
     return STATUS_INVALID_PARAMETER;
   }

-  LockObject(Connection, &OldIrql);
-
-  if (!Connection->AddressFile) {
-    UnlockObject(Connection, OldIrql);
-    TI_DbgPrint(MID_TRACE, ("No address file is asscociated.\n"));
-    return STATUS_INVALID_PARAMETER;
-  }
-
-  LockObjectAtDpcLevel(Connection->AddressFile);
-
-  /* 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);
-
-  if (Status == STATUS_SUCCESS)
-  {
-      /* Remove the address file from this connection */
-      DereferenceObject(Connection->AddressFile);
-      Connection->AddressFile = NULL;
-  }
-
-  UnlockObject(Connection, OldIrql);
-
-  return Status;
+  /* NO-OP because we need the address to deallocate the port when the connection closes */
+
+  return STATUS_SUCCESS;
 }

@@ -653,6 +608,7 @@
      Status = STATUS_NO_MEMORY;

if( NT_SUCCESS(Status) ) {
+          ReferenceObject(Connection->AddressFile);
      Connection->AddressFile->Listener->AddressFile =
          Connection->AddressFile;

Modified: branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/fileobjs.c
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/drivers/ne...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/fileobjs.c [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/drivers/network/tcpip/tcpip/fileobjs.c [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -409,7 +409,6 @@
   /* We have to close this listener because we started it */
   if( AddrFile->Listener )
   {
-      AddrFile->Listener->AddressFile = NULL;
       TCPClose( AddrFile->Listener );
   }

Modified: branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/event.c
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/lib/driver...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/event.c [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/event.c [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -63,7 +63,7 @@
 }

VOID
-FlushAllQueues(PCONNECTION_ENDPOINT Connection, const NTSTATUS Status)
+FlushReceiveQueue(PCONNECTION_ENDPOINT Connection, const NTSTATUS Status)
 {
     PTDI_BUCKET Bucket;
     PLIST_ENTRY Entry;
@@ -84,13 +84,35 @@
         CompleteBucket(Connection, Bucket, FALSE);
     }

-    // Calling with Status == STATUS_SUCCESS means that we got a graceful closure
-    // so we don't want to kill everything else since send is still valid in this state
-    //
+    DereferenceObject(Connection);
+}
+
+VOID
+FlushAllQueues(PCONNECTION_ENDPOINT Connection, NTSTATUS Status)
+{
+    PTDI_BUCKET Bucket;
+    PLIST_ENTRY Entry;
+    
+    ReferenceObject(Connection);
+        
+    while ((Entry = ExInterlockedRemoveHeadList(&Connection->ReceiveRequest, &Connection->Lock)))
+    {
+        Bucket = CONTAINING_RECORD( Entry, TDI_BUCKET, Entry );
+        
+        TI_DbgPrint(DEBUG_TCP,
+                    ("Completing Receive request: %x %x\n",
+                     Bucket->Request, Status));
+        
+        Bucket->Status = Status;
+        Bucket->Information = 0;
+        
+        CompleteBucket(Connection, Bucket, FALSE);
+    }
+
+    /* We completed the reads successfully but we need to return failure now */
     if (Status == STATUS_SUCCESS)
     {
-        DereferenceObject(Connection);
-        return;
+        Status = STATUS_FILE_CLOSED;
     }

while ((Entry = ExInterlockedRemoveHeadList(&Connection->ListenRequest, &Connection->Lock)))
@@ -134,17 +156,61 @@
 VOID
 TCPFinEventHandler(void *arg, const err_t err)
 {
-    PCONNECTION_ENDPOINT Connection = (PCONNECTION_ENDPOINT)arg;
-    const NTSTATUS status = TCPTranslateError(err);
-
-    /* Only clear the pointer if the shutdown was caused by an error */
-    if (err != ERR_OK)
-    {
-        /* We're got closed by the error so remove the PCB pointer */
+    PCONNECTION_ENDPOINT Connection = (PCONNECTION_ENDPOINT)arg, LastConnection;
+    const NTSTATUS Status = TCPTranslateError(err);
+    KIRQL OldIrql;
+
+    ASSERT(Connection->AddressFile);
+
+    /* Check if this was a partial socket closure */
+    if (err == ERR_OK && Connection->SocketContext)
+    {
+        /* Just flush the receive queue and get out of here */
+        FlushReceiveQueue(Connection, STATUS_SUCCESS);
+    }
+    else
+    {
+        /* First off all, remove the PCB pointer */
         Connection->SocketContext = NULL;
-    }
-
-    FlushAllQueues(Connection, status);
+
+        /* Complete all outstanding requests now */
+        FlushAllQueues(Connection, Status);
+
+        LockObject(Connection, &OldIrql);
+
+        LockObjectAtDpcLevel(Connection->AddressFile);
+
+        /* Unlink this connection from the address file */
+        if (Connection->AddressFile->Connection == Connection)
+        {
+            Connection->AddressFile->Connection = Connection->Next;
+            DereferenceObject(Connection);
+        }
+        else if (Connection->AddressFile->Listener == Connection)
+        {
+            Connection->AddressFile->Listener = NULL;
+            DereferenceObject(Connection);
+        }
+        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);
+            }
+        }
+
+        UnlockObjectFromDpcLevel(Connection->AddressFile);
+
+        /* Remove the address file from this connection */
+        DereferenceObject(Connection->AddressFile);
+        Connection->AddressFile = NULL;
+
+        UnlockObject(Connection, OldIrql);
+    }
 }

VOID
@@ -186,7 +252,7 @@
             ASSERT( ((PTCP_PCB)Bucket->AssociatedEndpoint->SocketContext)->state == CLOSED );

/*  free socket context created in FileOpenConnection, as we're using a new one */
-            LibTCPClose(Bucket->AssociatedEndpoint, TRUE);
+            LibTCPClose(Bucket->AssociatedEndpoint, TRUE, FALSE);

/* free previously created socket context (we don't use it, we use newpcb) */
             Bucket->AssociatedEndpoint->SocketContext = newpcb;

Modified: branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/tcp.c
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/lib/driver...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/tcp.c [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/lib/drivers/ip/transport/tcp/tcp.c [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -106,15 +106,12 @@

Socket = Connection->SocketContext;

-    /* 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 (Connection->SocketContext)
     {
         FlushAllQueues(Connection, STATUS_CANCELLED);

-        LibTCPClose(Connection, FALSE);
+        LibTCPClose(Connection, FALSE, TRUE);
     }

UnlockObject(Connection, OldIrql);

Modified: branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/core/tcp_in.c
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/lib/driver...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/core/tcp_in.c [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/core/tcp_in.c [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -345,6 +345,10 @@
       } else if (recv_flags & TF_CLOSED) {
         /* The connection has been closed and we will deallocate the
            PCB. */
+        TCP_EVENT_CLOSED(pcb, err);
+        if (err == ERR_ABRT) {
+          goto aborted;
+        }
         tcp_pcb_remove(&tcp_active_pcbs, pcb);
         memp_free(MEMP_TCP_PCB, pcb);
       } else {

Modified: branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/include/rosip.h
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/lib/driver...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/include/rosip.h [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/include/rosip.h [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -34,7 +34,7 @@
 err_t       LibTCPSend(PCONNECTION_ENDPOINT Connection, void *const dataptr, const u16_t len, const int safe);
 err_t       LibTCPConnect(PCONNECTION_ENDPOINT Connection, struct ip_addr *const ipaddr, const u16_t port);
 err_t       LibTCPShutdown(PCONNECTION_ENDPOINT Connection, const int shut_rx, const int shut_tx);
-err_t       LibTCPClose(PCONNECTION_ENDPOINT Connection, const int safe);
+err_t       LibTCPClose(PCONNECTION_ENDPOINT Connection, const int safe, const int callback);

err_t       LibTCPGetPeerName(PTCP_PCB pcb, struct ip_addr *const ipaddr, u16_t *const port);
 err_t       LibTCPGetHostName(PTCP_PCB pcb, struct ip_addr *const ipaddr, u16_t *const port);

Modified: branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/rostcp.c
URL: http://svn.reactos.org/svn/reactos/branches/GSoC_2011/TcpIpDriver/lib/driver...
==============================================================================
--- branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/rostcp.c [iso-8859-1] (original)
+++ branches/GSoC_2011/TcpIpDriver/lib/drivers/lwip/src/rostcp.c [iso-8859-1] Fri Aug  5 10:28:11 2011
@@ -599,27 +599,25 @@
 LibTCPShutdownCallback(void *arg)
 {
     struct shutdown_callback_msg *msg = arg;
-    
+    PTCP_PCB pcb = msg->Connection->SocketContext;
+
     if (!msg->Connection->SocketContext)
     {
         msg->Error = ERR_CLSD;
         goto done;
     }

-    /*
-        We check here if the pcb is in state ESTABLISHED or SYN_RECV because otherwise
-        it means lwIP will take care of it anyway and if it does so before us it will
-        cause memory corruption.
-    */
-    if ((((PTCP_PCB)msg->Connection->SocketContext)->state == ESTABLISHED) ||
-        (((PTCP_PCB)msg->Connection->SocketContext)->state == SYN_RCVD))
-    {
-        msg->Error = 
-            tcp_shutdown((PTCP_PCB)msg->Connection->SocketContext,
-                msg->shut_rx, msg->shut_tx);
-    }
-    else
-        msg->Error = ERR_OK;
+    if (pcb->state == CLOSE_WAIT)
+    {
+        /* This case actually results in a socket closure later (lwIP bug?) */
+        msg->Connection->SocketContext = NULL;
+    }
+
+    msg->Error = tcp_shutdown(pcb, msg->shut_rx, msg->shut_tx);
+    if (msg->Error)
+    {
+        msg->Connection->SocketContext = pcb;
+    }

done:
     KeSetEvent(&msg->Event, IO_NO_INCREMENT, FALSE);
@@ -662,6 +660,7 @@

/* Input */
     PCONNECTION_ENDPOINT Connection;
+    int Callback;

/* Output */
     err_t Error;
@@ -672,6 +671,8 @@
 LibTCPCloseCallback(void *arg)
 {
     struct close_callback_msg *msg = arg;
+    PTCP_PCB pcb = msg->Connection->SocketContext;
+    int state;

if (!msg->Connection->SocketContext)
     {
@@ -681,22 +682,43 @@

LibTCPEmptyQueue(msg->Connection);

-    if (((PTCP_PCB)msg->Connection->SocketContext)->state == LISTEN)
-    {
-        msg->Error = tcp_close((PTCP_PCB)msg->Connection->SocketContext);
+    /* Clear the PCB pointer */
+    msg->Connection->SocketContext = NULL;
+
+    /* Save the old PCB state */
+    state = pcb->state;
+
+    msg->Error = tcp_close(pcb);
+    if (!msg->Error)
+    {
+        if (msg->Callback)
+        {
+            /* Call the FIN handler in the cases where it will not be called by lwIP */
+            switch (state)
+            {
+                case CLOSED:
+                case LISTEN:
+                case SYN_SENT:
+                   TCPFinEventHandler(msg->Connection, ERR_OK);
+                   break;
+
+                default:
+                   break;
+            }
+        }
     }
     else
     {
-        tcp_abort((PTCP_PCB)msg->Connection->SocketContext);
-        msg->Error = ERR_OK;
-    }
-    
+        /* Restore the PCB pointer */
+        msg->Connection->SocketContext = pcb;
+    }
+
 done:
     KeSetEvent(&msg->Event, IO_NO_INCREMENT, FALSE);
 }

err_t
-LibTCPClose(PCONNECTION_ENDPOINT Connection, const int safe)
+LibTCPClose(PCONNECTION_ENDPOINT Connection, const int safe, const int callback)
 {
     err_t ret;
     struct close_callback_msg *msg;
@@ -707,6 +729,7 @@
         KeInitializeEvent(&msg->Event, NotificationEvent, FALSE);

msg->Connection = Connection;
+        msg->Callback = callback;

if (safe)
             LibTCPCloseCallback(msg);