Skip to content

Commit

Permalink
Fix data loss or crash in TCPEndPoint with LwIP (#4022)
Browse files Browse the repository at this point in the history 
* Fix data loss or crash in TCPEndPoint with LwIP

#### Problem

Under the configuration CHIP_SYSTEM_CONFIG_USE_LWIP, in some cases where
the data size exceeds the TCP window size, TCPEndPoint can either die or
lose data when accounting of un-acked data falls out of sync.

#### Summary of Changes

Imported fix from Weave:

* 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().

Fixes #4013 - Data loss or crash in TCPEndPoint with LwIP

* Restyled by clang-format

Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@kpschoedel @restyled-commits
kpschoedel and restyled-commits authored Nov 30, 2020
1 parent 36845b6 commit 591208a
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 39 deletions.
164 changes: 127 additions & 37 deletions src/inet/TCPEndPoint.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -708,12 +708,6 @@ INET_ERROR TCPEndPoint::Send(PacketBuffer * data, bool push)

#if CHIP_SYSTEM_CONFIG_USE_LWIP

if (mUnsentQueue == NULL)
{
mUnsentQueue = data;
mUnsentOffset = 0;
}

#if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
if (!mUserTimeoutTimerRunning)
{
Expand Down Expand Up @@ -1175,6 +1169,10 @@ void TCPEndPoint::Init(InetLayer * inetLayer)
#endif // CHIP_SYSTEM_CONFIG_USE_SOCKETS

#endif // INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT

#if CHIP_SYSTEM_CONFIG_USE_LWIP
mUnackedLength = 0;
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP
}

INET_ERROR TCPEndPoint::DriveSending()
Expand All @@ -1195,50 +1193,62 @@ 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 chip::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.
VerifyOrDie(bufDataLen >= mUnsentOffset);
uint16_t sendLen = static_cast<uint16_t>(bufDataLen - mUnsentOffset);
uint16_t sendLen = static_cast<uint16_t>(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.
//
// This cast is safe, because mUnsentOffset + sendLen <=
// bufDataLen, which fits in uint16_t.
mUnsentOffset = static_cast<uint16_t>(mUnsentOffset + sendLen);
if (mUnsentOffset == bufDataLen)
{
mUnsentQueue = mUnsentQueue->Next();
mUnsentOffset = 0;
}

// Adjust the remaining window size.
//
// This cast is safe because sendLen <= sendWindowSize here.
sendWindowSize = static_cast<uint16_t>(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 = chip::System::MapErrorLwIP(lwipErr);
break;
}
// Start accounting for the data sent as yet-to-be-acked.
// This cast is safe, because mUnackedLength + sendLen <= bufDataLen, which fits in uint16_t.
mUnackedLength = static_cast<uint16_t>(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.
// This cast is safe, because unsentOffset + sendLen <= bufDataLen, which fits in uint16_t.
unsentOffset = static_cast<uint16_t>(unsentOffset + sendLen);
if (unsentOffset == bufDataLen)
{
currentUnsentBuf = currentUnsentBuf->Next();
unsentOffset = 0;
}

// Adjust the remaining window size.
sendWindowSize = static_cast<uint16_t>(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.
Expand All @@ -1256,7 +1266,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)
Expand Down Expand Up @@ -1562,6 +1572,9 @@ INET_ERROR TCPEndPoint::DoClose(INET_ERROR err, bool suppressCallback)
mSendQueue = nullptr;
PacketBuffer::Free(mRcvQueue);
mRcvQueue = nullptr;
#if CHIP_SYSTEM_CONFIG_USE_LWIP
mUnackedLength = 0;
#endif // CHIP_SYSTEM_CONFIG_USE_LWIP

// Call the appropriate app callback if allowed.
if (!suppressCallback)
Expand Down Expand Up @@ -1741,6 +1754,67 @@ void TCPEndPoint::RestartTCPUserTimeoutTimer()

#if CHIP_SYSTEM_CONFIG_USE_LWIP

uint16_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 static_cast<uint16_t>(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.
chip::System::PacketBuffer * currentUnsentBuf = mSendQueue;
uint16_t unsentOffset = 0;
uint16_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 = static_cast<uint16_t>(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!
Expand Down Expand Up @@ -1832,16 +1906,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)
{
ChipLogError(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)
{
ChipLogError(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);
mSendQueue = mSendQueue->Consume(lenSent);
mUnackedLength = static_cast<uint16_t>(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.

Expand Down Expand Up @@ -1869,12 +1959,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);
}
}
Expand Down
12 changes: 10 additions & 2 deletions src/inet/TCPEndPoint.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -634,9 +634,17 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis
void StopConnectTimer();

#if CHIP_SYSTEM_CONFIG_USE_LWIP
chip::System::PacketBuffer * mUnsentQueue;
uint16_t mUnsentOffset;
struct BufferOffset
{
const chip::System::PacketBuffer * buffer;
uint16_t offset;
};

uint16_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.

uint16_t RemainingToSend();
BufferOffset FindStartOfUnsent();
INET_ERROR GetPCB(IPAddressType addrType);
void HandleDataSent(uint16_t len);
void HandleDataReceived(chip::System::PacketBuffer * buf);
Expand Down

0 comments on commit 591208a

Please sign in to comment.