[teamsyncd]: Fix bug when removing selectable in select function (#665)

In the current implementation, the selectable is removed inside the
select() function. However, the select function will retrieve multiple
file descriptors at one time. If the file descriptor is removed, it
will cause segmentation fault.

Signed-off-by: Shu0T1an ChenG <[email protected]>

Author: Shuotian Cheng

teamsyncd/teamsync.cpp

@@ -24,13 +24,34 @@ TeamSync::TeamSync(DBConnector *db, DBConnector *stateDb, Select *select) :
 {
 }
 
+void TeamSync::doSelectableTask()
+{
+    /* Start to track the new team instances */
+    for (auto s : m_selectablesToAdd)
+    {
+        m_select->addSelectable(m_teamSelectables[s].get());
+    }
+
+    m_selectablesToAdd.clear();
+
+    /* No longer track the deprecated team instances */
+    for (auto s : m_selectablesToRemove)
+    {
+        m_select->removeSelectable(m_teamSelectables[s].get());
+        m_teamSelectables.erase(s);
+    }
+
+    m_selectablesToRemove.clear();
+}
+
 void TeamSync::onMsg(int nlmsg_type, struct nl_object *obj)
 {
     struct rtnl_link *link = (struct rtnl_link *)obj;
     if ((nlmsg_type != RTM_NEWLINK) && (nlmsg_type != RTM_DELLINK))
         return;
 
     string lagName = rtnl_link_get_name(link);
+
     /* Listens to LAG messages */
     char *type = rtnl_link_get_type(link);
     if (!type || (strcmp(type, TEAM_DRV_NAME) != 0))
@@ -63,35 +84,40 @@ void TeamSync::addLag(const string &lagName, int ifindex, bool admin_state,
                    lagName.c_str(), admin_state ? "up" : "down", oper_state ? "up" : "down");
 
     /* Return when the team instance has already been tracked */
-    if (m_teamPorts.find(lagName) != m_teamPorts.end())
+    if (m_teamSelectables.find(lagName) != m_teamSelectables.end())
         return;
 
-    /* Start track the team instance */
-    auto sync = make_shared<TeamPortSync>(lagName, ifindex, &m_lagMemberTable);
-    m_select->addSelectable(sync.get());
-    m_teamPorts[lagName] = sync;
-
     fvVector.clear();
     FieldValueTuple s("state", "ok");
     fvVector.push_back(s);
     m_stateLagTable.set(lagName, fvVector);
+
+    /* Create the team instance */
+    auto sync = make_shared<TeamPortSync>(lagName, ifindex, &m_lagMemberTable);
+    m_teamSelectables[lagName] = sync;
+    m_selectablesToAdd.insert(lagName);
 }
 
 void TeamSync::removeLag(const string &lagName)
 {
+    /* Delete all members */
+    auto selectable = m_teamSelectables[lagName];
+    for (auto it : selectable->m_lagMembers)
+    {
+        m_lagMemberTable.del(lagName + ":" + it.first);
+    }
+
     /* Delete the LAG */
     m_lagTable.del(lagName);
 
     SWSS_LOG_INFO("Remove %s", lagName.c_str());
 
     /* Return when the team instance hasn't been tracked before */
-    if (m_teamPorts.find(lagName) == m_teamPorts.end())
+    if (m_teamSelectables.find(lagName) == m_teamSelectables.end())
         return;
 
-    /* No longer track the current team instance */
-    m_select->removeSelectable(m_teamPorts[lagName].get());
-    m_teamPorts.erase(lagName);
     m_stateLagTable.del(lagName);
+    m_selectablesToRemove.insert(lagName);
 }
 
 const struct team_change_handler TeamSync::TeamPortSync::gPortChangeHandler = {
@@ -114,7 +140,8 @@ TeamSync::TeamPortSync::TeamPortSync(const string &lagName, int ifindex,
     }
 
     int err = team_init(m_team, ifindex);
-    if (err) {
+    if (err)
+    {
         team_free(m_team);
         m_team = NULL;
         SWSS_LOG_ERROR("Unable to init team socket");
@@ -123,7 +150,8 @@ TeamSync::TeamPortSync::TeamPortSync(const string &lagName, int ifindex,
     }
 
     err = team_change_handler_register(m_team, &gPortChangeHandler, this);
-    if (err) {
+    if (err)
+    {
         team_free(m_team);
         m_team = NULL;
         SWSS_LOG_ERROR("Unable to register port change event");

teamsyncd/teamsync.h

@@ -18,12 +18,12 @@ class TeamSync : public NetMsg
 public:
     TeamSync(DBConnector *db, DBConnector *stateDb, Select *select);
 
-    /*
-     * Listens to RTM_NEWLINK and RTM_DELLINK to undestand if there is a new
-     * team device
-     */
+    /* Listen to RTM_NEWLINK, RTM_DELLINK to track team devices */
     virtual void onMsg(int nlmsg_type, struct nl_object *obj);
 
+    /* Handle all selectables add/removal events */
+    void doSelectableTask();
+
     class TeamPortSync : public Selectable
     {
     public:
@@ -35,6 +35,8 @@ class TeamSync : public NetMsg
         int getFd() override;
         void readData() override;
 
+        /* member_name -> enabled|disabled */
+        std::map<std::string, bool> m_lagMembers;
     protected:
         int onChange();
         static int teamdHandler(struct team_handle *th, void *arg,
@@ -45,7 +47,6 @@ class TeamSync : public NetMsg
         struct team_handle *m_team;
         std::string m_lagName;
         int m_ifindex;
-        std::map<std::string, bool> m_lagMembers; /* map[ifname] = status (enabled|disabled) */
     };
 
 protected:
@@ -58,7 +59,11 @@ class TeamSync : public NetMsg
     ProducerStateTable m_lagTable;
     ProducerStateTable m_lagMemberTable;
     Table m_stateLagTable;
-    std::map<std::string, std::shared_ptr<TeamPortSync> > m_teamPorts;
+
+    /* Store selectables needed to be updated in doSelectableTask function */
+    std::set<std::string> m_selectablesToAdd;
+    std::set<std::string> m_selectablesToRemove;
+    std::map<std::string, std::shared_ptr<TeamPortSync> > m_teamSelectables;
 };
 
 }

teamsyncd/teamsyncd.cpp

@@ -35,6 +35,8 @@ int main(int argc, char **argv)
             {
                 Selectable *temps;
                 s.select(&temps);
+
+                sync.doSelectableTask();
             }
         }
         catch (const std::exception& e)