Remove raw SessionHolder::Get (#18490)

* Fix session holder.

* Review feedback

examples/shell/shell_common/cmd_ping.cpp

@@ -320,7 +320,7 @@ void StartPinging(streamer_t * stream, char * destination)
     err = EstablishSecureSession(stream, GetEchoPeerAddress());
     SuccessOrExit(err);
 
-    err = gEchoClient.Init(&gExchangeManager, gSession.Get());
+    err = gEchoClient.Init(&gExchangeManager, gSession.Get().Value());
     SuccessOrExit(err);
 
     // Arrange to get a callback whenever an Echo Response is received.

examples/shell/shell_common/cmd_send.cpp

@@ -128,7 +128,7 @@ CHIP_ERROR SendMessage(streamer_t * stream)
     uint32_t payloadSize = gSendArguments.GetPayloadSize();
 
     // Create a new exchange context.
-    auto * ec = gExchangeManager.NewContext(gSession.Get(), &gMockAppDelegate);
+    auto * ec = gExchangeManager.NewContext(gSession.Get().Value(), &gMockAppDelegate);
     VerifyOrExit(ec != nullptr, err = CHIP_ERROR_NO_MEMORY);
 
     payloadBuf = MessagePacketBuffer::New(payloadSize);

src/app/OperationalDeviceProxy.cpp

@@ -191,7 +191,7 @@ CHIP_ERROR OperationalDeviceProxy::UpdateDeviceData(const Transport::PeerAddress
             return CHIP_NO_ERROR;
         }
 
-        mSecureSession.Get()->AsSecureSession()->SetPeerAddress(addr);
+        mSecureSession.Get().Value()->AsSecureSession()->SetPeerAddress(addr);
     }
 
     return err;
@@ -305,7 +305,7 @@ CHIP_ERROR OperationalDeviceProxy::Disconnect()
     ReturnErrorCodeIf(mState != State::SecureConnected, CHIP_ERROR_INCORRECT_STATE);
     if (mSecureSession)
     {
-        mInitParams.sessionManager->ExpirePairing(mSecureSession.Get());
+        mInitParams.sessionManager->ExpirePairing(mSecureSession.Get().Value());
     }
     MoveToState(State::Initialized);
 

src/app/OperationalDeviceProxy.h

@@ -182,7 +182,7 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy,
 
     Messaging::ExchangeManager * GetExchangeManager() const override { return mInitParams.exchangeMgr; }
 
-    chip::Optional<SessionHandle> GetSecureSession() const override { return mSecureSession.ToOptional(); }
+    chip::Optional<SessionHandle> GetSecureSession() const override { return mSecureSession.Get(); }
 
     Transport::PeerAddress GetPeerAddress() const { return mDeviceAddress; }
 

src/app/ReadClient.cpp

@@ -269,7 +269,9 @@ CHIP_ERROR ReadClient::SendReadRequest(ReadPrepareParams & aReadPrepareParams)
     ReturnErrorOnFailure(request.EndOfReadRequestMessage().GetError());
     ReturnErrorOnFailure(writer.Finalize(&msgBuf));
 
-    mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get(), this);
+    VerifyOrReturnError(aReadPrepareParams.mSessionHolder, CHIP_ERROR_MISSING_SECURE_SESSION);
+
+    mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get().Value(), this);
     VerifyOrReturnError(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
 
     mpExchangeCtx->SetResponseTimeout(aReadPrepareParams.mTimeout);
@@ -913,7 +915,9 @@ CHIP_ERROR ReadClient::SendSubscribeRequest(ReadPrepareParams & aReadPreparePara
     ReturnErrorOnFailure(err = request.EndOfSubscribeRequestMessage().GetError());
     ReturnErrorOnFailure(writer.Finalize(&msgBuf));
 
-    mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get(), this);
+    VerifyOrReturnError(aReadPrepareParams.mSessionHolder, CHIP_ERROR_MISSING_SECURE_SESSION);
+
+    mpExchangeCtx = mpExchangeMgr->NewContext(aReadPrepareParams.mSessionHolder.Get().Value(), this);
     VerifyOrReturnError(mpExchangeCtx != nullptr, err = CHIP_ERROR_NO_MEMORY);
     mpExchangeCtx->SetResponseTimeout(kImMessageTimeout);
 

src/app/ReadHandler.cpp

@@ -221,7 +221,7 @@ CHIP_ERROR ReadHandler::SendStatusReport(Protocols::InteractionModel::Status aSt
     {
         VerifyOrReturnLogError(mpExchangeCtx == nullptr, CHIP_ERROR_INCORRECT_STATE);
         VerifyOrReturnLogError(mSessionHandle, CHIP_ERROR_INCORRECT_STATE);
-        mpExchangeCtx = InteractionModelEngine::GetInstance()->GetExchangeManager()->NewContext(mSessionHandle.Get(), this);
+        mpExchangeCtx = InteractionModelEngine::GetInstance()->GetExchangeManager()->NewContext(mSessionHandle.Get().Value(), this);
     }
     VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);
 
@@ -240,7 +240,7 @@ CHIP_ERROR ReadHandler::SendReportData(System::PacketBufferHandle && aPayload, b
     {
         VerifyOrReturnLogError(mpExchangeCtx == nullptr, CHIP_ERROR_INCORRECT_STATE);
         VerifyOrReturnLogError(mSessionHandle, CHIP_ERROR_INCORRECT_STATE);
-        mpExchangeCtx = InteractionModelEngine::GetInstance()->GetExchangeManager()->NewContext(mSessionHandle.Get(), this);
+        mpExchangeCtx = InteractionModelEngine::GetInstance()->GetExchangeManager()->NewContext(mSessionHandle.Get().Value(), this);
     }
 
     VerifyOrReturnLogError(mpExchangeCtx != nullptr, CHIP_ERROR_INCORRECT_STATE);

src/app/tests/integration/chip_im_initiator.cpp

@@ -266,7 +266,7 @@ CHIP_ERROR SendCommandRequest(std::unique_ptr<chip::app::CommandSender> && comma
     err = commandSender->FinishCommand();
     SuccessOrExit(err);
 
-    err = commandSender->SendCommandRequest(gSession.Get(), chip::MakeOptional(gMessageTimeout));
+    err = commandSender->SendCommandRequest(gSession.Get().Value(), chip::MakeOptional(gMessageTimeout));
     SuccessOrExit(err);
 
     gCommandCount++;
@@ -302,7 +302,7 @@ CHIP_ERROR SendBadCommandRequest(std::unique_ptr<chip::app::CommandSender> && co
     err = commandSender->FinishCommand();
     SuccessOrExit(err);
 
-    err = commandSender->SendCommandRequest(gSession.Get(), chip::MakeOptional(gMessageTimeout));
+    err = commandSender->SendCommandRequest(gSession.Get().Value(), chip::MakeOptional(gMessageTimeout));
     SuccessOrExit(err);
     gCommandCount++;
     commandSender.release();
@@ -330,7 +330,7 @@ CHIP_ERROR SendReadRequest()
 
     printf("\nSend read request message to Node: %" PRIu64 "\n", chip::kTestDeviceNodeId);
 
-    chip::app::ReadPrepareParams readPrepareParams(gSession.Get());
+    chip::app::ReadPrepareParams readPrepareParams(gSession.Get().Value());
     readPrepareParams.mTimeout                     = gMessageTimeout;
     readPrepareParams.mpAttributePathParamsList    = &attributePathParams;
     readPrepareParams.mAttributePathParamsListSize = 1;
@@ -367,7 +367,7 @@ CHIP_ERROR SendWriteRequest(chip::app::WriteClient & apWriteClient)
 
     SuccessOrExit(err = apWriteClient.EncodeAttribute(
                       chip::app::AttributePathParams(2 /* endpoint */, 3 /* cluster */, 4 /* attribute */), true));
-    SuccessOrExit(err = apWriteClient.SendWriteRequest(gSession.Get(), gMessageTimeout));
+    SuccessOrExit(err = apWriteClient.SendWriteRequest(gSession.Get().Value(), gMessageTimeout));
 
     gWriteCount++;
 
@@ -384,7 +384,7 @@ CHIP_ERROR SendSubscribeRequest()
     CHIP_ERROR err   = CHIP_NO_ERROR;
     gLastMessageTime = chip::System::SystemClock().GetMonotonicTimestamp();
 
-    chip::app::ReadPrepareParams readPrepareParams(gSession.Get());
+    chip::app::ReadPrepareParams readPrepareParams(gSession.Get().Value());
     chip::app::EventPathParams * eventPathParams           = new chip::app::EventPathParams[2];
     chip::app::AttributePathParams * attributePathParams   = new chip::app::AttributePathParams[1];
     readPrepareParams.mpEventPathParamsList                = eventPathParams;

src/controller/CommissioneeDeviceProxy.cpp

@@ -47,7 +47,8 @@ CHIP_ERROR CommissioneeDeviceProxy::SendCommands(app::CommandSender * commandObj
 {
     VerifyOrReturnError(mSecureSession, CHIP_ERROR_INCORRECT_STATE);
     VerifyOrReturnError(commandObj != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
-    return commandObj->SendCommandRequest(mSecureSession.Get(), timeout);
+    VerifyOrReturnError(mSecureSession, CHIP_ERROR_MISSING_SECURE_SESSION);
+    return commandObj->SendCommandRequest(mSecureSession.Get().Value(), timeout);
 }
 
 void CommissioneeDeviceProxy::OnSessionReleased()
@@ -60,7 +61,7 @@ CHIP_ERROR CommissioneeDeviceProxy::CloseSession()
     ReturnErrorCodeIf(mState != ConnectionState::SecureConnected, CHIP_ERROR_INCORRECT_STATE);
     if (mSecureSession)
     {
-        mSessionManager->ExpirePairing(mSecureSession.Get());
+        mSessionManager->ExpirePairing(mSecureSession.Get().Value());
     }
     mState = ConnectionState::NotConnected;
     mPairing.Clear();
@@ -87,7 +88,7 @@ CHIP_ERROR CommissioneeDeviceProxy::UpdateDeviceData(const Transport::PeerAddres
         return CHIP_NO_ERROR;
     }
 
-    Transport::SecureSession * secureSession = mSecureSession.Get()->AsSecureSession();
+    Transport::SecureSession * secureSession = mSecureSession.Get().Value()->AsSecureSession();
     secureSession->SetPeerAddress(addr);
 
     return CHIP_NO_ERROR;

src/controller/CommissioneeDeviceProxy.h

@@ -137,7 +137,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionDelegate
     CHIP_ERROR SetPeerId(ByteSpan rcac, ByteSpan noc) override;
     const Transport::PeerAddress & GetPeerAddress() const { return mDeviceAddress; }
 
-    chip::Optional<SessionHandle> GetSecureSession() const override { return mSecureSession.ToOptional(); }
+    chip::Optional<SessionHandle> GetSecureSession() const override { return mSecureSession.Get(); }
 
     Messaging::ExchangeManager * GetExchangeManager() const override { return mExchangeMgr; }
 

src/controller/python/chip/clusters/attribute.cpp

@@ -85,7 +85,7 @@ OnReportBeginCallback gOnReportEndCallback                           = nullptr;
 
 void PythonResubscribePolicy(uint32_t aNumCumulativeRetries, uint32_t & aNextSubscriptionIntervalMsec, bool & aShouldResubscribe)
 {
-    aShouldResubscribe = false;
+    aShouldResubscribe = true;
 }
 
 class ReadClientCallback : public ReadClient::Callback

src/lib/core/CHIPError.cpp

@@ -206,6 +206,9 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err)
     case CHIP_ERROR_TLV_TAG_NOT_FOUND.AsInteger():
         desc = "TLV tag not found";
         break;
+    case CHIP_ERROR_MISSING_SECURE_SESSION.AsInteger():
+        desc = "Missing secure session";
+        break;
     case CHIP_ERROR_INVALID_PATH_LIST.AsInteger():
         desc = "Invalid TLV path list";
         break;

src/lib/core/CHIPError.h

@@ -1500,7 +1500,14 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_TLV_TAG_NOT_FOUND                           CHIP_CORE_ERROR(0x76)
 
-// unused CHIP_CORE_ERROR(0x77)
+/**
+ * @def CHIP_ERROR_MISSING_SECURE_SESSION
+ *
+ * @brief
+ *
+ *  A secure session is needed to do work, but is missing/is not present.
+ */
+#define CHIP_ERROR_MISSING_SECURE_SESSION                      CHIP_CORE_ERROR(0x77)
 
 // unused CHIP_CORE_ERROR(0x78)
 
@@ -2388,10 +2395,6 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_IM_MALFORMED_STATUS_RESPONSE_MESSAGE      CHIP_CORE_ERROR(0xe1)
 
-/**
- *  @}
- */
-
 // clang-format on
 
 // !!!!! IMPORTANT !!!!!  If you add new CHIP errors, please update the translation

src/messaging/ExchangeContext.cpp

@@ -183,10 +183,24 @@ CHIP_ERROR ExchangeContext::SendMessage(Protocols::Id protocolId, uint8_t msgTyp
             return CHIP_ERROR_INTERNAL;
         }
 
+        //
+        // It is possible that we might have evicted a session as a side-effect of processing an inbound message on this exchange.
+        // We cannot proceed any further sending a message since we don't have an attached session, so let's error out.
+        //
+        // This should not happen to well-behaved logic attempting to sending messages on exchanges, so let's print out a warning
+        // to ensure it alerts someone to fixing their logic...
+        //
+        if (!mSession)
+        {
+            ChipLogError(ExchangeManager,
+                         "WARNING: We shouldn't be sending a message on an exchange that has no attached session...");
+            return CHIP_ERROR_MISSING_SECURE_SESSION;
+        }
+
         // Create a new scope for `err`, to avoid shadowing warning previous `err`.
-        CHIP_ERROR err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get(), mExchangeId, IsInitiator(),
-                                               GetReliableMessageContext(), reliableTransmissionRequested, protocolId, msgType,
-                                               std::move(msgBuf));
+        CHIP_ERROR err = mDispatch.SendMessage(GetExchangeMgr()->GetSessionManager(), mSession.Get().Value(), mExchangeId,
+                                               IsInitiator(), GetReliableMessageContext(), reliableTransmissionRequested,
+                                               protocolId, msgType, std::move(msgBuf));
         if (err != CHIP_NO_ERROR && IsResponseExpected())
         {
             CancelResponseTimer();

src/messaging/ExchangeContext.h

@@ -156,7 +156,13 @@ class DLL_EXPORT ExchangeContext : public ReliableMessageContext,
 
     ExchangeMessageDispatch & GetMessageDispatch() { return mDispatch; }
 
-    SessionHandle GetSessionHandle() const { return mSession.Get(); }
+    SessionHandle GetSessionHandle() const
+    {
+        VerifyOrDie(mSession);
+        auto sessionHandle = mSession.Get();
+        return std::move(sessionHandle.Value());
+    }
+
     bool HasSessionHandle() const { return mSession; }
 
     uint16_t GetExchangeId() const { return mExchangeId; }

src/messaging/tests/MessagingContext.cpp

@@ -107,12 +107,14 @@ CHIP_ERROR MessagingContext::CreateSessionBobToFriends()
 
 SessionHandle MessagingContext::GetSessionBobToAlice()
 {
-    return mSessionBobToAlice.Get();
+    auto sessionHandle = mSessionBobToAlice.Get();
+    return std::move(sessionHandle.Value());
 }
 
 SessionHandle MessagingContext::GetSessionAliceToBob()
 {
-    return mSessionAliceToBob.Get();
+    auto sessionHandle = mSessionAliceToBob.Get();
+    return std::move(sessionHandle.Value());
 }
 
 SessionHandle MessagingContext::GetSessionBobToFriends()
@@ -122,12 +124,18 @@ SessionHandle MessagingContext::GetSessionBobToFriends()
 
 void MessagingContext::ExpireSessionBobToAlice()
 {
-    mSessionManager.ExpirePairing(mSessionBobToAlice.Get());
+    if (mSessionBobToAlice)
+    {
+        mSessionManager.ExpirePairing(mSessionBobToAlice.Get().Value());
+    }
 }
 
 void MessagingContext::ExpireSessionAliceToBob()
 {
-    mSessionManager.ExpirePairing(mSessionAliceToBob.Get());
+    if (mSessionAliceToBob)
+    {
+        mSessionManager.ExpirePairing(mSessionAliceToBob.Get().Value());
+    }
 }
 
 void MessagingContext::ExpireSessionBobToFriends()

src/messaging/tests/echo/echo_requester.cpp

@@ -255,7 +255,7 @@ int main(int argc, char * argv[])
     err = EstablishSecureSession();
     SuccessOrExit(err);
 
-    err = gEchoClient.Init(&gExchangeManager, gSession.Get());
+    err = gEchoClient.Init(&gExchangeManager, gSession.Get().Value());
     SuccessOrExit(err);
 
     // Arrange to get a callback whenever an Echo Response is received.

src/protocols/echo/EchoClient.cpp

@@ -70,8 +70,10 @@ CHIP_ERROR EchoClient::SendEchoRequest(System::PacketBufferHandle && payload, Me
         mExchangeCtx = nullptr;
     }
 
+    VerifyOrReturnError(mSecureSession, CHIP_ERROR_INVALID_MESSAGE_TYPE);
+
     // Create a new exchange context.
-    mExchangeCtx = mExchangeMgr->NewContext(mSecureSession.Get(), this);
+    mExchangeCtx = mExchangeMgr->NewContext(mSecureSession.Get().Value(), this);
     if (mExchangeCtx == nullptr)
     {
         return CHIP_ERROR_NO_MEMORY;

src/protocols/secure_channel/PairingSession.cpp

@@ -67,7 +67,8 @@ void PairingSession::Finish()
     CHIP_ERROR err = ActivateSecureSession(address);
     if (err == CHIP_NO_ERROR)
     {
-        mDelegate->OnSessionEstablished(mSecureSessionHolder.Get());
+        VerifyOrDie(mSecureSessionHolder);
+        mDelegate->OnSessionEstablished(mSecureSessionHolder.Get().Value());
     }
     else
     {
@@ -155,14 +156,14 @@ void PairingSession::Clear()
 
     if (mSecureSessionHolder)
     {
-        SessionHandle session = mSecureSessionHolder.Get();
+        auto session = mSecureSessionHolder.Get();
         // Call Release before ExpirePairing because we don't want to receive OnSessionReleased() event here
         mSecureSessionHolder.Release();
-        if (!session->AsSecureSession()->IsActiveSession() && mSessionManager != nullptr)
+        if (!session.Value()->AsSecureSession()->IsActiveSession() && mSessionManager != nullptr)
         {
             // Make sure to clean up our pending session, since we're the only
             // ones who have access to it do do so.
-            mSessionManager->ExpirePairing(session);
+            mSessionManager->ExpirePairing(session.Value());
         }
     }
 

src/transport/SessionHolder.h

@@ -51,10 +51,16 @@ class SessionHolder : public SessionDelegate, public IntrusiveListNodeBase
     void Release();
 
     operator bool() const { return mSession.HasValue(); }
-    SessionHandle Get() const { return SessionHandle{ mSession.Value().Get() }; }
-    Optional<SessionHandle> ToOptional() const
+    Optional<SessionHandle> Get() const
     {
-        return mSession.HasValue() ? chip::MakeOptional<SessionHandle>(Get()) : chip::Optional<SessionHandle>::Missing();
+        //
+        // We cannot return mSession directly even if Optional<SessionHandle> is internally composed of the same bits,
+        // since they are not actually equivalent type-wise, and SessionHandle does not permit copy-construction.
+        //
+        // So, construct a new Optional<SessionHandle> from the underlying Transport::Session reference.
+        //
+        return mSession.HasValue() ? chip::MakeOptional<SessionHandle>(mSession.Value().Get())
+                                   : chip::Optional<SessionHandle>::Missing();
     }
 
     Transport::Session * operator->() const { return &mSession.Value().Get(); }