Secure session ref count: Address left over comment for #17599 (#19299)

src/messaging/ExchangeMgr.cpp

@@ -111,8 +111,12 @@ CHIP_ERROR ExchangeManager::Shutdown()
 
 ExchangeContext * ExchangeManager::NewContext(const SessionHandle & session, ExchangeDelegate * delegate)
 {
- // Disallow creating exchange on an inactive session
- VerifyOrReturnError(session->IsActiveSession(), nullptr);
+ if (!session->IsActiveSession())
+ {
+ // Disallow creating exchange on an inactive session
+ ChipLogError(ExchangeManager, "NewContext failed: session inactive");
+ return nullptr;
+ }
  return mContextPool.CreateObject(this, mNextExchangeId++, session, true, delegate);
 }
 

src/protocols/secure_channel/PairingSession.cpp

@@ -27,7 +27,7 @@ CHIP_ERROR PairingSession::AllocateSecureSession(SessionManager & sessionManager
 {
  auto handle = sessionManager.AllocateSession(GetSecureSessionType());
  VerifyOrReturnError(handle.HasValue(), CHIP_ERROR_NO_MEMORY);
- VerifyOrReturnError(mSecureSessionHolder.GrabPairing(handle.Value()), CHIP_ERROR_INTERNAL);
+ VerifyOrReturnError(mSecureSessionHolder.GrabPairingSession(handle.Value()), CHIP_ERROR_INTERNAL);
  mSessionManager = &sessionManager;
  return CHIP_NO_ERROR;
 }

src/transport/SecureSession.h

@@ -66,14 +66,15 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
  };
 
  // Test-only: inject a session in Active state.
+ // TODO: Tests should allocate a pending session and then call Activate(), just like non-test code does.
  SecureSession(SecureSessionTable & table, Type secureSessionType, uint16_t localSessionId, NodeId localNodeId,
  NodeId peerNodeId, CATValues peerCATs, uint16_t peerSessionId, FabricIndex fabric,
  const ReliableMessageProtocolConfig & config) :
  mTable(table),
  mState(State::kActive), mSecureSessionType(secureSessionType), mLocalNodeId(localNodeId), mPeerNodeId(peerNodeId),
  mPeerCATs(peerCATs), mLocalSessionId(localSessionId), mPeerSessionId(peerSessionId), mMRPConfig(config)
  {
- Retain(); // Put the test session in Active state
+ Retain(); // Put the test session in Active state. This ref is released inside MarkForRemoval
  SetFabricIndex(fabric);
  ChipLogDetail(Inet, "SecureSession[%p]: Allocated for Test Type:%d LSID:%d", this, to_underlying(mSecureSessionType),
  mLocalSessionId);
@@ -119,7 +120,7 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
  mMRPConfig = config;
  SetFabricIndex(peerNode.GetFabricIndex());
 
- Retain();
+ Retain(); // This ref is released inside MarkForRemoval
  mState = State::kActive;
  ChipLogDetail(Inet, "SecureSession[%p]: Activated - Type:%d LSID:%d", this, to_underlying(mSecureSessionType),
  mLocalSessionId);
@@ -230,9 +231,6 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
 private:
  enum class State : uint8_t
  {
- // kPending denotes a secure session object that is internally
- // reserved by the stack before and during session establishment.
- //
  // Although the stack can tolerate eviction of these (releasing one
  // out from under the holder would exhibit as CHIP_ERROR_INCORRECT_STATE
  // during CASE or PASE), intent is that we should not and would leave
@@ -247,9 +245,9 @@ class SecureSession : public Session, public ReferenceCounted<SecureSession, Sec
  kActive = 2,
 
  // The session is pending for removal, all SessionHolders are already
- // cleared during MarkForRemoval, no future SessionHolder is able grab
- // this session, when all SessionHandles goes out of scope, the session
- // object will be released automatically.
+ // cleared during MarkForRemoval, no future SessionHolder is able to
+ // grab this session, when all SessionHandles go out of scope, the
+ // session object will be released automatically.
  kPendingRemoval = 3,
  };
 

src/transport/SessionHandle.h

@@ -28,11 +28,12 @@ class Session;
 
 /** @brief
  * Non-copyable session reference. All SessionHandles are created within SessionManager. It is not allowed to store SessionHandle
- * anywhere except for function arguments and return values.
+ *  anywhere except for function arguments and return values.
  *
- * SessionHandle is reference counted such that it never dangling, but there can be a gray period when the session is mark as
- * pending removal but not actually removed yet. During this period, the handle is functional, but the underlying session won't be
- * able to grabbed by any SessionHolder. SessionHandle->IsActiveSession can be used to check if the session is active.
+ * SessionHandle is reference counted such that it is never dangling, but there can be a gray period when the session is marked
+ * as pending removal but not actually removed yet. During this period, the handle is functional, but the underlying session
+ * won't be able to be grabbed by any SessionHolder. SessionHandle->IsActiveSession can be used to check if the session is
+ * active.
  */
 class SessionHandle
 {

src/transport/SessionHolder.cpp

@@ -74,7 +74,7 @@ SessionHolder & SessionHolder::operator=(SessionHolder && that)
  return *this;
 }
 
-bool SessionHolder::GrabPairing(const SessionHandle & session)
+bool SessionHolder::GrabPairingSession(const SessionHandle & session)
 {
  Release();
 

src/transport/SessionHolder.h

@@ -47,7 +47,7 @@ class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase
  return mSession.HasValue() && &mSession.Value().Get() == &session.mSession.Get();
  }
 
- bool GrabPairing(const SessionHandle & session); // Should be only used inside CASE/PASE pairing.
+ bool GrabPairingSession(const SessionHandle & session); // Should be only used inside CASE/PASE pairing.
  bool Grab(const SessionHandle & session);
  void Release();
 

src/transport/tests/TestSessionManager.cpp

@@ -672,7 +672,7 @@ void SessionAllocationTest(nlTestSuite * inSuite, void * inContext)
  auto handle = sessionManager.AllocateSession(Transport::SecureSession::Type::kPASE);
  NL_TEST_ASSERT(inSuite, handle.HasValue());
  SessionHolder session;
- session.GrabPairing(handle.Value());
+ session.GrabPairingSession(handle.Value());
  sessionId1 = session->AsSecureSession()->GetLocalSessionId();
  }