Remove peerNodeId in MCSP receive table (Fix: #6340) (#6865)

project-chip · Jun 9, 2021 · 5435593 · 5435593
1 parent 879dd67
commit 5435593
Show file tree

Hide file tree

Showing 4 changed files with 34 additions and 35 deletions.
diff --git a/src/protocols/secure_channel/MessageCounterManager.cpp b/src/protocols/secure_channel/MessageCounterManager.cpp
@@ -69,13 +69,13 @@ CHIP_ERROR MessageCounterManager::StartSync(SecureSessionHandle session, Transpo
     return CHIP_NO_ERROR;
 }
 
-CHIP_ERROR MessageCounterManager::QueueReceivedMessageAndStartSync(SecureSessionHandle session,
+CHIP_ERROR MessageCounterManager::QueueReceivedMessageAndStartSync(const PacketHeader & packetHeader, SecureSessionHandle session,
                                                                    Transport::PeerConnectionState * state,
                                                                    const Transport::PeerAddress & peerAddress,
                                                                    System::PacketBufferHandle && msgBuf)
 {
     // Queue the message to be reprocessed when sync completes.
-    ReturnErrorOnFailure(AddToReceiveTable(state->GetPeerNodeId(), peerAddress, std::move(msgBuf)));
+    ReturnErrorOnFailure(AddToReceiveTable(packetHeader, peerAddress, std::move(msgBuf)));
     ReturnErrorOnFailure(StartSync(session, state));
 
     // After the message that triggers message counter synchronization is stored, and a message counter
@@ -115,32 +115,24 @@ void MessageCounterManager::OnResponseTimeout(Messaging::ExchangeContext * excha
     exchangeContext->Close();
 }
 
-CHIP_ERROR MessageCounterManager::AddToReceiveTable(NodeId peerNodeId, const Transport::PeerAddress & peerAddress,
+CHIP_ERROR MessageCounterManager::AddToReceiveTable(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress,
                                                     System::PacketBufferHandle && msgBuf)
 {
-    bool added     = false;
-    CHIP_ERROR err = CHIP_NO_ERROR;
+    ReturnErrorOnFailure(packetHeader.EncodeBeforeData(msgBuf));
 
     for (ReceiveTableEntry & entry : mReceiveTable)
     {
-        if (entry.peerNodeId == kUndefinedNodeId)
+        if (entry.msgBuf.IsNull())
         {
-            entry.peerNodeId  = peerNodeId;
             entry.peerAddress = peerAddress;
             entry.msgBuf      = std::move(msgBuf);
-            added             = true;
 
-            break;
+            return CHIP_NO_ERROR;
         }
     }
 
-    if (!added)
-    {
-        ChipLogError(SecureChannel, "MCSP ReceiveTable Already Full");
-        err = CHIP_ERROR_NO_MEMORY;
-    }
-
-    return err;
+    ChipLogError(SecureChannel, "MCSP ReceiveTable Already Full");
+    return CHIP_ERROR_NO_MEMORY;
 }
 
 /**
@@ -158,14 +150,26 @@ void MessageCounterManager::ProcessPendingMessages(NodeId peerNodeId)
     // this table was using an application group key; that's why it was added.
     for (ReceiveTableEntry & entry : mReceiveTable)
     {
-        if (entry.peerNodeId == peerNodeId)
+        if (!entry.msgBuf.IsNull())
         {
-            // Reprocess message.
-            secureSessionMgr->OnMessageReceived(entry.peerAddress, std::move(entry.msgBuf));
-
-            // Explicitly free any buffer owned by this handle.
-            entry.msgBuf     = nullptr;
-            entry.peerNodeId = kUndefinedNodeId;
+            PacketHeader packetHeader;
+            uint16_t headerSize = 0;
+
+            if (packetHeader.Decode((entry.msgBuf)->Start(), (entry.msgBuf)->DataLength(), &headerSize) != CHIP_NO_ERROR)
+            {
+                ChipLogError(SecureChannel, "MessageCounterManager::ProcessPendingMessages: Failed to decode PacketHeader");
+                entry.msgBuf = nullptr;
+                continue;
+            }
+
+            if (packetHeader.GetSourceNodeId().HasValue() && packetHeader.GetSourceNodeId().Value() == peerNodeId)
+            {
+                // Reprocess message.
+                secureSessionMgr->OnMessageReceived(entry.peerAddress, std::move(entry.msgBuf));
+
+                // Explicitly free any buffer owned by this handle.
+                entry.msgBuf = nullptr;
+            }
         }
     }
 }

diff --git a/src/protocols/secure_channel/MessageCounterManager.h b/src/protocols/secure_channel/MessageCounterManager.h
@@ -47,8 +47,8 @@ class MessageCounterManager : public Messaging::ExchangeDelegate, public Transpo
 
     // Implement MessageCounterManagerInterface
     CHIP_ERROR StartSync(SecureSessionHandle session, Transport::PeerConnectionState * state) override;
-    CHIP_ERROR QueueReceivedMessageAndStartSync(SecureSessionHandle session, Transport::PeerConnectionState * state,
-                                                const Transport::PeerAddress & peerAddress,
+    CHIP_ERROR QueueReceivedMessageAndStartSync(const PacketHeader & packetHeader, SecureSessionHandle session,
+                                                Transport::PeerConnectionState * state, const Transport::PeerAddress & peerAddress,
                                                 System::PacketBufferHandle && msgBuf) override;
 
     /**
@@ -74,7 +74,7 @@ class MessageCounterManager : public Messaging::ExchangeDelegate, public Transpo
      *  @retval  #CHIP_ERROR_NO_MEMORY If there is no empty slot left in the table for addition.
      *  @retval  #CHIP_NO_ERROR On success.
      */
-    CHIP_ERROR AddToReceiveTable(NodeId peerNodeId, const Transport::PeerAddress & peerAddress,
+    CHIP_ERROR AddToReceiveTable(const PacketHeader & packetHeader, const Transport::PeerAddress & peerAddress,
                                  System::PacketBufferHandle && msgBuf);
 
 private:
@@ -90,10 +90,6 @@ class MessageCounterManager : public Messaging::ExchangeDelegate, public Transpo
      */
     struct ReceiveTableEntry
     {
-        ReceiveTableEntry() : peerNodeId(kUndefinedNodeId) {}
-
-        // TODO(#6340): peerNodeId may not needed if we can extract it from msgBuf
-        NodeId peerNodeId;                  /**< The peerNodeId of the message. kUndefinedNodeId if is not in use. */
         Transport::PeerAddress peerAddress; /**< The peer address for the message*/
         System::PacketBufferHandle msgBuf;  /**< A handle to the PacketBuffer object holding the message data. */
     };

diff --git a/src/transport/MessageCounterManagerInterface.h b/src/transport/MessageCounterManagerInterface.h
@@ -37,7 +37,8 @@ class MessageCounterManagerInterface
      * Called when have received a message but session message counter is not synced.  It will queue the message and start sync if
      * the sync procedure is not started yet.
      */
-    virtual CHIP_ERROR QueueReceivedMessageAndStartSync(SecureSessionHandle session, Transport::PeerConnectionState * state,
+    virtual CHIP_ERROR QueueReceivedMessageAndStartSync(const PacketHeader & packetHeader, SecureSessionHandle session,
+                                                        Transport::PeerConnectionState * state,
                                                         const Transport::PeerAddress & peerAddress,
                                                         System::PacketBufferHandle && msgBuf) = 0;
 };

diff --git a/src/transport/SecureSessionMgr.cpp b/src/transport/SecureSessionMgr.cpp
@@ -349,12 +349,10 @@ void SecureSessionMgr::SecureMessageDispatch(const PacketHeader & packetHeader,
     {
         if (!state->GetSessionMessageCounter().GetPeerMessageCounter().IsSynchronized())
         {
-            err = packetHeader.EncodeBeforeData(msg);
-            SuccessOrExit(err);
-
             // Queue and start message sync procedure
             err = mMessageCounterManager->QueueReceivedMessageAndStartSync(
-                { state->GetPeerNodeId(), state->GetPeerKeyID(), state->GetAdminId() }, state, peerAddress, std::move(msg));
+                packetHeader, { state->GetPeerNodeId(), state->GetPeerKeyID(), state->GetAdminId() }, state, peerAddress,
+                std::move(msg));
 
             if (err != CHIP_NO_ERROR)
             {