[cgutman] 57975: [AFD] - Fix a bug during socket closure that could cause buffered data to be lost [LWIP] - Only signal errors when the packet queue is empty to avoid dropping received data - Use p...

Author: cgutman
Date: Sun Dec 23 09:53:36 2012
New Revision: 57975

URL: http://svn.reactos.org/svn/reactos?rev=57975&view=rev
Log:
[AFD]
- Fix a bug during socket closure that could cause buffered data to be lost
[LWIP]
- Only signal errors when the packet queue is empty to avoid dropping received data
- Use pbuf_copy_partial to copy pbuf chains instead of doing it manually which had a bug
CORE-6655 #resolve CORE-6741 #resolve

Modified:
    trunk/reactos/drivers/network/afd/afd/main.c
    trunk/reactos/drivers/network/afd/afd/read.c
    trunk/reactos/drivers/network/afd/include/afd.h
    trunk/reactos/drivers/network/tcpip/include/titypes.h
    trunk/reactos/lib/drivers/ip/transport/tcp/event.c
    trunk/reactos/lib/drivers/lwip/src/rostcp.c

Modified: trunk/reactos/drivers/network/afd/afd/main.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/mai...
==============================================================================
--- trunk/reactos/drivers/network/afd/afd/main.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/afd/main.c [iso-8859-1] Sun Dec 23 09:53:36 2012
@@ -610,6 +610,7 @@
         /* Signal complete connection closure immediately */
         FCB->PollState |= AFD_EVENT_ABORT;
         FCB->PollStatus[FD_CLOSE_BIT] = Irp->IoStatus.Status;
+        FCB->LastReceiveStatus = STATUS_FILE_CLOSED;
         PollReeval(FCB->DeviceExt, FCB->FileObject);
     }

@@ -708,8 +709,8 @@
         /* Mark us as overread to complete future reads with an error */
         FCB->Overread = TRUE;

-        /* Set a successful close status to indicate a shutdown on overread */
-        FCB->PollStatus[FD_CLOSE_BIT] = STATUS_SUCCESS;
+        /* Set a successful receive status to indicate a shutdown on overread */
+        FCB->LastReceiveStatus = STATUS_SUCCESS;

/* Clear the receive event */
         FCB->PollState &= ~AFD_EVENT_RECEIVE;

Modified: trunk/reactos/drivers/network/afd/afd/read.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/afd/rea...
==============================================================================
--- trunk/reactos/drivers/network/afd/afd/read.c [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/afd/read.c [iso-8859-1] Sun Dec 23 09:53:36 2012
@@ -52,11 +52,12 @@

static VOID HandleReceiveComplete( PAFD_FCB FCB, NTSTATUS Status, ULONG_PTR Information )
 {
+    FCB->LastReceiveStatus = Status;
+
     /* We got closed while the receive was in progress */
     if (FCB->TdiReceiveClosed)
     {
-        FCB->Recv.Content = 0;
-        FCB->Recv.BytesUsed = 0;
+        /* The received data is discarded */
     }
     /* Receive successful */
     else if (Status == STATUS_SUCCESS)
@@ -67,30 +68,20 @@
         /* Check for graceful closure */
         if (Information == 0)
         {
+            /* Receive is closed */
             FCB->TdiReceiveClosed = TRUE;
-
-            /* Signal graceful receive shutdown */
-            FCB->PollState |= AFD_EVENT_DISCONNECT;
-            FCB->PollStatus[FD_CLOSE_BIT] = Status;
-
-            PollReeval( FCB->DeviceExt, FCB->FileObject );
-        }
-
-        /* Issue another receive IRP to keep the buffer well stocked */
-        RefillSocketBuffer(FCB);
+        }
+        else
+        {
+            /* Issue another receive IRP to keep the buffer well stocked */
+            RefillSocketBuffer(FCB);
+        }
     }
     /* Receive failed with no data (unexpected closure) */
     else
     {
-        FCB->Recv.BytesUsed = 0;
-        FCB->Recv.Content = 0;
+        /* Previously received data remains intact */
         FCB->TdiReceiveClosed = TRUE;
-
-        /* Signal complete connection failure immediately */
-        FCB->PollState |= AFD_EVENT_CLOSE;
-        FCB->PollStatus[FD_CLOSE_BIT] = Status;
-
-        PollReeval( FCB->DeviceExt, FCB->FileObject );
     }
 }

@@ -168,9 +159,6 @@

AFD_DbgPrint(MID_TRACE,("%x %x\n", FCB, Irp));

-    /* Kick the user that receive would be possible now */
-    /* XXX Not implemented yet */
-
     AFD_DbgPrint(MID_TRACE,("FCB %x Receive data waiting %d\n",
                             FCB, FCB->Recv.Content));

