Fix Weave TCPEndPoint crash in some lwIP cases

* Weave, when using an lwIP networking interface, employs
  the raw lwIP callback-based scheme to handle TCP sends.
  In order to avoid multiple copies of packet buffers on
  small devices, the TCP sends are done in-place over the
  incoming packet buffers. Packet buffers must be retained
  in full until acknowledgement has been received by lwIP.
* The existing logic had a corner case where if there was
  more data queued than the local TCP send window, the
  accounting of buffers and lengths to retain the un-acked
  buffers could get out of balance, leading to an eventual
  crash. This was root caused to an invalid assumption that
  the packet buffer chain did not need traversal for offset
  readjustment (which is true in most cases, only by
  coincidence).
* This change removes separate accounting of the unsent
  data position and replaces it with simple counting of
  sent-but-not-acked data and a skip-loop at the start
  of DriveSending().

Author: holbrookt

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);