From f2f85d9980a1bd4ca060ad121d39e1efd65d8957 Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Thu, 21 Oct 2021 15:40:56 +0800 Subject: [PATCH 1/3] Fix UnauthenticatedSession leak --- src/transport/SessionManager.cpp | 12 ++-- src/transport/SessionManager.h | 8 +-- src/transport/UnauthenticatedSessionTable.h | 65 +++++++++++---------- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/src/transport/SessionManager.cpp b/src/transport/SessionManager.cpp index 7dc32a45271c77..ed763f0a3a3200 100644 --- a/src/transport/SessionManager.cpp +++ b/src/transport/SessionManager.cpp @@ -189,7 +189,7 @@ CHIP_ERROR SessionManager::SendPreparedMessage(SessionHandle session, const Encr else { auto unauthenticated = session.GetUnauthenticatedSession(); - mUnauthenticatedSessions.MarkSessionActive(unauthenticated.Get()); + mUnauthenticatedSessions.MarkSessionActive(unauthenticated); destination = &unauthenticated->GetPeerAddress(); ChipLogProgress(Inet, @@ -339,13 +339,14 @@ void SessionManager::OnMessageReceived(const PeerAddress & peerAddress, System:: void SessionManager::MessageDispatch(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress, System::PacketBufferHandle && msg) { - Transport::UnauthenticatedSession * session = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress); - if (session == nullptr) + Optional optionalSession = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress); + if (!optionalSession.HasValue()) { ChipLogError(Inet, "UnauthenticatedSession exhausted"); return; } + Transport::UnauthenticatedSessionHandle session = optionalSession.Value(); SessionManagerDelegate::DuplicateMessage isDuplicate = SessionManagerDelegate::DuplicateMessage::No; // Verify message counter @@ -357,7 +358,7 @@ void SessionManager::MessageDispatch(const PacketHeader & packetHeader, const Tr } VerifyOrDie(err == CHIP_NO_ERROR); - mUnauthenticatedSessions.MarkSessionActive(*session); + mUnauthenticatedSessions.MarkSessionActive(session); PayloadHeader payloadHeader; ReturnOnFailure(payloadHeader.DecodeAndConsume(msg)); @@ -374,8 +375,7 @@ void SessionManager::MessageDispatch(const PacketHeader & packetHeader, const Tr if (mCB != nullptr) { - mCB->OnMessageReceived(packetHeader, payloadHeader, SessionHandle(Transport::UnauthenticatedSessionHandle(*session)), - peerAddress, isDuplicate, std::move(msg)); + mCB->OnMessageReceived(packetHeader, payloadHeader, SessionHandle(session), peerAddress, isDuplicate, std::move(msg)); } } diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 159251a8202421..0f34c262e21a6f 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -265,11 +265,9 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate Optional CreateUnauthenticatedSession(const Transport::PeerAddress & peerAddress) { - Transport::UnauthenticatedSession * session = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress); - if (session == nullptr) - return Optional::Missing(); - - return Optional::Value(SessionHandle(Transport::UnauthenticatedSessionHandle(*session))); + Optional session = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress); + return session.HasValue() ? Optional::Value(SessionHandle(session.Value())) + : Optional::Missing(); } private: diff --git a/src/transport/UnauthenticatedSessionTable.h b/src/transport/UnauthenticatedSessionTable.h index 701b27afe1bcca..7658889ed821d2 100644 --- a/src/transport/UnauthenticatedSessionTable.h +++ b/src/transport/UnauthenticatedSessionTable.h @@ -44,7 +44,7 @@ class UnauthenticatedSessionDeleter * @brief * An UnauthenticatedSession stores the binding of TransportAddress, and message counters. */ -class UnauthenticatedSession : public ReferenceCounted +class UnauthenticatedSession : public ReferenceCounted { public: UnauthenticatedSession(const PeerAddress & address) : mPeerAddress(address) { mLocalMessageCounter.Init(); } @@ -82,6 +82,39 @@ template FindOrAllocateEntry(const PeerAddress & address) + { + UnauthenticatedSession * result = FindEntry(address); + if (result != nullptr) + return MakeOptional(*result); + + CHIP_ERROR err = AllocEntry(address, result); + if (err == CHIP_NO_ERROR) + { + return MakeOptional(*result); + } + else + { + return Optional::Missing(); + } + } + + /// Mark a session as active + void MarkSessionActive(UnauthenticatedSessionHandle session) + { + session->SetLastActivityTimeMs(mTimeSource.GetCurrentMonotonicTimeMs()); + } + + /// Allows access to the underlying time source used for keeping track of connection active time + Time::TimeSource & GetTimeSource() { return mTimeSource; } + +private: /** * Allocates a new session out of the internal resource pool. * @@ -125,36 +158,6 @@ class UnauthenticatedSessionTable return result; } - /** - * Get a peer given the peer id. If the peer doesn't exist in the cache, allocate a new entry for it. - * - * @return the peer found or allocated, nullptr if not found and allocate failed. - */ - CHECK_RETURN_VALUE - UnauthenticatedSession * FindOrAllocateEntry(const PeerAddress & address) - { - UnauthenticatedSession * result = FindEntry(address); - if (result != nullptr) - return result; - - CHIP_ERROR err = AllocEntry(address, result); - if (err == CHIP_NO_ERROR) - { - return result; - } - else - { - return nullptr; - } - } - - /// Mark a session as active - void MarkSessionActive(UnauthenticatedSession & entry) { entry.SetLastActivityTimeMs(mTimeSource.GetCurrentMonotonicTimeMs()); } - - /// Allows access to the underlying time source used for keeping track of connection active time - Time::TimeSource & GetTimeSource() { return mTimeSource; } - -private: UnauthenticatedSession * FindLeastRecentUsedEntry() { UnauthenticatedSession * result = nullptr; From 5c6596210024b9182a207128aeceaf583c531248 Mon Sep 17 00:00:00 2001 From: Zang MingJie Date: Fri, 22 Oct 2021 18:50:08 +0800 Subject: [PATCH 2/3] Resovle comments --- src/lib/core/Optional.h | 10 ++++++++++ src/transport/SessionManager.h | 3 +-- src/transport/UnauthenticatedSessionTable.h | 4 ++-- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/lib/core/Optional.h b/src/lib/core/Optional.h index 6e14e00f506bc5..6f76e53ced5475 100644 --- a/src/lib/core/Optional.h +++ b/src/lib/core/Optional.h @@ -29,6 +29,14 @@ namespace chip { + +/// An empty class type used to indicate optional type with uninitialized state. +struct NullOptionalType +{ + explicit NullOptionalType() = default; +}; +constexpr NullOptionalType NullOptional{}; + /** * Pairs an object with a boolean value to determine if the object value * is actually valid or not. @@ -38,6 +46,8 @@ class Optional { public: constexpr Optional() : mHasValue(false) {} + constexpr Optional(NullOptionalType) : mHasValue(false) {}; + ~Optional() { if (mHasValue) diff --git a/src/transport/SessionManager.h b/src/transport/SessionManager.h index 0f34c262e21a6f..43f6e0adbf1a12 100644 --- a/src/transport/SessionManager.h +++ b/src/transport/SessionManager.h @@ -266,8 +266,7 @@ class DLL_EXPORT SessionManager : public TransportMgrDelegate Optional CreateUnauthenticatedSession(const Transport::PeerAddress & peerAddress) { Optional session = mUnauthenticatedSessions.FindOrAllocateEntry(peerAddress); - return session.HasValue() ? Optional::Value(SessionHandle(session.Value())) - : Optional::Missing(); + return session.HasValue() ? MakeOptional(session.Value()) : NullOptional; } private: diff --git a/src/transport/UnauthenticatedSessionTable.h b/src/transport/UnauthenticatedSessionTable.h index 7658889ed821d2..70b46f26bf56b2 100644 --- a/src/transport/UnauthenticatedSessionTable.h +++ b/src/transport/UnauthenticatedSessionTable.h @@ -83,9 +83,9 @@ class UnauthenticatedSessionTable { public: /** - * Get a peer given the peer id. If the peer doesn't exist in the cache, allocate a new entry for it. + * Get a session given the peer address. If the session doesn't exist in the cache, allocate a new entry for it. * - * @return the peer found or allocated, nullptr if not found and allocate failed. + * @return the session found or allocated, nullptr if not found and allocation failed. */ CHECK_RETURN_VALUE Optional FindOrAllocateEntry(const PeerAddress & address) From 5927048b754fcc2fc21f590238b3d47eb23bfca0 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 22 Oct 2021 10:50:37 +0000 Subject: [PATCH 3/3] Restyled by clang-format --- src/lib/core/Optional.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/lib/core/Optional.h b/src/lib/core/Optional.h index 6f76e53ced5475..b3861a9ea43052 100644 --- a/src/lib/core/Optional.h +++ b/src/lib/core/Optional.h @@ -29,7 +29,6 @@ namespace chip { - /// An empty class type used to indicate optional type with uninitialized state. struct NullOptionalType { @@ -46,7 +45,7 @@ class Optional { public: constexpr Optional() : mHasValue(false) {} - constexpr Optional(NullOptionalType) : mHasValue(false) {}; + constexpr Optional(NullOptionalType) : mHasValue(false){}; ~Optional() {