Skip to content

Commit d0538c5

Browse files
pidarpedgmarcosb
andauthored
[Cherry-pick] Shot in the dark at fixing TCP memory leak in project-chip#42006 (project-chip#42672) (project-chip#42805)
* Shot in the dark at fixing TCP memory leak in project-chip#42006 (project-chip#42672) * Shot in the dark at fixing memory leak in project-chip#42006 Test unfortunately does not repro the leak, but seems like a good test to have regardless Test doesn't repro because it seems unauthenticated session is immediately discarded * Apply review suggestions * Apply review suggestions * Fix fake tests & add additional attempt to cleanup the leak by trying to notify references of closed connection * Disable TestTCPConnection.cpp in v1.5 because of Testing namespace changes. --------- Co-authored-by: Marcos B <15697303+gmarcosb@users.noreply.github.com>
1 parent 5612d36 commit d0538c5

5 files changed

Lines changed: 263 additions & 2 deletions

File tree

src/transport/SessionManager.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -711,6 +711,7 @@ void SessionManager::HandleConnectionClosed(Transport::ActiveTCPConnectionState
711711
appTCPConnCbCtxt->connClosedCb(conn, conErr);
712712
}
713713
MarkSecureSessionOverTCPForEviction(conn, conErr);
714+
mUnauthenticatedSessions.MarkSessionOverTCPForEviction(conn);
714715
}
715716

716717
CHIP_ERROR SessionManager::TCPConnect(const PeerAddress & peerAddress, Transport::AppTCPConnectionCallbackCtxt * appState,

src/transport/UnauthenticatedSessionTable.h

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,6 +294,25 @@ class UnauthenticatedSessionTable
294294
return Optional<SessionHandle>::Missing();
295295
}
296296

297+
#if INET_CONFIG_ENABLE_TCP_ENDPOINT
298+
void MarkSessionOverTCPForEviction(Transport::ActiveTCPConnectionState & conn)
299+
{
300+
mEntries.ForEachActiveObject([&](UnauthenticatedSession * session) {
301+
if (session->GetTCPConnection() == conn)
302+
{
303+
session->ReleaseTCPConnection();
304+
305+
// If the session has no other references, we can release it.
306+
if (session->GetReferenceCount() == 0)
307+
{
308+
mEntries.ReleaseObject(static_cast<EntryType *>(session));
309+
}
310+
}
311+
return Loop::Continue;
312+
});
313+
}
314+
#endif // INET_CONFIG_ENABLE_TCP_ENDPOINT
315+
297316
private:
298317
using EntryType = detail::UnauthenticatedSessionPoolEntry<kMaxSessionCount>;
299318
friend EntryType;
@@ -304,7 +323,6 @@ class UnauthenticatedSessionTable
304323
* @returns CHIP_NO_ERROR if new session created. May fail if maximum session count has been reached (with
305324
* CHIP_ERROR_NO_MEMORY).
306325
*/
307-
CHECK_RETURN_VALUE
308326
CHIP_ERROR AllocEntry(UnauthenticatedSession::SessionRole sessionRole, NodeId ephemeralInitiatorNodeID,
309327
const PeerAddress & peerAddress, const ReliableMessageProtocolConfig & config,
310328
UnauthenticatedSession *& entry)

src/transport/raw/TCP.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,8 @@ ActiveTCPConnectionState * TCPBase::AllocateConnection(const Inet::TCPEndPointHa
160160
char addrStr[Transport::PeerAddress::kMaxToStringSize];
161161
activeConnection->mPeerAddr.ToString(addrStr);
162162
ChipLogError(Inet, "Leaked TCP connection %p to %s.", activeConnection, addrStr);
163+
// Try to notify callbacks in the hope that they release; the connection is no good
164+
CloseConnectionInternal(*activeConnection, CHIP_ERROR_CONNECTION_CLOSED_UNEXPECTEDLY, SuppressCallback::No);
163165
}
164166
ActiveTCPConnectionHandle releaseUnclaimed(activeConnection);
165167
}

