Data loss or crash in TCPEndPoint with LwIP

[kpschoedel]

Problem

Under the configuration CHIP_SYSTEM_CONFIG_USE_LWIP, TCPEndPoint::DriveSending() can fail in various ways.

For an illustration with simple arithmetic consider a PacketBuffer capacity of 1600 bytes and a window size of 800 bytes.

1. Code calls TCPEndPoint::Send() with 1600 bytes of data. mSendQueue and mUnsentQueue both point to the start of this data.

     mSendQueue
     pbuf::payload
     ↓
    [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓]
     ↑
     mUnsentOffset
     mUnsentQueue

2. In TCPEndPoint::DriveSending(), we have bufDataLen of 1600 and sendLen of 800 (clamped by sendWindowSize), so mUnsentOffet becomes 800. The 800 bytes are sent via LwIP and DriveSending() completes because sendWindowSize is used up.

     mSendQueue
     pbuf::payload
     ↓
    [▒▒▒▒▒▒▒▒▓▓▓▓▓▓▓▓]
     ↑       ↑
     ⋮       mUnsentOffset
     mUnsentQueue

3. LwIP calls back to TCPEndPoint::HandleDataSent(800), which calls mSendQueue->Consume(800):

     mSendQueue
     ⋮       pbuf::payload
     ↓       ↓
    [░░░░░░░░▓▓▓▓▓▓▓▓]
     ↑       ↑
     ⋮       mUnsentOffset
     mUnsentQueue

5. Since mUnsentQueue != NULL, HandleDataSent() calls DriveSending() again.

    1204: bufDataLen = mUnsentQueue->DataLength();               // is 800
    1208: sendData = mUnsentQueue->Start() + mUnsentOffset;      // at the end!
    1211: sendLen = bufDataLen - mUnsentOffset;                  // 0 technically correct

(Note: a different window size could instead trigger the VerifyOrDie at line 1210)

     mSendQueue
     ⋮       pbuf::payload
     ↓       ↓
    [░░░░░░░░▓▓▓▓▓▓▓▓]
     ↑       ↑       ↑
     ⋮      ⋮       sendData
     ⋮      mUnsentOffset
     mUnsentQueue

1220: mUnsentOffset = mUnsentOffset + sendLen                 // remains 800
1221: if (mUnsentOffset == bufDataLen) {                      // oops
1223:   mUnsentQueue  = mUnsentQueue->Next();                 // nullptr
1224:   mUnsentOffset = 0;

     mSendQueue
     ⋮       pbuf::payload
     ↓       ↓
    [░░░░░░░░▓▓▓▓▓▓▓▓]
                     ↑
              sendData

1230: sendWindowSize = sendWindowSize - sendLen               // remains 800
1233: canSend = (mUnsentQueue != NULL && sendWindowSize > 0); // false
1236: lwipErr = tcp_write(mTCP, sendData, sendLen, …          // sends 0 bytes

6. Since canSend is false, we leave DriveSending().
   Note that since we sent 0 bytes, HandleDataSent() will never advance pbuf::payload and mSendQueue.

     mSendQueue
     ⋮       pbuf::payload
     ↓       ↓
    [░░░░░░░░▓▓▓▓▓▓▓▓]

7. Code calls TCPEndPoint::Send() with another 1000 bytes of data;
   this is chained to the end of mSendQueue, but mUnsentQueue is null,
   so it is set to the new buffer.

     mSendQueue
     ⋮       pbuf::payload
     ↓       ↓
    [░░░░░░░░▓▓▓▓▓▓▓▓]→[▓▓▓▓▓▓▓▓▓▓_______]
                        ↑
                        mUnsentOffset
                        mUnsentQueue

8. Now, in DriveSending(), we have sendData pointing to the new buffer.

     mSendQueue
     ⋮       pbuf::payload
     ↓       ↓
    [░░░░░░░░▓▓▓▓▓▓▓▓]→[▒▒▒▒▒▒▒▒▓▓_______]
             ↑      ↑   ↑       ↑
             └UNSENT┘   ⋮       mUnsentOffset
                        mUnsentQueue

9. LwIP calls back to TCPEndPoint::HandleDataSent(800), which calls mSendQueue->Consume(800):
   Data loss!

     mSendQueue
     ↓
    [▒▒▒▒▒▒▒▒▓▓_______]
     ↑       ↑
     ⋮       mUnsentOffset
     mUnsentQueue

Proposed Solution

Incorporate a fix from Weave that examines the current state to determine what next to send.

[issue-label-bot]

Issue-Label Bot is automatically applying the label bug to this issue, with a confidence of 0.93. Please mark this comment with 👍 or 👎 to give our bot feedback!

Links: app homepage, dashboard and code for this bot.