[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

svn path=/trunk/; revision=57975

reactos/drivers/network/afd/afd/main.c

@@ -610,6 +610,7 @@ DisconnectComplete(PDEVICE_OBJECT DeviceObject,
         /* 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 @@ AfdDisconnect(PDEVICE_OBJECT DeviceObject, PIRP Irp,
         /* 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 */
+        /* Set a successful receive status to indicate a shutdown on overread */
-        FCB->PollStatus[FD_CLOSE_BIT] = STATUS_SUCCESS;
+        FCB->LastReceiveStatus = STATUS_SUCCESS;
 
         /* Clear the receive event */
         FCB->PollState &= ~AFD_EVENT_RECEIVE;

reactos/drivers/network/afd/afd/read.c

@@ -52,11 +52,12 @@ static VOID RefillSocketBuffer( PAFD_FCB FCB )
 
 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;
+        /* The received data is discarded */
-        FCB->Recv.BytesUsed = 0;
     }
     /* Receive successful */
     else if (Status == STATUS_SUCCESS)
@@ -67,30 +68,20 @@ static VOID HandleReceiveComplete( PAFD_FCB FCB, NTSTATUS Status, ULONG_PTR Info
         /* 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 );
         }
-
+        else
-        /* Issue another receive IRP to keep the buffer well stocked */
+        {
-        RefillSocketBuffer(FCB);
+            /* Issue another receive IRP to keep the buffer well stocked */
+            RefillSocketBuffer(FCB);
+        }
     }
     /* Receive failed with no data (unexpected closure) */
     else
     {
-        FCB->Recv.BytesUsed = 0;
+        /* Previously received data remains intact */
-        FCB->Recv.Content = 0;
         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 @@ static NTSTATUS ReceiveActivity( PAFD_FCB FCB, PIRP Irp ) {
 
     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 @@ static NTSTATUS ReceiveActivity( PAFD_FCB FCB, PIRP Irp ) {
                                     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 @@ static NTSTATUS ReceiveActivity( PAFD_FCB FCB, PIRP Irp ) {
             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 @@ static NTSTATUS ReceiveActivity( PAFD_FCB FCB, PIRP Irp ) {
         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 */

reactos/drivers/network/afd/include/afd.h

@@ -203,6 +203,7 @@ typedef struct _AFD_FCB {
     PVOID Context;
     DWORD PollState;
     NTSTATUS PollStatus[FD_MAX_EVENTS];
+    NTSTATUS LastReceiveStatus;
     UINT ContextSize;
     PVOID ConnectData;
     UINT FilledConnectData;

reactos/drivers/network/tcpip/include/titypes.h

@@ -278,6 +278,7 @@ typedef struct _CONNECTION_ENDPOINT {
     /* Socket state */
     BOOLEAN SendShutdown;
     BOOLEAN ReceiveShutdown;
+    NTSTATUS ReceiveShutdownStatus;
 
     struct _CONNECTION_ENDPOINT *Next; /* Next connection in address file list */
 } CONNECTION_ENDPOINT, *PCONNECTION_ENDPOINT;

reactos/lib/drivers/ip/transport/tcp/event.c

@@ -260,61 +260,53 @@ FlushAllQueues(PCONNECTION_ENDPOINT Connection, NTSTATUS Status)
 VOID
 TCPFinEventHandler(void *arg, const err_t err)
 {
-    PCONNECTION_ENDPOINT Connection = (PCONNECTION_ENDPOINT)arg, LastConnection;
+   PCONNECTION_ENDPOINT Connection = (PCONNECTION_ENDPOINT)arg, LastConnection;
-    const NTSTATUS Status = TCPTranslateError(err);
+   const NTSTATUS Status = TCPTranslateError(err);
-    KIRQL OldIrql;
+   KIRQL OldIrql;
 
-    ASSERT(Connection->AddressFile);
+   ASSERT(Connection->AddressFile);
+   ASSERT(err != ERR_OK);
 
-    /* Check if this was a partial socket closure */
+   /* First off all, remove the PCB pointer */
-    if (err == ERR_OK && Connection->SocketContext)
+   Connection->SocketContext = NULL;
-    {
-        /* 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 */
+   /* Complete all outstanding requests now */
-        FlushAllQueues(Connection, Status);
+   FlushAllQueues(Connection, Status);
 
-        LockObject(Connection, &OldIrql);
+   LockObject(Connection, &OldIrql);
 
-        LockObjectAtDpcLevel(Connection->AddressFile);
+   LockObjectAtDpcLevel(Connection->AddressFile);
 
-        /* Unlink this connection from the address file */
+   /* Unlink this connection from the address file */
-        if (Connection->AddressFile->Connection == Connection)
+   if (Connection->AddressFile->Connection == Connection)
-        {
+   {
-            Connection->AddressFile->Connection = Connection->Next;
+       Connection->AddressFile->Connection = Connection->Next;
-            DereferenceObject(Connection);
+       DereferenceObject(Connection);
-        }
+   }
-        else if (Connection->AddressFile->Listener == Connection)
+   else if (Connection->AddressFile->Listener == Connection)
-        {
+   {
-            Connection->AddressFile->Listener = NULL;
+       Connection->AddressFile->Listener = NULL;
-            DereferenceObject(Connection);
+       DereferenceObject(Connection);
-        }
+   }
-        else
+   else
-        {
+   {
-            LastConnection = Connection->AddressFile->Connection;
+       LastConnection = Connection->AddressFile->Connection;
-            while (LastConnection->Next != Connection && LastConnection->Next != NULL)
+       while (LastConnection->Next != Connection && LastConnection->Next != NULL)
-                LastConnection = LastConnection->Next;
+           LastConnection = LastConnection->Next;
-            if (LastConnection->Next == Connection)
+       if (LastConnection->Next == Connection)
-            {
+       {
-                LastConnection->Next = Connection->Next;
+           LastConnection->Next = Connection->Next;
-                DereferenceObject(Connection);
+           DereferenceObject(Connection);
-            }
+       }
-        }
+   }
 
-        UnlockObjectFromDpcLevel(Connection->AddressFile);
+   UnlockObjectFromDpcLevel(Connection->AddressFile);
 
-        /* Remove the address file from this connection */
+   /* Remove the address file from this connection */
-        DereferenceObject(Connection->AddressFile);
+   DereferenceObject(Connection->AddressFile);
-        Connection->AddressFile = NULL;
+   Connection->AddressFile = NULL;
 
-        UnlockObject(Connection, OldIrql);
+   UnlockObject(Connection, OldIrql);
-    }
 }
     
 VOID

reactos/lib/drivers/lwip/src/rostcp.c

@@ -31,6 +31,9 @@ extern KEVENT TerminationEvent;
 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 @@ NTSTATUS LibTCPGetDataFromConnectionQueue(PCONNECTION_ENDPOINT Connection, PUCHA
     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 @@ NTSTATUS LibTCPGetDataFromConnectionQueue(PCONNECTION_ENDPOINT Connection, PUCHA
         {
             p = qp->p;
 
-            /* Calculate the payload first */
+            /* Calculate the payload length first */
-            Payload = p->payload;
+            PayloadLength = p->tot_len;
-            Payload += qp->Offset;
-            PayloadLength = p->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 @@ NTSTATUS LibTCPGetDataFromConnectionQueue(PCONNECTION_ENDPOINT Connection, PUCHA
 
             UnlockObject(Connection, OldIrql);
 
-            /* Return to a lower IRQL because the receive buffer may be pageable memory */
+            Copied = pbuf_copy_partial(p, RecvBuffer, ReadLength, Offset);
-            RtlCopyMemory(RecvBuffer,
+            ASSERT(Copied == ReadLength);
-                          Payload,
-                          ReadLength);
 
             LockObject(Connection, &OldIrql);
 
@@ -141,6 +141,7 @@ NTSTATUS LibTCPGetDataFromConnectionQueue(PCONNECTION_ENDPOINT Connection, PUCHA
                 ASSERT(RecvLen == 0);
             }
 
+            ASSERT((*Received) != 0);
             Status = STATUS_SUCCESS;
 
             if (!RecvLen)
@@ -150,7 +151,7 @@ NTSTATUS LibTCPGetDataFromConnectionQueue(PCONNECTION_ENDPOINT Connection, PUCHA
     else
     {
         if (Connection->ReceiveShutdown)
-            Status = STATUS_SUCCESS;
+            Status = Connection->ReceiveShutdownStatus;
         else
             Status = STATUS_PENDING;
     }
@@ -202,8 +203,6 @@ err_t
 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 @@ InternalRecvEventHandler(void *arg, PTCP_PCB pcb, struct pbuf *p, const err_t er
 
     if (p)
     {
-        pb = p;
+        LibTCPEnqueuePacket(Connection, p);
-        RecvLen = 0;
-        while (pb != NULL)
-        {
-            /* Enqueue this buffer */
-            LibTCPEnqueuePacket(Connection, pb);
-            RecvLen += pb->len;
 
-            /* Advance and unchain the buffer */
+        tcp_recved(pcb, p->tot_len);
-            pb = pbuf_dechain(pb);;
-        }
-
-        tcp_recved(pcb, RecvLen);
 
         TCPRecvEventHandler(arg);
     }
@@ -239,7 +228,20 @@ InternalRecvEventHandler(void *arg, PTCP_PCB pcb, struct pbuf *p, const err_t er
          * 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 @@ static
 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 @@ LibTCPShutdownCallback(void *arg)
     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 @@ LibTCPCloseCallback(void *arg)
            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: