Merge pull request #634 from holbrookt/fix-tcp-endpoint-crash

Fix Weave TCPEndPoint crash in some lwIP cases

Author: robszewczyk

src/inet/TCPEndPoint.cpp

@@ -708,12 +708,6 @@ INET_ERROR TCPEndPoint::Send(PacketBuffer *data, bool push)
 
 #if WEAVE_SYSTEM_CONFIG_USE_LWIP
 
-    if (mUnsentQueue == NULL)
-    {
-        mUnsentQueue = data;
-        mUnsentOffset = 0;
-    }
-
 #if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
     if (!mUserTimeoutTimerRunning)
     {
@@ -1199,6 +1193,10 @@ void TCPEndPoint::Init(InetLayer *inetLayer)
 #endif // WEAVE_SYSTEM_CONFIG_USE_SOCKETS
 
 #endif // INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
+
+#if WEAVE_SYSTEM_CONFIG_USE_LWIP
+    mUnackedLength = 0;
+#endif // WEAVE_SYSTEM_CONFIG_USE_LWIP
 }
 
 INET_ERROR TCPEndPoint::DriveSending()
@@ -1219,44 +1217,60 @@ INET_ERROR TCPEndPoint::DriveSending()
         uint16_t sendWindowSize = tcp_sndbuf(mTCP);
 
         // If there's data to be sent and the send window is open...
-        bool canSend = (mUnsentQueue != NULL && sendWindowSize > 0);
+        bool canSend = (RemainingToSend() > 0 && sendWindowSize > 0);
         if (canSend)
         {
+            // Find first packet buffer with remaining data to send by skipping
+            // all sent but un-acked data.
+            TCPEndPoint::BufferOffset startOfUnsent = FindStartOfUnsent();
+            const Weave::System::PacketBuffer* currentUnsentBuf = startOfUnsent.buffer;
+            uint16_t unsentOffset = startOfUnsent.offset;
+
             // While there's data to be sent and a window to send it in...
             do
             {
-                uint16_t bufDataLen = mUnsentQueue->DataLength();
+                VerifyOrDie(currentUnsentBuf != NULL);
+
+                uint16_t bufDataLen = currentUnsentBuf->DataLength();
 
                 // Get a pointer to the start of unsent data within the first buffer on the unsent queue.
-                uint8_t *sendData = mUnsentQueue->Start() + mUnsentOffset;
+                const uint8_t *sendData = currentUnsentBuf->Start() + unsentOffset;
 
                 // Determine the amount of data to send from the current buffer.
-                uint16_t sendLen = bufDataLen - mUnsentOffset;
+                uint16_t sendLen = bufDataLen - unsentOffset;
                 if (sendLen > sendWindowSize)
                     sendLen = sendWindowSize;
 
-                // Adjust the unsent data offset by the length of data to be written. If the entire buffer
-                // has been sent advance to the next one.
-                mUnsentOffset += sendLen;
-                if (mUnsentOffset == bufDataLen)
-                {
-                    mUnsentQueue = mUnsentQueue->Next();
-                    mUnsentOffset = 0;
-                }
-
-                // Adjust the remaining window size.
-                sendWindowSize -= sendLen;
-
-                // Determine if there's more data to be sent after this buffer.
-                canSend = (mUnsentQueue != NULL && sendWindowSize > 0);
-
                 // Call LwIP to queue the data to be sent, telling it if there's more data to come.
+                // Data is queued in-place as a reference within the source packet buffer. It is
+                // critical that the underlying packet buffer not be freed until the data
+                // is acknowledged, otherwise retransmissions could use an invalid
+                // backing. Using TCP_WRITE_FLAG_COPY would eliminate this requirement, but overall
+                // requires many more memory allocations which may be problematic when very
+                // memory-constrained or when using pool-based allocations.
                 lwipErr = tcp_write(mTCP, sendData, sendLen, (canSend) ? TCP_WRITE_FLAG_MORE : 0);
                 if (lwipErr != ERR_OK)
                 {
                     err = Weave::System::MapErrorLwIP(lwipErr);
                     break;
                 }
+                // Start accounting for the data sent as yet-to-be-acked.
+                mUnackedLength += sendLen;
+
+                // Adjust the unsent data offset by the length of data that was written.
+                // If the entire buffer has been sent advance to the next one.
+                unsentOffset += sendLen;
+                if (unsentOffset == bufDataLen)
+                {
+                    currentUnsentBuf = currentUnsentBuf->Next();
+                    unsentOffset = 0;
+                }
+
+                // Adjust the remaining window size.
+                sendWindowSize -= sendLen;
+
+                // Determine if there's more data to be sent after this buffer.
+                canSend = (RemainingToSend() > 0 && sendWindowSize > 0);
             } while (canSend);
 
             // Call LwIP to send the queued data.
@@ -1274,7 +1288,7 @@ INET_ERROR TCPEndPoint::DriveSending()
         if (err == INET_NO_ERROR)
         {
             // If in the SendShutdown state and the unsent queue is now empty, shutdown the PCB for sending.
-            if (State == kState_SendShutdown && mUnsentQueue == NULL)
+            if (State == kState_SendShutdown && (RemainingToSend() == 0))
             {
                 lwipErr = tcp_shutdown(mTCP, 0, 1);
                 if (lwipErr != ERR_OK)
@@ -1571,6 +1585,9 @@ INET_ERROR TCPEndPoint::DoClose(INET_ERROR err, bool suppressCallback)
         mSendQueue = NULL;
         PacketBuffer::Free(mRcvQueue);
         mRcvQueue = NULL;
+#if WEAVE_SYSTEM_CONFIG_USE_LWIP
+        mUnackedLength = 0;
+#endif // WEAVE_SYSTEM_CONFIG_USE_LWIP
 
         // Call the appropriate app callback if allowed.
         if (!suppressCallback)
@@ -1752,6 +1769,67 @@ void TCPEndPoint::RestartTCPUserTimeoutTimer()
 
 #if WEAVE_SYSTEM_CONFIG_USE_LWIP
 
+uint32_t TCPEndPoint::RemainingToSend()
+{
+    if (mSendQueue == NULL)
+    {
+        return 0;
+    }
+    else
+    {
+        // We can never have reported more unacked data than there is pending
+        // in the send queue! This would indicate a critical accounting bug.
+        VerifyOrDie(mUnackedLength <= mSendQueue->TotalLength());
+
+        return mSendQueue->TotalLength() - mUnackedLength;
+    }
+}
+
+TCPEndPoint::BufferOffset TCPEndPoint::FindStartOfUnsent()
+{
+    // Find first packet buffer with remaining data to send by skipping
+    // all sent but un-acked data. This is necessary because of the Consume()
+    // call in HandleDataSent(), which potentially releases backing memory for
+    // fully-sent packet buffers, causing an invalidation of all possible
+    // offsets one might have cached. The TCP acnowledgements may come back
+    // with a variety of sizes depending on prior activity, and size of the
+    // send window. The only way to ensure we get the correct offsets into
+    // unsent data while retaining the buffers that have un-acked data is to
+    // traverse all sent-but-unacked data in the chain to reach the beginning
+    // of ready-to-send data.
+    Weave::System::PacketBuffer* currentUnsentBuf = mSendQueue;
+    uint16_t unsentOffset = 0;
+    uint32_t leftToSkip = mUnackedLength;
+
+    VerifyOrDie(leftToSkip < mSendQueue->TotalLength());
+
+    while (leftToSkip > 0)
+    {
+        VerifyOrDie(currentUnsentBuf != NULL);
+        uint16_t bufDataLen = currentUnsentBuf->DataLength();
+        if (leftToSkip >= bufDataLen)
+        {
+            // We have more to skip than current packet buffer size.
+            // Follow the chain to continue.
+            currentUnsentBuf = currentUnsentBuf->Next();
+            leftToSkip -= bufDataLen;
+        }
+        else
+        {
+            // Done skipping all data, currentUnsentBuf is first packet buffer
+            // containing unsent data.
+            unsentOffset = leftToSkip;
+            leftToSkip = 0;
+        }
+    }
+
+    TCPEndPoint::BufferOffset startOfUnsent;
+    startOfUnsent.buffer = currentUnsentBuf;
+    startOfUnsent.offset = unsentOffset;
+
+    return startOfUnsent;
+}
+
 INET_ERROR TCPEndPoint::GetPCB(IPAddressType addrType)
 {
     // IMMPORTANT: This method MUST be called with the LwIP stack LOCKED!
@@ -1843,16 +1921,32 @@ void TCPEndPoint::HandleDataSent(uint16_t lenSent)
 {
     if (IsConnected())
     {
+        // Ensure we do not have internal inconsistency in the lwIP, which
+        // could cause invalid pointer accesses.
+        if (lenSent > mUnackedLength)
+        {
+            WeaveLogError(Inet, "Got more ACKed bytes (%d) than were pending (%d)", (int)lenSent, (int)mUnackedLength);
+            DoClose(INET_ERROR_UNEXPECTED_EVENT, false);
+            return;
+        }
+        else if (mSendQueue == NULL)
+        {
+            WeaveLogError(Inet, "Got ACK for %d bytes but data backing gone", (int)lenSent);
+            DoClose(INET_ERROR_UNEXPECTED_EVENT, false);
+            return;
+        }
+
         // Consume data off the head of the send queue equal to the amount of data being acknowledged.
         mSendQueue = mSendQueue->Consume(lenSent);
+        mUnackedLength -= lenSent;
 
 #if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
         // Only change the UserTimeout timer if lenSent > 0,
         // indicating progress being made in sending data
         // across.
         if (lenSent > 0)
         {
-            if (mSendQueue == NULL)
+            if (RemainingToSend() == 0)
             {
                 // If the output queue has been flushed then stop the timer.
 
@@ -1880,12 +1974,12 @@ void TCPEndPoint::HandleDataSent(uint16_t lenSent)
         if (OnDataSent != NULL)
             OnDataSent(this, lenSent);
 
-        // If unsent data exists, attempt to sent it now...
-        if (mUnsentQueue != NULL)
+        // If unsent data exists, attempt to send it now...
+        if (RemainingToSend() > 0)
             DriveSending();
 
         // If in the closing state and the send queue is now empty, attempt to transition to closed.
-        if (State == kState_Closing && mSendQueue == NULL)
+        if ((State == kState_Closing) && (RemainingToSend() == 0))
             DoClose(INET_NO_ERROR, false);
     }
 }

src/inet/TCPEndPoint.h

@@ -621,9 +621,16 @@ class NL_DLL_EXPORT TCPEndPoint : public EndPointBasis
     void StopConnectTimer(void);
 
 #if WEAVE_SYSTEM_CONFIG_USE_LWIP
-    Weave::System::PacketBuffer *mUnsentQueue;
-    uint16_t mUnsentOffset;
+    struct BufferOffset {
+        const Weave::System::PacketBuffer *buffer;
+        uint32_t offset;
+    };
 
+    uint32_t mUnackedLength;                            // Amount sent but awaiting ACK. Used as a form of reference count
+                                                        // to hang-on to backing packet buffers until they are no longer needed.
+
+    uint32_t RemainingToSend();
+    BufferOffset FindStartOfUnsent();
     INET_ERROR GetPCB(IPAddressType addrType);
     void HandleDataSent(uint16_t len);
     void HandleDataReceived(Weave::System::PacketBuffer *buf);