@@ -188,7 +176,7 @@
                                     TotalBytesCopied));
             UnlockBuffers( RecvReq->BufferArray,
                            RecvReq->BufferCount, FALSE );
-            if (FCB->Overread && FCB->PollStatus[FD_CLOSE_BIT] == STATUS_SUCCESS)
+            if (FCB->Overread && FCB->LastReceiveStatus == STATUS_SUCCESS)
             {
                 /* Overread after a graceful disconnect so complete with an error */
                 Status = STATUS_FILE_CLOSED;
@@ -196,7 +184,7 @@
             else
             {
                 /* Unexpected disconnect by the remote host or initial read after a graceful disconnnect */
-                Status = FCB->PollStatus[FD_CLOSE_BIT];
+                Status = FCB->LastReceiveStatus;
             }
             NextIrp->IoStatus.Status = Status;
             NextIrp->IoStatus.Information = 0;
@@ -260,6 +248,21 @@
         FCB->PollState &= ~AFD_EVENT_RECEIVE;
     }

+    /* Signal FD_CLOSE if no buffered data remains and the socket can't receive any more */
+    if (CantReadMore(FCB))
+    {
+        if (FCB->LastReceiveStatus == STATUS_SUCCESS)
+        {
+            FCB->PollState |= AFD_EVENT_DISCONNECT;
+        }
+        else
+        {
+            FCB->PollState |= AFD_EVENT_CLOSE;
+        }
+        FCB->PollStatus[FD_CLOSE_BIT] = FCB->LastReceiveStatus;
+        PollReeval(FCB->DeviceExt, FCB->FileObject);
+    }
+
     AFD_DbgPrint(MID_TRACE,("RetStatus for irp %x is %x\n", Irp, RetStatus));

/* Sometimes we're called with a NULL Irp */

Modified: trunk/reactos/drivers/network/afd/include/afd.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/afd/include...
==============================================================================
--- trunk/reactos/drivers/network/afd/include/afd.h [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/afd/include/afd.h [iso-8859-1] Sun Dec 23 09:53:36 2012
@@ -203,6 +203,7 @@
     PVOID Context;
     DWORD PollState;
     NTSTATUS PollStatus[FD_MAX_EVENTS];
+    NTSTATUS LastReceiveStatus;
     UINT ContextSize;
     PVOID ConnectData;
     UINT FilledConnectData;

Modified: trunk/reactos/drivers/network/tcpip/include/titypes.h
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/drivers/network/tcpip/inclu...
==============================================================================
--- trunk/reactos/drivers/network/tcpip/include/titypes.h [iso-8859-1] (original)
+++ trunk/reactos/drivers/network/tcpip/include/titypes.h [iso-8859-1] Sun Dec 23 09:53:36 2012
@@ -278,6 +278,7 @@
     /* Socket state */
     BOOLEAN SendShutdown;
     BOOLEAN ReceiveShutdown;
+    NTSTATUS ReceiveShutdownStatus;

struct _CONNECTION_ENDPOINT *Next; /* Next connection in address file list */
 } CONNECTION_ENDPOINT, *PCONNECTION_ENDPOINT;

Modified: trunk/reactos/lib/drivers/ip/transport/tcp/event.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/drivers/ip/transport/tc...
==============================================================================
--- trunk/reactos/lib/drivers/ip/transport/tcp/event.c [iso-8859-1] (original)
+++ trunk/reactos/lib/drivers/ip/transport/tcp/event.c [iso-8859-1] Sun Dec 23 09:53:36 2012
@@ -260,61 +260,53 @@
 VOID
 TCPFinEventHandler(void *arg, const err_t err)
 {
-    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, TRUE);
-    }
-    else
-    {
-        /* First off all, remove the PCB pointer */
-        Connection->SocketContext = NULL;
-
-        /* 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);
-    }
+   PCONNECTION_ENDPOINT Connection = (PCONNECTION_ENDPOINT)arg, LastConnection;
+   const NTSTATUS Status = TCPTranslateError(err);
+   KIRQL OldIrql;
+
+   ASSERT(Connection->AddressFile);
+   ASSERT(err != ERR_OK);
+
+   /* First off all, remove the PCB pointer */
+   Connection->SocketContext = NULL;
+
+   /* 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

Modified: trunk/reactos/lib/drivers/lwip/src/rostcp.c
URL: http://svn.reactos.org/svn/reactos/trunk/reactos/lib/drivers/lwip/src/rostcp...
==============================================================================
--- trunk/reactos/lib/drivers/lwip/src/rostcp.c [iso-8859-1] (original)
+++ trunk/reactos/lib/drivers/lwip/src/rostcp.c [iso-8859-1] Sun Dec 23 09:53:36 2012
@@ -31,6 +31,9 @@
 extern NPAGED_LOOKASIDE_LIST MessageLookasideList;
 extern NPAGED_LOOKASIDE_LIST QueueEntryLookasideList;

+/* Required for ERR_T to NTSTATUS translation in receive error handling */
+NTSTATUS TCPTranslateError(const err_t err);
+
 static
 void
 LibTCPEmptyQueue(PCONNECTION_ENDPOINT Connection)
@@ -84,9 +87,8 @@
     PQUEUE_ENTRY qp;
     struct pbuf* p;
     NTSTATUS Status;
-    UINT ReadLength, PayloadLength;
+    UINT ReadLength, PayloadLength, Offset, Copied;
     KIRQL OldIrql;
-    PUCHAR Payload;

(*Received) = 0;

@@ -98,14 +100,14 @@
         {
             p = qp->p;

-            /* Calculate the payload first */
-            Payload = p->payload;
-            Payload += qp->Offset;
-            PayloadLength = p->len;
+            /* Calculate the payload length first */
+            PayloadLength = p->tot_len;
             PayloadLength -= qp->Offset;
+            Offset = qp->Offset;

/* Check if we're reading the whole buffer */
             ReadLength = MIN(PayloadLength, RecvLen);
+            ASSERT(ReadLength != 0);
             if (ReadLength != PayloadLength)
             {
                 /* Save this one for later */
@@ -116,10 +118,8 @@

UnlockObject(Connection, OldIrql);

-            /* Return to a lower IRQL because the receive buffer may be pageable memory */
-            RtlCopyMemory(RecvBuffer,
-                          Payload,
-                          ReadLength);
+            Copied = pbuf_copy_partial(p, RecvBuffer, ReadLength, Offset);
+            ASSERT(Copied == ReadLength);

LockObject(Connection, &OldIrql);

@@ -141,6 +141,7 @@
                 ASSERT(RecvLen == 0);
             }

+            ASSERT((*Received) != 0);
             Status = STATUS_SUCCESS;

if (!RecvLen)
@@ -150,7 +151,7 @@
     else
     {
         if (Connection->ReceiveShutdown)
-            Status = STATUS_SUCCESS;
+            Status = Connection->ReceiveShutdownStatus;
         else
             Status = STATUS_PENDING;
     }
@@ -202,8 +203,6 @@
 InternalRecvEventHandler(void *arg, PTCP_PCB pcb, struct pbuf *p, const err_t err)
 {
     PCONNECTION_ENDPOINT Connection = arg;
-    struct pbuf *pb;
-    ULONG RecvLen;

/* Make sure the socket didn't get closed */
     if (!arg)
@@ -216,19 +215,9 @@

if (p)
     {
-        pb = p;
-        RecvLen = 0;
-        while (pb != NULL)
-        {
-            /* Enqueue this buffer */
-            LibTCPEnqueuePacket(Connection, pb);
-            RecvLen += pb->len;
-
-            /* Advance and unchain the buffer */
-            pb = pbuf_dechain(pb);;
-        }
-
-        tcp_recved(pcb, RecvLen);
+        LibTCPEnqueuePacket(Connection, p);
+
+        tcp_recved(pcb, p->tot_len);

TCPRecvEventHandler(arg);
     }
@@ -239,7 +228,20 @@
          * whole socket here (by calling tcp_close()) as that would violate TCP specs
          */
         Connection->ReceiveShutdown = TRUE;
-        TCPFinEventHandler(arg, ERR_OK);
+        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)
+        {
+            /* Remotely initiated close */
+            TCPRecvEventHandler(arg);
+        }
+        else
+        {
+            /* Locally initated close */
+            TCPFinEventHandler(arg, ERR_CLSD);
+        }
     }

return ERR_OK;
@@ -279,10 +281,31 @@
 void
 InternalErrorEventHandler(void *arg, const err_t err)
 {
+    PCONNECTION_ENDPOINT Connection = arg;
+    KIRQL OldIrql;
+
     /* Make sure the socket didn't get closed */
     if (!arg) return;

-    TCPFinEventHandler(arg, err);
+    /* 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);
+    }
 }

static
@@ -621,7 +644,10 @@
     else
     {
         if (msg->Input.Shutdown.shut_rx)
+        {
             msg->Input.Shutdown.Connection->ReceiveShutdown = TRUE;
+            msg->Input.Shutdown.Connection->ReceiveShutdownStatus = STATUS_FILE_CLOSED;
+        }

if (msg->Input.Shutdown.shut_tx)
             msg->Input.Shutdown.Connection->SendShutdown = TRUE;
@@ -688,7 +714,7 @@
            msg->Output.Close.Error = tcp_close(pcb);

if (!msg->Output.Close.Error && msg->Input.Close.Callback)
-               TCPFinEventHandler(msg->Input.Close.Connection, ERR_OK);
+               TCPFinEventHandler(msg->Input.Close.Connection, ERR_CLSD);
            break;

default: