[ros-diffs] [cgutman] 59705: [TCPIP] - Add logging of address files and connections (on temporarily for testing changes) [LWIP] - Drastically simplify the closing state machine - All connection objects (and as...

Author: cgutman
Date: Mon Aug 12 03:09:28 2013
New Revision: 59705

URL: http://svn.reactos.org/svn/reactos?rev=59705&view=rev
Log:
[TCPIP]
- Add logging of address files and connections (on temporarily for testing changes)
[LWIP]
- Drastically simplify the closing state machine
- All connection objects (and as a result address files too) are now getting properly
reaped now
- Tested with Firefox and Abyss Web Server

Modified:
    trunk/reactos/drivers/network/tcpip/include/fileobjs.h
    trunk/reactos/drivers/network/tcpip/include/tcp.h
    trunk/reactos/drivers/network/tcpip/include/titypes.h
    trunk/reactos/drivers/network/tcpip/tcpip/fileobjs.c
    trunk/reactos/lib/drivers/ip/network/ip.c
    trunk/reactos/lib/drivers/lwip/src/rostcp.c

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] Mon Aug 12
03:09:28 2013
@@ -35,4 +35,6 @@
 NTSTATUS FileCloseControlChannel(
   PTDI_REQUEST Request);
 
+VOID LogActiveObjects(VOID);
+
 /* EOF */

Modified: trunk/reactos/drivers/network/tcpip/include/tcp.h
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/incl…
==============================================================================
--- trunk/reactos/drivers/network/tcpip/include/tcp.h	[iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/include/tcp.h	[iso-8859-1] Mon Aug 12 03:09:28
2013
@@ -211,3 +211,6 @@
 FlushAllQueues(PCONNECTION_ENDPOINT Connection, NTSTATUS Status);
 
 VOID CompleteBucket(PCONNECTION_ENDPOINT Connection, PTDI_BUCKET Bucket, const BOOLEAN
Synchronous);
+
+void
+LibTCPDumpPcb(PVOID SocketContext);

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] Mon Aug 12 03:09:28
2013
@@ -281,6 +281,7 @@
     BOOLEAN SendShutdown;
     BOOLEAN ReceiveShutdown;
     NTSTATUS ReceiveShutdownStatus;
+    BOOLEAN Closing;
 
     struct _CONNECTION_ENDPOINT *Next; /* Next connection in address file list */
 } CONNECTION_ENDPOINT, *PCONNECTION_ENDPOINT;

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] Mon Aug 12 03:09:28
2013
@@ -10,6 +10,8 @@
 
 #include "precomp.h"
 
+/* Uncomment for logging of connections and address files every 10 seconds */
+#define LOG_OBJECTS
 
 /* List of all address file objects managed by this driver */
 LIST_ENTRY AddressFileListHead;
@@ -97,6 +99,88 @@
    }
 
    return FALSE;
