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.

Sign up for GitHub

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

Update buffer allocation strategy and maximum buffer size when using TCP as transport #30404

Closed
wants to merge 13 commits into from
Closed

Update buffer allocation strategy and maximum buffer size when using TCP as transport #30404

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a419234
Update buffer allocation strategy and maximum buffer size when using …
hnnajh Nov 10, 2023
0bad674
Adding TCP server and client config flag
hnnajh Nov 10, 2023
72421bd
Fix the size limits to be per session instead of one for all sessions
hnnajh Nov 29, 2023
976cb70
remove TCP flags
hnnajh Nov 29, 2023
94d1d8c
address comments
hnnajh Dec 4, 2023
200dea3
Restyled by clang-format
restyled-commits Dec 4, 2023
84adb59
Fix unit tests
hnnajh Dec 5, 2023
15f187a
Restyled by clang-format
restyled-commits Dec 5, 2023
efd7902
fix group messaging
hnnajh Dec 6, 2023
ae12f9a
Merge pull request #181 from project-chip/master
hnnajh Jan 19, 2024
1c2c52c
Restyled by clang-format
restyled-commits Jan 19, 2024
53ec6de
Merge branch 'tcp_heapallocation' of https://github.com/hnnajh/connec…
hnnajh Jan 20, 2024
f0609dc
Merge branch 'master' into tcp_heapallocation
hnnajh Jan 20, 2024
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
4 changes: 2 additions & 2 deletions src/transport/SecureMessageCodec.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ using System::PacketBufferHandle;
namespace SecureMessageCodec {

CHIP_ERROR Encrypt(const CryptoContext & context, CryptoContext::ConstNonceView nonce, PayloadHeader & payloadHeader,
PacketHeader & packetHeader, System::PacketBufferHandle & msgBuf)
PacketHeader & packetHeader, System::PacketBufferHandle & msgBuf, size_t inputMaxLength)
{
VerifyOrReturnError(!msgBuf.IsNull(), CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(!msgBuf->HasChainedBuffer(), CHIP_ERROR_INVALID_MESSAGE_LENGTH);
VerifyOrReturnError(msgBuf->TotalLength() <= kMaxAppMessageLen, CHIP_ERROR_MESSAGE_TOO_LONG);
VerifyOrReturnError(msgBuf->TotalLength() <= inputMaxLength, CHIP_ERROR_MESSAGE_TOO_LONG);

static_assert(std::is_same<decltype(msgBuf->TotalLength()), uint16_t>::value,
"Addition to generate payloadLength might overflow");
Expand Down
3 changes: 2 additions & 1 deletion src/transport/SecureMessageCodec.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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(

* @return A CHIP_ERROR value consistent with the result of the encryption operation
*/
CHIP_ERROR Encrypt(const CryptoContext & context, CryptoContext::ConstNonceView nonce, PayloadHeader & payloadHeader,
PacketHeader & packetHeader, System::PacketBufferHandle & msgBuf);
PacketHeader & packetHeader, System::PacketBufferHandle & msgBuf, size_t inputMaxLength = kMaxAppMessageLen);

/**
* @brief
Expand Down
15 changes: 13 additions & 2 deletions src/transport/SessionManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,8 @@ CHIP_ERROR SessionManager::PrepareMessage(const SessionHandle & sessionHandle, P
packetHeader.SetSessionId(keyContext->GetKeyHash());
CryptoContext::NonceStorage nonce;
CryptoContext::BuildNonce(nonce, packetHeader.GetSecurityFlags(), packetHeader.GetMessageCounter(), sourceNodeId);
CHIP_ERROR err = SecureMessageCodec::Encrypt(CryptoContext(keyContext), nonce, payloadHeader, packetHeader, message);
CHIP_ERROR err =
SecureMessageCodec::Encrypt(CryptoContext(keyContext), nonce, payloadHeader, packetHeader, message, kMaxAppMessageLen);
keyContext->Release();
ReturnErrorOnFailure(err);

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
// support large payloads
ReturnErrorOnFailure(SecureMessageCodec::Encrypt(session->GetCryptoContext(), nonce, payloadHeader, packetHeader,
message, kLargePayloadMaxMessageSizeBytes));
}
else
{
ReturnErrorOnFailure(SecureMessageCodec::Encrypt(session->GetCryptoContext(), nonce, payloadHeader, packetHeader,
message, kMaxAppMessageLen));
}

#if CHIP_PROGRESS_LOGGING
destination = session->GetPeerNodeId();
Expand Down
2 changes: 2 additions & 0 deletions src/transport/raw/MessageHeader.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Have added a ''CHIP_CONFIG_MAX_TCP_PAYLOAD_SIZE_BYTES 1000000" in a TCPConfig.h file as part of #30339


static constexpr uint16_t kMsgUnicastSessionIdUnsecured = 0x0000;

Expand Down