Address comments: SessionHolder::Grab/GrabPairing return false on fai…

…lure
project-chip · May 19, 2022 · 0b0c43f · 0b0c43f
1 parent f5d7ada
commit 0b0c43f
Show file tree

Hide file tree

Showing 8 changed files with 47 additions and 27 deletions.
diff --git a/src/app/OperationalDeviceProxy.cpp b/src/app/OperationalDeviceProxy.cpp
@@ -70,16 +70,16 @@ bool OperationalDeviceProxy::AttachToExistingSecureSession()
     ScopedNodeId peerNodeId(mPeerId.GetNodeId(), mFabricInfo->GetFabricIndex());
     auto sessionHandle =
         mInitParams.sessionManager->FindSecureSessionForNode(peerNodeId, MakeOptional(Transport::SecureSession::Type::kCASE));
-    if (sessionHandle.HasValue())
-    {
-        ChipLogProgress(Controller, "Found an existing secure session to [" ChipLogFormatX64 "-" ChipLogFormatX64 "]!",
-                        ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId()));
-        mDeviceAddress = sessionHandle.Value()->AsSecureSession()->GetPeerAddress();
-        mSecureSession.Grab(sessionHandle.Value());
-        return true;
-    }
+    if (!sessionHandle.HasValue())
+        return false;
 
-    return false;
+    ChipLogProgress(Controller, "Found an existing secure session to [" ChipLogFormatX64 "-" ChipLogFormatX64 "]!",
+        ChipLogValueX64(mPeerId.GetCompressedFabricId()), ChipLogValueX64(mPeerId.GetNodeId()));
+    mDeviceAddress = sessionHandle.Value()->AsSecureSession()->GetPeerAddress();
+    if (!mSecureSession.Grab(sessionHandle.Value()))
+        return false;
+
+    return true;
 }
 
 void OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected> * onConnection,
@@ -305,7 +305,9 @@ void OperationalDeviceProxy::OnSessionEstablished(const SessionHandle & session)
     VerifyOrReturn(mState != State::Uninitialized,
                    ChipLogError(Controller, "HandleCASEConnected was called while the device was not initialized"));
 
-    mSecureSession.Grab(session);
+    if (!mSecureSession.Grab(session))
+        return; // Got an invalid session, do not change any state
+
     MoveToState(State::SecureConnected);
     DequeueConnectionCallbacks(CHIP_NO_ERROR);
 

diff --git a/src/controller/CommissioneeDeviceProxy.cpp b/src/controller/CommissioneeDeviceProxy.cpp
@@ -97,8 +97,13 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres
 CHIP_ERROR CommissioneeDeviceProxy::SetConnected(const SessionHandle & session)
 {
     VerifyOrReturnError(mState == ConnectionState::Connecting, CHIP_ERROR_INCORRECT_STATE);
+    if (!mSecureSession.Grab(session))
+    {
+        mState = ConnectionState::NotConnected;
+        return CHIP_ERROR_INTERNAL;
+    }
+
     mState = ConnectionState::SecureConnected;
-    mSecureSession.Grab(session);
     return CHIP_NO_ERROR;
 }
 

diff --git a/src/protocols/secure_channel/PairingSession.cpp b/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);
-    mSecureSessionHolder.GrabPairing(handle.Value());
+    VerifyOrReturnError(mSecureSessionHolder.GrabPairing(handle.Value()), CHIP_ERROR_INTERNAL);
     mSessionManager = &sessionManager;
     return CHIP_NO_ERROR;
 }

diff --git a/src/transport/GroupSession.h b/src/transport/GroupSession.h
@@ -41,6 +41,8 @@ class IncomingGroupSession : public Session, public ReferenceCounted<IncomingGro
     void Retain() override { ReferenceCounted<IncomingGroupSession, NoopDeletor<IncomingGroupSession>, 0>::Retain(); }
     void Release() override { ReferenceCounted<IncomingGroupSession, NoopDeletor<IncomingGroupSession>, 0>::Release(); }
 