+}
+
+VOID
+LogActiveObjects(VOID)
+{
+#ifdef LOG_OBJECTS
+    PLIST_ENTRY CurrentEntry;
+    KIRQL OldIrql;
+    PADDRESS_FILE AddrFile;
+    PCONNECTION_ENDPOINT Conn;
+
+    DbgPrint("----------- TCP/IP Active Object Dump -------------\n");
+    
+    TcpipAcquireSpinLock(&AddressFileListLock, &OldIrql);
+
+    CurrentEntry = AddressFileListHead.Flink;
+    while (CurrentEntry != &AddressFileListHead)
+    {
+        AddrFile = CONTAINING_RECORD(CurrentEntry, ADDRESS_FILE, ListEntry);
+
+        DbgPrint("Address File (%s, %d, %d) @ 0x%p | Ref count: %d | Sharers:
%d\n",
+                 A2S(&AddrFile->Address), WN2H(AddrFile->Port),
AddrFile->Protocol,
+                 AddrFile, AddrFile->RefCount, AddrFile->Sharers);
+        DbgPrint("\tListener: ");
+        if (AddrFile->Listener == NULL)
+            DbgPrint("<None>\n");
+        else
+            DbgPrint("0x%p\n", AddrFile->Listener);
+        DbgPrint("\tAssociated endpoints: ");
+        if (AddrFile->Connection == NULL)
+            DbgPrint("<None>\n");
+        else
+        {
+            Conn = AddrFile->Connection;
+            while (Conn)
+            {
+                DbgPrint("0x%p ", Conn);
+                Conn = Conn->Next;
+            }
+            DbgPrint("\n");
+        }
+        
+        CurrentEntry = CurrentEntry->Flink;
+    }
+    
+    TcpipReleaseSpinLock(&AddressFileListLock, OldIrql);
+    
+    TcpipAcquireSpinLock(&ConnectionEndpointListLock, &OldIrql);
+    
+    CurrentEntry = ConnectionEndpointListHead.Flink;
+    while (CurrentEntry != &ConnectionEndpointListHead)
+    {
+        Conn = CONTAINING_RECORD(CurrentEntry, CONNECTION_ENDPOINT, ListEntry);
+        
+        DbgPrint("Connection @ 0x%p | Ref count: %d\n", Conn,
Conn->RefCount);
+        DbgPrint("\tPCB: ");
+        if (Conn->SocketContext == NULL)
+            DbgPrint("<None>\n");
+        else
+        {
+            DbgPrint("0x%p\n", Conn->SocketContext);
+            LibTCPDumpPcb(Conn->SocketContext);
+        }
+        DbgPrint("\tPacket queue status: %s\n",
IsListEmpty(&Conn->PacketQueue) ? "Empty" : "Not Empty");
+        DbgPrint("\tRequest lists: Connect: %s | Recv: %s | Send: %s | Shutdown: %s
| Listen: %s\n",
+                 IsListEmpty(&Conn->ConnectRequest) ? "Empty" :
"Not Empty",
+                 IsListEmpty(&Conn->ReceiveRequest) ? "Empty" :
"Not Empty",
+                 IsListEmpty(&Conn->SendRequest) ? "Empty" : "Not
Empty",
+                 IsListEmpty(&Conn->ShutdownRequest) ? "Empty" :
"Not Empty",
+                 IsListEmpty(&Conn->ListenRequest) ? "Empty" : "Not
Empty");
+        DbgPrint("\tSend shutdown: %s\n", Conn->SendShutdown ?
"Yes" : "No");
+        DbgPrint("\tReceive shutdown: %s\n", Conn->ReceiveShutdown ?
"Yes" : "No");
+        if (Conn->ReceiveShutdown) DbgPrint("\tReceive shutdown status:
0x%x\n", Conn->ReceiveShutdownStatus);
+        DbgPrint("\tClosing: %s\n", Conn->Closing ? "Yes" :
"No");
+        
+        CurrentEntry = CurrentEntry->Flink;
+    }
+    
+    TcpipReleaseSpinLock(&ConnectionEndpointListLock, OldIrql);
+
+    DbgPrint("---------------------------------------------------\n");
+#endif
 }
 
 PADDRESS_FILE AddrFindShared(

Modified: trunk/reactos/lib/drivers/ip/network/ip.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/lib/drivers/ip/network/ip.…
==============================================================================
--- trunk/reactos/lib/drivers/ip/network/ip.c	[iso-8859-1] (original)
+++ trunk/reactos/lib/drivers/ip/network/ip.c	[iso-8859-1] Mon Aug 12 03:09:28 2013
@@ -23,6 +23,8 @@
 /* Work around calling timer at Dpc level */
 
 IP_PROTOCOL_HANDLER ProtocolTable[IP_PROTOCOL_TABLE_SIZE];
+
+ULONG IpTimerExpirations;
 
 VOID
 TCPRegisterInterface(PIP_INTERFACE IF);
@@ -119,9 +121,16 @@
  *     SystemArgument1 = Unused
  *     SystemArgument2 = Unused
  * NOTES:
- *     This routine is dispatched once in a while to do maintainance jobs
- */
-{
+ *     This routine is dispatched once in a while to do maintenance jobs
+ */
+{
+    IpTimerExpirations++;
+    
+    if ((IpTimerExpirations % 10) == 0)
+    {
+        LogActiveObjects();
+    }
+
     /* Check if datagram fragments have taken too long to assemble */
     IPDatagramReassemblyTimeout();
 

Modified: trunk/reactos/lib/drivers/lwip/src/rostcp.c
URL:
http://svn.reactos.org/svn/reactos/trunk/reactos/lib/drivers/lwip/src/rostc…
==============================================================================
--- trunk/reactos/lib/drivers/lwip/src/rostcp.c	[iso-8859-1] (original)
+++ trunk/reactos/lib/drivers/lwip/src/rostcp.c	[iso-8859-1] Mon Aug 12 03:09:28 2013
@@ -35,6 +35,21 @@
 /* Required for ERR_T to NTSTATUS translation in receive error handling */
 NTSTATUS TCPTranslateError(const err_t err);
 
+void
+LibTCPDumpPcb(PVOID SocketContext)
+{
+    struct tcp_pcb *pcb = (struct tcp_pcb*)SocketContext;
+    unsigned int addr = ntohl(pcb->remote_ip.addr);
+
+    DbgPrint("\tState: %s\n", tcp_state_str[pcb->state]);
+    DbgPrint("\tRemote: (%d.%d.%d.%d, %d)\n",
+    (addr >> 24) & 0xFF,
+    (addr >> 16) & 0xFF,
+    (addr >> 8) & 0xFF,
+    addr & 0xFF,
+    pcb->remote_port);
+}
+
 static
 void
 LibTCPEmptyQueue(PCONNECTION_ENDPOINT Connection)
@@ -231,18 +246,15 @@
         Connection->ReceiveShutdown = TRUE;
         Connection->ReceiveShutdownStatus = STATUS_SUCCESS;
 
-        /* This code path executes for both remotely and locally initiated closures,
-         * and we need to distinguish between them */
-        if (Connection->SocketContext)
+        /* If we already did a send shutdown, we're in TIME_WAIT so we can't use
this PCB anymore */
+        if (Connection->SendShutdown)
         {
-            /* Remotely initiated close */
-            TCPRecvEventHandler(arg);
+            Connection->SocketContext = NULL;
+            tcp_arg(pcb, NULL);
         }
-        else
-        {
-            /* Locally initated close */
-            TCPFinEventHandler(arg, ERR_CLSD);
-        }
+
+        /* Remotely initiated close */
+        TCPRecvEventHandler(arg);
     }
 
     return ERR_OK;
@@ -285,30 +297,18 @@
 InternalErrorEventHandler(void *arg, const err_t err)
 {
     PCONNECTION_ENDPOINT Connection = arg;
-    KIRQL OldIrql;
 
     /* Make sure the socket didn't get closed */
     if (!arg) return;
 
-    /* Check if data is left to be read */
-    LockObject(Connection, &OldIrql);
-    if (IsListEmpty(&Connection->PacketQueue))
-    {
-        UnlockObject(Connection, OldIrql);
-
-        /* Deliver the error now */
-        TCPFinEventHandler(arg, err);
-    }
-    else
-    {
-        UnlockObject(Connection, OldIrql);
-
-        /* Defer the error delivery until all data is gone */
-        Connection->ReceiveShutdown = TRUE;
-        Connection->ReceiveShutdownStatus = TCPTranslateError(err);
-
-        TCPRecvEventHandler(arg);
-    }
+    /* The PCB is dead now */
+    Connection->SocketContext = NULL;
+
+    /* Defer the error delivery until all data is gone */
+    Connection->ReceiveShutdown = TRUE;
+    Connection->ReceiveShutdownStatus = TCPTranslateError(err);
+
+    TCPRecvEventHandler(arg);
 }
 
 static
@@ -633,7 +633,12 @@
         goto done;
     }
 
-    /* These need to be called separately, otherwise we get a tcp_close() */
+    /* LwIP makes the (questionable) assumption that SHUTDOWN_RDWR is equivalent to
tcp_close().
+     * This assumption holds even if the shutdown calls are done separately (even through
multiple
+     * WinSock shutdown() calls). This assumption means that lwIP has the right to
deallocate our
+     * PCB without telling us if we shutdown TX and RX. To avoid these problems,
we'll clear the
+     * socket context if we have called shutdown for TX and RX.
+     */
     if (msg->Input.Shutdown.shut_rx) {
         msg->Output.Shutdown.Error = tcp_shutdown(pcb, TRUE, FALSE);
     }
@@ -651,6 +656,14 @@
 
         if (msg->Input.Shutdown.shut_tx)
             msg->Input.Shutdown.Connection->SendShutdown = TRUE;
+
+        if (msg->Input.Shutdown.Connection->ReceiveShutdown &&
+            msg->Input.Shutdown.Connection->SendShutdown)
+        {
+            /* The PCB is not ours anymore */
+            msg->Input.Shutdown.Connection->SocketContext = NULL;
+            tcp_arg(pcb, NULL);
+        }
     }
 
 done:
@@ -697,37 +710,43 @@
     /* Empty the queue even if we're already "closed" */
     LibTCPEmptyQueue(msg->Input.Close.Connection);
 
-    if (!msg->Input.Close.Connection->SocketContext)
+    /* Check if we've already been closed */
+    if (msg->Input.Close.Connection->Closing)
     {
         msg->Output.Close.Error = ERR_OK;
         goto done;
     }
 
-    /* Clear the PCB pointer */
+    /* Enter "closing" mode if we're doing a normal close */
+    if (msg->Input.Close.Callback)
+        msg->Input.Close.Connection->Closing = TRUE;
+
+    /* Check if the PCB was already "closed" but the client doesn't know it
yet */
+    if (!msg->Input.Close.Connection->SocketContext)
+    {
+        if (msg->Input.Close.Callback)
+            TCPFinEventHandler(msg->Input.Close.Connection, ERR_CLSD);
+        msg->Output.Close.Error = ERR_OK;
+        goto done;
+    }
+
+    /* Clear the PCB pointer and stop callbacks */
     msg->Input.Close.Connection->SocketContext = NULL;
-
-    switch (pcb->state)
-    {
-        case CLOSED:
-        case LISTEN:
-        case SYN_SENT:
-           msg->Output.Close.Error = tcp_close(pcb);
-
-           if (!msg->Output.Close.Error && msg->Input.Close.Callback)
-               TCPFinEventHandler(msg->Input.Close.Connection, ERR_CLSD);
-           break;
-
-        default:
-           /* Abort the socket */
-           tcp_abort(pcb);
-           msg->Output.Close.Error = ERR_OK;
-           break;
-    }
+    tcp_arg(pcb, NULL);
+
+    /* This may generate additional callbacks but we don't care,
+     * because they're too inconsistent to rely on */
+    msg->Output.Close.Error = tcp_close(pcb);
 
     if (msg->Output.Close.Error)
     {
         /* Restore the PCB pointer */
         msg->Input.Close.Connection->SocketContext = pcb;
+        msg->Input.Close.Connection->Closing = FALSE;
+    }
+    else if (msg->Input.Close.Callback)
+    {
+        TCPFinEventHandler(msg->Input.Close.Connection, ERR_CLSD);
     }
 
 done: