Update buffer allocation strategy and maximum buffer size when using TCP as transport #30404
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
|
PR #30404: Size comparison from dbd6d1d to a419234 Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
|
PR #30404: Size comparison from dbd6d1d to 0bad674 Increases above 0.2%:
Increases (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, telink)
Decreases (5 builds for cc32xx, linux, mbed)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, telink)
|
bzbarsky-apple
requested changes
Nov 13, 2023
hnnajh
requested review from
tcarmelveilleux,
bzbarsky-apple,
Damian-Nordic and
pidarped
pidarped
reviewed
Dec 2, 2023
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
carol-apple,
cecille,
chrisdecenzo and
chshu
pullapprove
bot
requested review from
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang,
yufengwangca and
yunhanw-google
pidarped
reviewed
Dec 5, 2023
| @@ -47,10 +47,11 @@ namespace SecureMessageCodec { | |||
| * @param msgBuf The message buffer that contains the unencrypted message. If | |||
| * the operation is successful, this buffer will be mutated to contain | |||
| * the encrypted message. | |||
| * @param inputMaxLength Max size for input | |||
There was a problem hiding this comment.
What is the point of this argument? Why wouldn't callers just pass msgBuf->TotalLength() for it?
Either this argument should not exist, or the documentation needs to do a much better job of explaining what this is for and how it should be used.
My temptation is towards the former: why does the crypto stuff care about this length? It doesn't.
There was a problem hiding this comment.
so this argument is meant to provide the method the maximum length of payload's length
that means we can't just pass msgBuf->TotalLength() because that would bypass the check :) , i think this place is okay since that check is common for all the places that's calling SecureMessageCodec::Encrypt(
|
PR #30404: Size comparison from 5fd93c4 to f0609dc Increases (71 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, nrfconnect, psoc6, qpg, stm32, telink)
Decreases (3 builds for bl702l, linux)
Full report (73 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, qpg, stm32, telink)
|
pidarped
reviewed
Jan 22, 2024
| @@ -237,7 +238,17 @@ CHIP_ERROR SessionManager::PrepareMessage(const SessionHandle & sessionHandle, P | |||
| NodeId sourceNodeId = session->GetLocalScopedNodeId().GetNodeId(); | |||
| CryptoContext::BuildNonce(nonce, packetHeader.GetSecurityFlags(), messageCounter, sourceNodeId); | |||
|
|
|||
| ReturnErrorOnFailure(SecureMessageCodec::Encrypt(session->GetCryptoContext(), nonce, payloadHeader, packetHeader, message)); | |||
| if (session->GetPeerAddress().GetTransportType() == Transport::Type::kTcp) | |||
There was a problem hiding this comment.
Instead of passing the max size into Encrypt() by having this logic here, a better approach might be to have a utility function hanging off the Session that returns this. It could be in the Session object or could also be in the SessionManager, potentially. That utility can be called from inside Encrypt(and elsewhere) for checks.
pidarped
reviewed
Jan 22, 2024
| @@ -71,6 +71,8 @@ static_assert(detail::kMaxApplicationPayloadAndMICSizeBytes > kMaxTagLen, "Need | |||
| // tag we will not have source/destination node IDs, but above we are including | |||
| // those in the header sizes. | |||
| static constexpr size_t kMaxAppMessageLen = detail::kMaxApplicationPayloadAndMICSizeBytes - kMaxTagLen; | |||
| // large payload limit | |||
| static constexpr size_t kLargePayloadMaxMessageSizeBytes = 128000; | |||
There was a problem hiding this comment.
Have added a ''CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES 1000000" in a TCPConfig.h file as part of #30339
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
|
Old PR with changes requested and merge conflicts. Assuming stale and closing. |
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.
Depending on the transport choice, the node would have to be able to allocate buffers appropriately. When TCP is supported, buffer allocation would be done from the heap. That choice is made at compile time. However, the max size of the heap-allocated buffer would differ based on whether the node is using MRP or TCP underneath, at run time. We would need a dynamic way to choose that max buffer size allocation so that a larger allocation can be made from the heap when using TCP.
Fixes #29698