Adjust storage and processing in SystemPacketBuffer and associated code for large payloads. #31776
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pidarped
force-pushed
the
TCPBufferMgmt
branch
from
958af1c to
cbb1148
Compare
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
bzbarsky-apple,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
harsha-rajendran,
hawk248,
hicklin,
jepenven-silabs,
jmartinez-silabs,
jmeg-sfy,
joonhaengHeo,
jtung-apple and
kkasperczyk-no
yufengwangca
approved these changes
Feb 22, 2024
pidarped
force-pushed
the
TCPBufferMgmt
branch
from
b30bbf7 to
8597870
Compare
|
PR #31776: Size comparison from 9aeff06 to 8597870 Increases (1 build for stm32)
|
pidarped
force-pushed
the
TCPBufferMgmt
branch
from
8597870 to
68f4090
Compare
|
PR #31776: Size comparison from 9aeff06 to 68f4090 Increases (40 builds for cc13x4_26x4, cc32xx, efr32, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
Decreases (70 builds for bl602, bl702, bl702l, cyw30739, efr32, esp32, k32w, linux, nrfconnect, qpg, stm32, telink)
Full report (84 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
tcarmelveilleux
approved these changes
Feb 23, 2024
pidarped
force-pushed
the
TCPBufferMgmt
branch
from
68f4090 to
b8fa50b
Compare
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
force-pushed
the
TCPBufferMgmt
branch
from
b8fa50b to
5caf8d1
Compare
pidarped
added a commit
that referenced
this pull request
Feb 23, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
that referenced
this pull request
Feb 27, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
| @@ -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()); | |||
There was a problem hiding this comment.
Why does bufLen here need to be uint32_t? send takes a size_t.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
What was the error? There shouldn't be an error in that case...
| @@ -496,14 +496,14 @@ CHIP_ERROR TCPEndPointImplSockets::DriveSendingImpl() | |||
| break; | |||
| } | |||
|
|
|||
| if (lenSentRaw < 0 || lenSentRaw > bufLen) | |||
| if (lenSentRaw < 0 || bufLen < static_cast<uint32_t>(lenSentRaw)) | |||
There was a problem hiding this comment.
And this should cast to size_t, I would think.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Yeah, but the cast to set bufLen initially was unsafe, no?
Why can't lenSent just be size_t?
There was a problem hiding this comment.
For the reason as above.
| @@ -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())); | |||
There was a problem hiding this comment.
Should this be erroring out of the DataLength is too big?
There was a problem hiding this comment.
Not necessary. Threa, BLE and LwIP cap the message max length to UINT16_MAX. So, casting to uint16_t should be safe.
There was a problem hiding this comment.
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.
| @@ -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); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It should, ideally be checking with -1. Will take a look.
| @@ -31,7 +31,7 @@ namespace System { | |||
| CHIP_ERROR TLVPacketBufferBackingStore::OnInit(chip::TLV::TLVReader & reader, const uint8_t *& bufStart, uint32_t & bufLen) | |||
| { | |||
| bufStart = mHeadBuffer->Start(); | |||
| bufLen = mHeadBuffer->DataLength(); | |||
| bufLen = static_cast<uint32_t>(mHeadBuffer->DataLength()); | |||
There was a problem hiding this comment.
Needs to be checking the cast is safe, erroring if not?
This is all going to come back and bite us sometime. Please fix all such callsites.
| "Addition to generate payloadLength might overflow"); | ||
|
|
||
| ReturnErrorOnFailure(payloadHeader.EncodeBeforeData(msgBuf)); | ||
|
|
||
| uint8_t * data = msgBuf->Start(); | ||
| uint16_t totalLen = msgBuf->TotalLength(); | ||
| uint32_t totalLen = static_cast<uint32_t>(msgBuf->TotalLength()); |
There was a problem hiding this comment.
We should be checking this is safe!
|
|
||
| MessageAuthenticationCode mac; | ||
| ReturnErrorOnFailure(context.Encrypt(data, totalLen, data, nonce, packetHeader, mac)); | ||
|
|
||
| uint16_t taglen = 0; | ||
| ReturnErrorOnFailure(mac.Encode(packetHeader, &data[totalLen], msgBuf->AvailableDataLength(), &taglen)); | ||
|
|
||
| VerifyOrReturnError(CanCastTo<uint16_t>(totalLen + taglen), CHIP_ERROR_INTERNAL); | ||
| msgBuf->SetDataLength(static_cast<uint16_t>(totalLen + taglen)); | ||
| VerifyOrReturnError(CanCastTo<size_t>(totalLen + taglen), CHIP_ERROR_INTERNAL); |
There was a problem hiding this comment.
This is no longer doing correct checks. Back when totalLen and taglen were both uint16_t, we knew we could add without overflow, since it would add as (4-byte) int.
But now that we have 32-bit things here, we have to be a lot more careful with our additions. Please fix this.
| @@ -69,7 +69,7 @@ CHIP_ERROR Decrypt(const CryptoContext & context, CryptoContext::ConstNonceView | |||
| ReturnErrorCodeIf(msg.IsNull(), CHIP_ERROR_INVALID_ARGUMENT); | |||
|
|
|||
| uint8_t * data = msg->Start(); | |||
| uint16_t len = msg->DataLength(); | |||
| uint32_t len = static_cast<uint32_t>(msg->DataLength()); | |||
There was a problem hiding this comment.
Why is the cast safe?
| { | ||
| VerifyOrReturnError(size >= EncodeSizeBytes(), CHIP_ERROR_INVALID_ARGUMENT); | ||
| VerifyOrReturnError(size >= static_cast<size_t>(EncodeSizeBytes()), CHIP_ERROR_INVALID_ARGUMENT); |
There was a problem hiding this comment.
Why is the cast needed? What type does EncodeSizeBytes() return?
pidarped
added a commit
that referenced
this pull request
Mar 11, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
that referenced
this pull request
Mar 12, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Mar 14, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Mar 27, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 16, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 16, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 17, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 18, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 25, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 25, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
that referenced
this pull request
Apr 25, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
to pidarped/connectedhomeip
that referenced
this pull request
Apr 25, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
pidarped
added a commit
that referenced
this pull request
Apr 25, 2024
and associated code for large payloads. Extend uint16_t type variables to size_t for the APIs and all applicable places.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend uint16_t type variables to uint32_t wherever applicable.
Fixes #31115