From 99e0666082199c19a3862c0e5b7547639458d473 Mon Sep 17 00:00:00 2001
From: Simon Lin <simonlin@google.com>
Date: Thu, 21 May 2020 11:22:15 +0800
Subject: [PATCH] [mle] fix MLE Router handling errors (#4860)

---
 src/core/thread/mle.cpp          |  6 ++++++
 src/core/thread/mle.hpp          |  2 +-
 src/core/thread/mle_router.cpp   | 20 +++++++++++++++++++-
 src/core/thread/router_table.hpp | 16 ++++++++++++++++
 4 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/src/core/thread/mle.cpp b/src/core/thread/mle.cpp
index fd3c40ec8..3176a297f 100644
--- a/src/core/thread/mle.cpp
+++ b/src/core/thread/mle.cpp
@@ -3002,6 +3002,12 @@ otError Mle::HandleLeaderData(const Message &aMessage, const Ip6::MessageInfo &a
     {
         if (IsChild())
         {
+#if OPENTHREAD_FTD
+            // An FTD skips handling LeaderData of a different partition.
+            VerifyOrExit(!IsFullThreadDevice() || (leaderData.GetPartitionId() == mLeaderData.GetPartitionId() &&
+                                                   leaderData.GetLeaderRouterId() == GetLeaderId()),
+                         error = OT_ERROR_DROP);
+#endif
             SetLeaderData(leaderData.GetPartitionId(), leaderData.GetWeighting(), leaderData.GetLeaderRouterId());
             mRetrieveNewNetworkData = true;
         }
diff --git a/src/core/thread/mle.hpp b/src/core/thread/mle.hpp
index f364e1357..10ee1fafe 100644
--- a/src/core/thread/mle.hpp
+++ b/src/core/thread/mle.hpp
@@ -1636,6 +1636,7 @@ protected:
     bool          mRetrieveNewNetworkData;   ///< Indicating new Network Data is needed if set.
     DeviceRole    mRole;                     ///< Current Thread role.
     Router        mParent;                   ///< Parent information.
+    Router        mParentCandidate;          ///< Parent candidate information.
     DeviceMode    mDeviceMode;               ///< Device mode setting.
     AttachState   mAttachState;              ///< The parent request state.
     ReattachState mReattachState;            ///< Reattach state
@@ -1781,7 +1782,6 @@ private:
     bool       mReceivedResponseFromParent;
     LeaderData mParentLeaderData;
 
-    Router    mParentCandidate;
     Challenge mParentCandidateChallenge;
 
     Ip6::UdpSocket mSocket;
diff --git a/src/core/thread/mle_router.cpp b/src/core/thread/mle_router.cpp
index a8783759e..cba6f445a 100644
--- a/src/core/thread/mle_router.cpp
+++ b/src/core/thread/mle_router.cpp
@@ -1166,7 +1166,7 @@ otError MleRouter::HandleAdvertisement(const Message &         aMessage,
     const otThreadLinkInfo *linkInfo = static_cast<const otThreadLinkInfo *>(aMessageInfo.GetLinkInfo());
     uint8_t linkMargin = LinkQualityInfo::ConvertRssToLinkMargin(Get<Mac::Mac>().GetNoiseFloor(), linkInfo->mRss);
     Mac::ExtAddress macAddr;
-    uint16_t        sourceAddress;
+    uint16_t        sourceAddress = Mac::kShortAddrInvalid;
     LeaderData      leaderData;
     RouteTlv        route;
     uint32_t        partitionId;
@@ -1284,6 +1284,10 @@ otError MleRouter::HandleAdvertisement(const Message &         aMessage,
         if (processRouteTlv)
         {
             SuccessOrExit(error = ProcessRouteTlv(route));
+            if (Get<RouterTable>().Contains(*aNeighbor))
+            {
+                aNeighbor = NULL; // aNeighbor is no longer valid after `ProcessRouteTlv`
+            }
         }
     }
 
@@ -2237,6 +2241,7 @@ void MleRouter::HandleChildIdRequest(const Message &         aMessage,
     router = mRouterTable.GetRouter(macAddr);
     if (router != NULL)
     {
+        // The `router` here can be invalid
         RemoveNeighbor(*router);
     }
 
@@ -3300,6 +3305,8 @@ void MleRouter::RemoveRouterLink(Router &aRouter)
 
 void MleRouter::RemoveNeighbor(Neighbor &aNeighbor)
 {
+    VerifyOrExit(!aNeighbor.IsStateInvalid(), OT_NOOP);
+
     if (&aNeighbor == &mParent)
     {
         if (IsChild())
@@ -3307,8 +3314,14 @@ void MleRouter::RemoveNeighbor(Neighbor &aNeighbor)
             IgnoreError(BecomeDetached());
         }
     }
+    else if (&aNeighbor == &mParentCandidate)
+    {
+        mParentCandidate.Clear();
+    }
     else if (!IsActiveRouter(aNeighbor.GetRloc16()))
     {
+        OT_ASSERT(mChildTable.GetChildIndex(static_cast<Child &>(aNeighbor)) < kMaxChildren);
+
         if (aNeighbor.IsStateValidOrRestoring())
         {
             Signal(OT_NEIGHBOR_TABLE_EVENT_CHILD_REMOVED, aNeighbor);
@@ -3326,12 +3339,17 @@ void MleRouter::RemoveNeighbor(Neighbor &aNeighbor)
     }
     else if (aNeighbor.IsStateValid())
     {
+        OT_ASSERT(mRouterTable.Contains(aNeighbor));
+
         Signal(OT_NEIGHBOR_TABLE_EVENT_ROUTER_REMOVED, aNeighbor);
         mRouterTable.RemoveRouterLink(static_cast<Router &>(aNeighbor));
     }
 
     aNeighbor.GetLinkInfo().Clear();
     aNeighbor.SetState(Neighbor::kStateInvalid);
+
+exit:
+    return;
 }
 
 Neighbor *MleRouter::GetNeighbor(uint16_t aAddress)
diff --git a/src/core/thread/router_table.hpp b/src/core/thread/router_table.hpp
index b3916a87f..2e2e415f4 100644
--- a/src/core/thread/router_table.hpp
+++ b/src/core/thread/router_table.hpp
@@ -265,6 +265,22 @@ public:
      */
     Router *GetRouter(const Mac::ExtAddress &aExtAddress);
 
+    /**
+     * This method returns if the router table contains a given `Neighbor` instance.
+     *
+     * @param[in]  aNeighbor  A reference to a `Neighbor`.
+     *
+     * @retval TRUE  if @p aNeighbor is a `Router` in the router table.
+     * @retval FALSE if @p aNeighbor is not a `Router` in the router table
+     *               (i.e. mParent, mParentCandidate, a `Child` of the child table).
+     *
+     */
+    bool Contains(const Neighbor &aNeighbor) const
+    {
+        return mRouters <= &static_cast<const Router &>(aNeighbor) &&
+               &static_cast<const Router &>(aNeighbor) < mRouters + Mle::kMaxRouters;
+    }
+
     /**
      * This method retains diagnostic information for a given router.
      *