+    bool IsActiveSession() const override { return true; }
+
     Session::SessionType GetSessionType() const override { return Session::SessionType::kGroupIncoming; }
 #if CHIP_PROGRESS_LOGGING
     const char * GetSessionTypeString() const override { return "incoming group"; };
@@ -95,6 +97,8 @@ class OutgoingGroupSession : public Session, public ReferenceCounted<OutgoingGro
     void Retain() override { ReferenceCounted<OutgoingGroupSession, NoopDeletor<OutgoingGroupSession>, 0>::Retain(); }
     void Release() override { ReferenceCounted<OutgoingGroupSession, NoopDeletor<OutgoingGroupSession>, 0>::Release(); }
 
+    bool IsActiveSession() const override { return true; }
+
     Session::SessionType GetSessionType() const override { return Session::SessionType::kGroupOutgoing; }
 #if CHIP_PROGRESS_LOGGING
     const char * GetSessionTypeString() const override { return "outgoing group"; };

diff --git a/src/transport/Session.h b/src/transport/Session.h
@@ -68,7 +68,7 @@ class Session
     virtual void Retain()  = 0;
     virtual void Release() = 0;
 
-    virtual bool IsActiveSession() const { return true; }
+    virtual bool IsActiveSession() const = 0;
 
     virtual ScopedNodeId GetPeer() const                               = 0;
     virtual ScopedNodeId GetLocalScopedNodeId() const                  = 0;

diff --git a/src/transport/SessionHolder.cpp b/src/transport/SessionHolder.cpp
@@ -74,24 +74,31 @@ SessionHolder & SessionHolder::operator=(SessionHolder && that)
     return *this;
 }
 
-void SessionHolder::GrabPairing(const SessionHandle & session)
+bool SessionHolder::GrabPairing(const SessionHandle & session)
 {
     Release();
-    if (session->IsSecureSession() && session->AsSecureSession()->IsPairing())
-    {
-        mSession.Emplace(session.mSession);
-        session->AddHolder(*this);
-    }
+
+    if (!session->IsSecureSession())
+        return false;
+
+    if (!session->AsSecureSession()->IsPairing())
+        return false;
+
+    mSession.Emplace(session.mSession);
+    session->AddHolder(*this);
+    return true;
 }
 
-void SessionHolder::Grab(const SessionHandle & session)
+bool SessionHolder::Grab(const SessionHandle & session)
 {
     Release();
-    if (session->IsActiveSession())
-    {
-        mSession.Emplace(session.mSession);
-        session->AddHolder(*this);
-    }
+
+    if (!session->IsActiveSession())
+        return false;
+
+    mSession.Emplace(session.mSession);
+    session->AddHolder(*this);
+    return true;
 }
 
 void SessionHolder::Release()

diff --git a/src/transport/SessionHolder.h b/src/transport/SessionHolder.h
@@ -47,8 +47,8 @@ class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase
         return mSession.HasValue() && &mSession.Value().Get() == &session.mSession.Get();
     }
 
-    void GrabPairing(const SessionHandle & session); // Should be only used inside CASE/PASE pairing.
-    void Grab(const SessionHandle & session);
+    bool GrabPairing(const SessionHandle & session); // Should be only used inside CASE/PASE pairing.
+    bool Grab(const SessionHandle & session);
     void Release();
 
     operator bool() const { return mSession.HasValue(); }

diff --git a/src/transport/UnauthenticatedSessionTable.h b/src/transport/UnauthenticatedSessionTable.h
@@ -73,6 +73,8 @@ class UnauthenticatedSession : public Session,
     void Retain() override { ReferenceCounted<UnauthenticatedSession, NoopDeletor<UnauthenticatedSession>, 0>::Retain(); }
     void Release() override { ReferenceCounted<UnauthenticatedSession, NoopDeletor<UnauthenticatedSession>, 0>::Release(); }
 
+    bool IsActiveSession() const override { return true; }
+
     ScopedNodeId GetPeer() const override { return ScopedNodeId(GetPeerNodeId(), kUndefinedFabricIndex); }
     ScopedNodeId GetLocalScopedNodeId() const override { return ScopedNodeId(kUndefinedNodeId, kUndefinedFabricIndex); }