Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adjust storage and processing in SystemPacketBuffer and associated code for large payloads. #31776

Merged
merged 1 commit into from
Feb 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/app/BufferedReadCallback.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ CHIP_ERROR BufferedReadCallback::GenerateListTLV(TLV::ScopedBufferTLVReader & aR
//
// To avoid that, a single contiguous buffer is the best likely approach for now.
//
uint32_t totalBufSize = 0;
size_t totalBufSize = 0;
for (const auto & packetBuffer : mBufferedList)
{
totalBufSize += packetBuffer->TotalLength();
Expand Down
3 changes: 2 additions & 1 deletion src/app/server/EchoHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ chip::Protocols::Echo::EchoServer gEchoServer;
*/
void HandleEchoRequestReceived(chip::Messaging::ExchangeContext * ec, chip::System::PacketBufferHandle && payload)
{
ChipLogProgress(AppServer, "Echo Request, len=%u ... sending response.\n", payload->DataLength());
ChipLogProgress(AppServer, "Echo Request, len=%" PRIu32 "... sending response.\n",
static_cast<uint32_t>(payload->DataLength()));
}

} // namespace
Expand Down
10 changes: 5 additions & 5 deletions src/ble/BtpEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ CHIP_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle &&
// mRxFragnentSize may be smaller than the characteristic size. Make sure
// we're not truncating to a data length smaller than what we have already consumed.
VerifyOrExit(reader.OctetsRead() <= mRxFragmentSize, err = BLE_ERROR_REASSEMBLER_INCORRECT_STATE);
data->SetDataLength(chip::min(data->DataLength(), mRxFragmentSize));
data->SetDataLength(chip::min(data->DataLength(), static_cast<size_t>(mRxFragmentSize)));

// Now mark the bytes we consumed as consumed.
data->ConsumeHead(static_cast<uint16_t>(reader.OctetsRead()));
Expand Down Expand Up @@ -374,11 +374,11 @@ CHIP_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle &&
if (rx_flags.Has(HeaderFlags::kEndMessage))
{
// Trim remainder, if any, of the received packet buffer based on sender-specified length of reassembled message.
int padding = mRxBuf->DataLength() - mRxLength;
int padding = static_cast<uint16_t>(mRxBuf->DataLength()) - mRxLength;

if (padding > 0)
{
mRxBuf->SetDataLength(mRxLength);
mRxBuf->SetDataLength(static_cast<size_t>(mRxLength));
}

// Ensure all received fragments add up to sender-specified total message size.
Expand All @@ -403,7 +403,7 @@ CHIP_ERROR BtpEngine::HandleCharacteristicReceived(System::PacketBufferHandle &&
}
if (!mRxBuf.IsNull())
{
ChipLogError(Ble, "With rx buf data length = %u", mRxBuf->DataLength());
ChipLogError(Ble, "With rx buf data length = %u", static_cast<unsigned>(mRxBuf->DataLength()));
}
LogState();

Expand Down Expand Up @@ -456,7 +456,7 @@ bool BtpEngine::HandleCharacteristicSend(System::PacketBufferHandle data, bool s

mTxBuf = std::move(data);
mTxState = kState_InProgress;
mTxLength = mTxBuf->DataLength();
mTxLength = static_cast<uint16_t>(mTxBuf->DataLength());

ChipLogDebugBtpEngine(Ble, ">>> CHIPoBle preparing to send whole message:");
PrintBufDebug(mTxBuf);
Expand Down
8 changes: 4 additions & 4 deletions src/inet/TCPEndPoint.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ CHIP_ERROR TCPEndPoint::SetReceivedDataForTesting(System::PacketBufferHandle &&
return CHIP_NO_ERROR;
}

uint32_t TCPEndPoint::PendingSendLength()
size_t TCPEndPoint::PendingSendLength()
{
if (!mSendQueue.IsNull())
{
Expand All @@ -135,7 +135,7 @@ uint32_t TCPEndPoint::PendingSendLength()
return 0;
}

uint32_t TCPEndPoint::PendingReceiveLength()
size_t TCPEndPoint::PendingReceiveLength()
{
if (!mRcvQueue.IsNull())
{
Expand Down Expand Up @@ -333,8 +333,8 @@ void TCPEndPoint::DriveReceiving()
{
// Acknowledgement is done after handling the buffers to allow the
// application processing to throttle flow.
uint16_t ackLength = mRcvQueue->TotalLength();
CHIP_ERROR err = OnDataReceived(this, std::move(mRcvQueue));
size_t ackLength = mRcvQueue->TotalLength();
CHIP_ERROR err = OnDataReceived(this, std::move(mRcvQueue));
if (err != CHIP_NO_ERROR)
{
DoClose(err, false);
Expand Down
8 changes: 4 additions & 4 deletions src/inet/TCPEndPoint.h
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
* received. The operational semantics are undefined if \c len is larger
* than the total outstanding unacknowledged received data.
*/
virtual CHIP_ERROR AckReceive(uint16_t len) = 0;
virtual CHIP_ERROR AckReceive(size_t len) = 0;

/**
* @brief Set the receive queue, for testing.
Expand All @@ -295,15 +295,15 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
*
* @return Number of untransmitted bytes in the transmit queue.
*/
uint32_t PendingSendLength();
size_t PendingSendLength();

/**
* @brief Extract the length of the unacknowledged receive data.
*
* @return Number of bytes in the receive queue that have not yet been
* acknowledged with <tt>AckReceive(uint16_t len)</tt>.
*/
uint32_t PendingReceiveLength();
size_t PendingReceiveLength();

/**
* @brief Initiate TCP half close, in other words, finished with sending.
Expand Down Expand Up @@ -447,7 +447,7 @@ class DLL_EXPORT TCPEndPoint : public EndPointBasis<TCPEndPoint>
* is the length of the message text added to the TCP transmit window,
* which are eligible for sending by the underlying network stack.
*/
typedef void (*OnDataSentFunct)(TCPEndPoint * endPoint, uint16_t len);
typedef void (*OnDataSentFunct)(TCPEndPoint * endPoint, size_t len);

/**
* The endpoint's message text transmission event handling function
Expand Down
10 changes: 6 additions & 4 deletions src/inet/TCPEndPointImplLwIP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ CHIP_ERROR TCPEndPointImplLwIP::DriveSendingImpl()
{
VerifyOrDie(!startOfUnsent.buffer.IsNull());

uint16_t bufDataLen = startOfUnsent.buffer->DataLength();
uint16_t bufDataLen = static_cast<uint16_t>(startOfUnsent.buffer->DataLength());

// Get a pointer to the start of unsent data within the first buffer on the unsent queue.
const uint8_t * sendData = startOfUnsent.buffer->Start() + startOfUnsent.offset;
Expand Down Expand Up @@ -503,16 +503,18 @@ void TCPEndPointImplLwIP::DoCloseImpl(CHIP_ERROR err, State oldState)
}
}

CHIP_ERROR TCPEndPointImplLwIP::AckReceive(uint16_t len)
CHIP_ERROR TCPEndPointImplLwIP::AckReceive(size_t len)
{
VerifyOrReturnError(IsConnected(), CHIP_ERROR_INCORRECT_STATE);
CHIP_ERROR res = CHIP_NO_ERROR;

VerifyOrReturnError(len < UINT16_MAX, CHIP_ERROR_INVALID_ARGUMENT);

// Lock LwIP stack
LOCK_TCPIP_CORE();

if (mTCP != nullptr)
tcp_recved(mTCP, len);
tcp_recved(mTCP, static_cast<uint16_t>(len));
else
res = CHIP_ERROR_CONNECTION_ABORTED;

Expand Down Expand Up @@ -570,7 +572,7 @@ TCPEndPointImplLwIP::BufferOffset TCPEndPointImplLwIP::FindStartOfUnsent()
while (leftToSkip > 0)
{
VerifyOrDie(!startOfUnsent.buffer.IsNull());
uint16_t bufDataLen = startOfUnsent.buffer->DataLength();
uint16_t bufDataLen = static_cast<uint16_t>(startOfUnsent.buffer->DataLength());
if (leftToSkip >= bufDataLen)
{
// We have more to skip than current packet buffer size.
Expand Down
2 changes: 1 addition & 1 deletion src/inet/TCPEndPointImplLwIP.h
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class TCPEndPointImplLwIP : public TCPEndPoint, public EndPointStateLwIP
CHIP_ERROR EnableNoDelay() override;
CHIP_ERROR EnableKeepAlive(uint16_t interval, uint16_t timeoutCount) override;
CHIP_ERROR DisableKeepAlive() override;
CHIP_ERROR AckReceive(uint16_t len) override;
CHIP_ERROR AckReceive(size_t len) override;
#if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
void TCPUserTimeoutHandler() override;
#endif // INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
Expand Down
2 changes: 1 addition & 1 deletion src/inet/TCPEndPointImplOpenThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ CHIP_ERROR TCPEndPointImplOT::DisableKeepAlive()
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}
CHIP_ERROR TCPEndPointImplOT::AckReceive(uint16_t len)
CHIP_ERROR TCPEndPointImplOT::AckReceive(size_t len)
{
return CHIP_ERROR_NOT_IMPLEMENTED;
}
Expand Down
2 changes: 1 addition & 1 deletion src/inet/TCPEndPointImplOpenThread.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class TCPEndPointImplOT : public TCPEndPoint, public EndPointStateOpenThread
CHIP_ERROR EnableNoDelay() override;
CHIP_ERROR EnableKeepAlive(uint16_t interval, uint16_t timeoutCount) override;
CHIP_ERROR DisableKeepAlive() override;
CHIP_ERROR AckReceive(uint16_t len) override;
CHIP_ERROR AckReceive(size_t len) override;
#if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
void TCPUserTimeoutHandler() override;
#endif // INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
Expand Down
10 changes: 5 additions & 5 deletions src/inet/TCPEndPointImplSockets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ CHIP_ERROR TCPEndPointImplSockets::DisableKeepAlive()
return CHIP_NO_ERROR;
}

CHIP_ERROR TCPEndPointImplSockets::AckReceive(uint16_t len)
CHIP_ERROR TCPEndPointImplSockets::AckReceive(size_t len)
{
VerifyOrReturnError(IsConnected(), CHIP_ERROR_INCORRECT_STATE);

Expand Down Expand Up @@ -483,7 +483,7 @@ CHIP_ERROR TCPEndPointImplSockets::DriveSendingImpl()

while (!mSendQueue.IsNull())
{
uint16_t bufLen = mSendQueue->DataLength();
uint32_t bufLen = static_cast<uint32_t>(mSendQueue->DataLength());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does bufLen here need to be uint32_t? send takes a size_t.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't necessarily need to be, I guess. But, IIRC, I might have gotten a CI error(or warning being flagged as error) on some platform when casting ssize_t to size_t for lenSentRaw and then comparing with bufLen. So, this seemed like a practical workaround.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What was the error? There shouldn't be an error in that case...


ssize_t lenSentRaw = send(mSocket, mSendQueue->Start(), bufLen, sendFlags);

Expand All @@ -496,14 +496,14 @@ CHIP_ERROR TCPEndPointImplSockets::DriveSendingImpl()
break;
}

if (lenSentRaw < 0 || lenSentRaw > bufLen)
if (lenSentRaw < 0 || bufLen < static_cast<uint32_t>(lenSentRaw))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this should cast to size_t, I would think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue was, IIRC, that some platforms were complaining when casting from ssize_t to size_t, which was probably the reason I cast bufLen to uint32_t as well. lenSentRaw is ssize_t and directly casting to size_t. I might have to have an exclusive check for lenSentRaw to be positive when casting to size_t. Will check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how casting to size_t can possibly complain but casting to uint32_t succeed. But I'd love to learn more.

{
err = CHIP_ERROR_INCORRECT_STATE;
break;
}

// Cast is safe because bufLen is uint16_t.
uint16_t lenSent = static_cast<uint16_t>(lenSentRaw);
// Cast is safe because bufLen is uint32_t.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but the cast to set bufLen initially was unsafe, no?

Why can't lenSent just be size_t?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the reason as above.

uint32_t lenSent = static_cast<uint32_t>(lenSentRaw);

// Mark the connection as being active.
MarkActive();
Expand Down
4 changes: 2 additions & 2 deletions src/inet/TCPEndPointImplSockets.h
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class TCPEndPointImplSockets : public TCPEndPoint, public EndPointStateSockets
CHIP_ERROR EnableNoDelay() override;
CHIP_ERROR EnableKeepAlive(uint16_t interval, uint16_t timeoutCount) override;
CHIP_ERROR DisableKeepAlive() override;
CHIP_ERROR AckReceive(uint16_t len) override;
CHIP_ERROR AckReceive(size_t len) override;
#if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
void TCPUserTimeoutHandler() override;
#endif // INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
Expand All @@ -72,7 +72,7 @@ class TCPEndPointImplSockets : public TCPEndPoint, public EndPointStateSockets

#if INET_CONFIG_OVERRIDE_SYSTEM_TCP_USER_TIMEOUT
/// This counts the number of bytes written on the TCP socket since thelast probe into the TCP outqueue was made.
uint32_t mBytesWrittenSinceLastProbe;
size_t mBytesWrittenSinceLastProbe;

/// This is the measured size(in bytes) of the kernel TCP send queue at the end of the last user timeout window.
uint32_t mLastTCPKernelSendQueueLen;
Expand Down
2 changes: 1 addition & 1 deletion src/inet/UDPEndPointImplLwIP.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -352,7 +352,7 @@ void UDPEndPointImplLwIP::LwIPReceiveUDPMessage(void * arg, struct udp_pcb * pcb
if (buf->HasChainedBuffer())
{
// Have to allocate a new big-enough buffer and copy.
uint16_t messageSize = buf->TotalLength();
size_t messageSize = buf->TotalLength();
System::PacketBufferHandle copy = System::PacketBufferHandle::New(messageSize, 0);
if (copy.IsNull() || buf->Read(copy->Start(), messageSize) != CHIP_NO_ERROR)
{
Expand Down
2 changes: 1 addition & 1 deletion src/inet/UDPEndPointImplOpenThread.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ CHIP_ERROR UDPEndPointImplOT::SendMsgImpl(const IPPacketInfo * aPktInfo, System:
message = otUdpNewMessage(mOTInstance, NULL);
VerifyOrExit(message != NULL, error = OT_ERROR_NO_BUFS);

error = otMessageAppend(message, msg->Start(), msg->DataLength());
error = otMessageAppend(message, msg->Start(), static_cast<uint16_t>(msg->DataLength()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be erroring out of the DataLength is too big?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary. Threa, BLE and LwIP cap the message max length to UINT16_MAX. So, casting to uint16_t should be safe.

Copy link
Contributor

@bzbarsky-apple bzbarsky-apple Mar 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point is this: we are going to throw away the high bytes of the length. If those are nonzero, we should error out, not silently use a different length.


if (error == OT_ERROR_NONE)
{
Expand Down
7 changes: 5 additions & 2 deletions src/inet/UDPEndPointImplSockets.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,10 @@ CHIP_ERROR UDPEndPointImplSockets::SendMsgImpl(const IPPacketInfo * aPktInfo, Sy
{
return CHIP_ERROR_POSIX(errno);
}
if (lenSent != msg->DataLength())

size_t len = static_cast<size_t>(lenSent);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we only checking for -1 on send but for < 0 on receive? Pre-existing, but please do a followup to make them consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should, ideally be checking with -1. Will take a look.


if (len != msg->DataLength())
{
return CHIP_ERROR_OUTBOUND_MESSAGE_TOO_BIG;
}
Expand Down Expand Up @@ -612,7 +615,7 @@ void UDPEndPointImplSockets::HandlePendingIO(System::SocketEvents events)
{
lStatus = CHIP_ERROR_POSIX(errno);
}
else if (rcvLen > lBuffer->AvailableDataLength())
else if (lBuffer->AvailableDataLength() < static_cast<size_t>(rcvLen))
{
lStatus = CHIP_ERROR_INBOUND_MESSAGE_TOO_BIG;
}
Expand Down
2 changes: 1 addition & 1 deletion src/inet/tests/TestInetLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ static void HandleTCPConnectionClosed(TCPEndPoint * aEndPoint, CHIP_ERROR aError
}
}

static void HandleTCPDataSent(TCPEndPoint * aEndPoint, uint16_t len) {}
static void HandleTCPDataSent(TCPEndPoint * aEndPoint, size_t len) {}

static CHIP_ERROR HandleTCPDataReceived(TCPEndPoint * aEndPoint, PacketBufferHandle && aBuffer)
{
Expand Down
Loading
Loading