src/transport/tests/BUILD.gn

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,11 @@ chip_test_suite("tests") {
4646

4747
if (chip_device_platform != "esp32" && chip_device_platform != "nrfconnect" &&
4848
chip_device_platform != "nxp") {
49-
test_sources += [ "TestSecureSessionTable.cpp" ]
49+
test_sources += [
50+
"TestSecureSessionTable.cpp",
51+
# Disabling this test because of changes to the Testing namespace in master
52+
# "TestTCPConnection.cpp",
53+
]
5054
}
5155

5256
cflags = [ "-Wconversion" ]
@@ -58,6 +62,7 @@ chip_test_suite("tests") {
5862
"${chip_root}/src/lib/core",
5963
"${chip_root}/src/lib/core:string-builder-adapters",
6064
"${chip_root}/src/lib/support",
65+
"${chip_root}/src/lib/support:test_utils",
6166
"${chip_root}/src/lib/support:testing",
6267
"${chip_root}/src/protocols",
6368
"${chip_root}/src/transport",
Lines changed: 235 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,235 @@
1+
/*
2+
*
3+
* Copyright (c) 2025 Project CHIP Authors
4+
* All rights reserved.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
/**
20+
* @file
21+
* This file implements an E2E test for the SessionManager/Transport/UnauthenticatedSession
22+
* TCP connection.
23+
*/
24+
25+
#include <errno.h>
26+
#include <vector>
27+
28+
#include <pw_unit_test/framework.h>
29+
30+
#include <credentials/PersistentStorageOpCertStore.h>
31+
#include <crypto/DefaultSessionKeystore.h>
32+
#include <crypto/PersistentStorageOperationalKeystore.h>
33+
#include <crypto/RandUtils.h>
34+
#include <lib/core/CHIPCore.h>
35+
#include <lib/support/CHIPMem.h>
36+
#include <lib/support/CodeUtils.h>
37+
#include <lib/support/TestPersistentStorageDelegate.h>
38+
#include <lib/support/UnitTestUtils.h>
39+
#include <lib/support/tests/ExtraPwTestMacros.h>
40+
#include <platform/CHIPDeviceLayer.h>
41+
#include <protocols/secure_channel/MessageCounterManager.h>
42+
#include <system/SystemLayer.h>
43+
#include <system/SystemPacketBuffer.h>
44+
#include <transport/SessionManager.h>
45+
#include <transport/TransportMgr.h>
46+
#include <transport/raw/TCP.h>
47+
#include <transport/raw/tests/NetworkTestHelpers.h>
48+
49+
using namespace chip;
50+
using namespace chip::Inet;
51+
using namespace chip::Transport;
52+
using namespace chip::Testing;
53+
54+
namespace {
55+
56+
// Use only 2 active connections (1 outgoing, 1 incoming) to easily detect leaks (exhaustion)
57+
constexpr size_t kMaxTcpActiveConnectionCount = 2;
58+
constexpr size_t kMaxTcpPendingPackets = 4;
59+
constexpr int kMaxPortBindRetries = 100;
60+
61+
using TCPImpl = Transport::TCP<kMaxTcpActiveConnectionCount, kMaxTcpPendingPackets>;
62+
63+
uint16_t GetRandomPort()
64+
{
65+
return static_cast<uint16_t>(CHIP_PORT + chip::Crypto::GetRandU16() % 1000);
66+
}
67+
68+
CHIP_ERROR RetryPortSetup(uint16_t & outPort, std::function<CHIP_ERROR(uint16_t)> setupFn)
69+
{
70+
CHIP_ERROR err;
71+
for (int retryCount = 0; retryCount < kMaxPortBindRetries; retryCount++)
72+
{
73+
uint16_t port = GetRandomPort();
74+
75+
err = setupFn(port);
76+
if (err == CHIP_ERROR_POSIX(EADDRINUSE))
77+
{
78+
continue;
79+
}
80+
81+
outPort = (err == CHIP_NO_ERROR) ? port : 0;
82+
return err;
83+
}
84+
return err;
85+
}
86+
87+
class TestTCPConnection : public ::testing::Test, public SessionConnectionDelegate, public SessionMessageDelegate
88+
{
89+
public:
90+
static void SetUpTestSuite()
91+
{
92+
if (mIOContext == nullptr)
93+
{
94+
mIOContext = new IOContext();
95+
ASSERT_NE(mIOContext, nullptr);
96+
}
97+
EXPECT_SUCCESS(mIOContext->Init());
98+
}
99+
static void TearDownTestSuite()
100+
{
101+
if (mIOContext != nullptr)
102+
{
103+
mIOContext->Shutdown();
104+
delete mIOContext;
105+
mIOContext = nullptr;
106+
}
107+
}
108+
109+
void SetUp() override
110+
{
111+
// Ignore Init return value, we check correctness later/implicitly
112+
(void) mTransportMgrBase.Init(&mTCP);
113+
mTransportMgrBase.SetSessionManager(&mSessionManager);
114+
115+
mSessionManager.SetConnectionDelegate(this);
116+
mSessionManager.SetMessageDelegate(this);
117+
}
118+
119+
void TearDown() override
120+
{
121+
mSessionManager.Shutdown();
122+
mTransportMgrBase.Close();
123+
mTCP.Close();
124+
mFabricTable.Shutdown();
125+
mOpKeyStore.Finish();
126+
mOpCertStore.Finish();
127+
}
128+
129+
// SessionConnectionDelegate
130+
void OnTCPConnectionClosed(const Transport::ActiveTCPConnectionState & conn, const SessionHandle & session,
131+
CHIP_ERROR conErr) override
132+
{}
133+
134+
bool OnTCPConnectionAttemptComplete(Transport::ActiveTCPConnectionHandle & conn, CHIP_ERROR conErr) override { return true; }
135+
136+
// SessionMessageDelegate
137+
void OnMessageReceived(const PacketHeader & packetHeader, const PayloadHeader & payloadHeader, const SessionHandle & session,
138+
DuplicateMessage isDuplicate, System::PacketBufferHandle && msgBuf) override
139+
{}
140+
141+
protected:
142+
static IOContext * mIOContext;
143+
TCPImpl mTCP;
144+
TransportMgrBase mTransportMgrBase;
145+
SessionManager mSessionManager;
146+
secure_channel::MessageCounterManager mMessageCounterManager;
147+
chip::TestPersistentStorageDelegate mDeviceStorage;
148+
chip::Crypto::DefaultSessionKeystore mSessionKeystore;
149+
chip::PersistentStorageOperationalKeystore mOpKeyStore;
150+
chip::Credentials::PersistentStorageOpCertStore mOpCertStore;
151+
FabricTable mFabricTable;
152+
153+
CHIP_ERROR InitTCP(uint16_t & outPort)
154+
{
155+
return RetryPortSetup(outPort, [&](uint16_t port) {
156+
return mTCP.Init(Transport::TcpListenParameters(mIOContext->GetTCPEndPointManager())
157+
.SetAddressType(IPAddressType::kIPv6)
158+
.SetListenPort(port)
159+
.SetServerListenEnabled(true));
160+
});
161+
}
162+
163+
CHIP_ERROR InitSessionManager()
164+
{
165+
ReturnErrorOnFailure(mOpKeyStore.Init(&mDeviceStorage));
166+
ReturnErrorOnFailure(mOpCertStore.Init(&mDeviceStorage));
167+
168+
chip::FabricTable::InitParams initParams;
169+
initParams.storage = &mDeviceStorage;
170+
initParams.operationalKeystore = &mOpKeyStore;
171+
initParams.opCertStore = &mOpCertStore;
172+
173+
ReturnErrorOnFailure(mFabricTable.Init(initParams));
174+
175+
return mSessionManager.Init(&mIOContext->GetSystemLayer(), &mTransportMgrBase, &mMessageCounterManager, &mDeviceStorage,
176+
&mFabricTable, mSessionKeystore);
177+
}
178+
};
179+
180+
IOContext * TestTCPConnection::mIOContext = nullptr;
181+
182+
TEST_F(TestTCPConnection, TestUnauthenticatedSessionReleaseOnConnectionClose)
183+
{
184+
uint16_t port;
185+
EXPECT_SUCCESS(InitTCP(port));
186+
EXPECT_SUCCESS(InitSessionManager());
187+
188+
IPAddress addr;
189+
IPAddress::FromString("::1", addr);
190+
191+
// Connect to self (loopback)
192+
Transport::PeerAddress peerAddr = Transport::PeerAddress::TCP(addr, port);
193+
194+
// 1. Establish initial connection (Leaked Candidate)
195+
ActiveTCPConnectionHandle connHandle;
196+
EXPECT_SUCCESS(mTCP.TCPConnect(peerAddr, nullptr, connHandle));
197+
198+
mIOContext->DriveIOUntil(chip::System::Clock::Seconds16(5),
199+
[&]() { return !connHandle.IsNull() && connHandle->IsConnected(); });
200+
ASSERT_FALSE(connHandle.IsNull());
201+
ASSERT_TRUE(connHandle->IsConnected());
202+
203+
// 2. Create UnauthenticatedSession by simulating an incoming message
204+
PayloadHeader header;
205+
uint8_t payloadBuffer[64];
206+
uint16_t encodeLen;
207+
header.SetMessageType(Protocols::Id(VendorId::Common, 1221), 112).SetExchangeID(2233).SetInitiator(true);
208+
EXPECT_SUCCESS(header.Encode(payloadBuffer, &encodeLen));
209+
210+
auto msgBuf = System::PacketBufferHandle::NewWithData(payloadBuffer, encodeLen);
211+
PacketHeader packetHeader;
212+
packetHeader.SetSourceNodeId(1234).SetMessageCounter(1);
213+
packetHeader.SetSessionType(Header::SessionType::kUnicastSession); // Unauthenticated
214+
215+
EXPECT_SUCCESS(packetHeader.EncodeBeforeData(msgBuf));
216+
217+
{
218+
Transport::MessageTransportContext ctxt;
219+
ctxt.conn = connHandle;
220+
221+
mSessionManager.OnMessageReceived(peerAddr, std::move(msgBuf), &ctxt);
222+
}
223+
224+
// Close TCP connection & release local handle
225+
connHandle->ForceDisconnect();
226+
connHandle.Release();
227+
228+
// Drive IO to process TCP closure
229+
mIOContext->DriveIOUntil(chip::System::Clock::Seconds16(1), [&]() { return false; });
230+
231+
// 5. Try to connect again to make sure there's no leak
232+
EXPECT_SUCCESS(mTCP.TCPConnect(peerAddr, nullptr, connHandle));
233+
}
234+
235+
} // namespace

0 commit comments

Comments
 (0)