Fix potential risks (#2516)

- What I did Fix the following potential risks: Uncaught exceptions in /src/nvos-swss/orchagent/main.cpp and /src/nvos-swss/portsyncd/portsyncd.cpp Uninitialized scalar fields in /src/nvos-swss/orchagent/port.h Unchecked return value in /src/nvos-swss/orchagent/portsorch.cpp Pointer to local outside scope in /src/nvos-swss/orchagent/portsorch.cpp - How I did it Add "catch" to the uncaught exceptions Add initialization value to the uninitialized fields Add a check to the unchecked return value Delete an unnecessary comma - How I verified it Inspection and sanity tests Signed-off-by: Liran Artzi <[email protected]>
sonic-net · Jan 23, 2023 · 7f03db2 · 7f03db2
1 parent 383ee68
commit 7f03db2
Show file tree

Hide file tree

Showing 4 changed files with 103 additions and 73 deletions.
diff --git a/orchagent/main.cpp b/orchagent/main.cpp
@@ -170,9 +170,17 @@ void getCfgSwitchType(DBConnector *cfgDb, string &switch_type)
 {
     Table cfgDeviceMetaDataTable(cfgDb, CFG_DEVICE_METADATA_TABLE_NAME);
 
-    if (!cfgDeviceMetaDataTable.hget("localhost", "switch_type", switch_type))
+    try
     {
-        //Switch type is not configured. Consider it default = "switch" (regular switch)
+        if (!cfgDeviceMetaDataTable.hget("localhost", "switch_type", switch_type))
+        {
+            //Switch type is not configured. Consider it default = "switch" (regular switch)
+            switch_type = "switch";
+        }
+    }
+    catch(const std::system_error& e)
+    {
+        SWSS_LOG_ERROR("System error: %s", e.what());
         switch_type = "switch";
     }
 
@@ -196,64 +204,72 @@ bool getSystemPortConfigList(DBConnector *cfgDb, DBConnector *appDb, vector<sai_
         return true;
     }
 
-    string value;
-    if (!cfgDeviceMetaDataTable.hget("localhost", "switch_id", value))
+    try
     {
-        //VOQ switch id is not configured.
-        SWSS_LOG_ERROR("VOQ switch id is not configured");
-        return false;
-    }
+        string value;
+        if (!cfgDeviceMetaDataTable.hget("localhost", "switch_id", value))
+        {
+            //VOQ switch id is not configured.
+            SWSS_LOG_ERROR("VOQ switch id is not configured");
+            return false;
+        }
 
-    if (value.size())
-        gVoqMySwitchId = stoi(value);
+        if (value.size())
+            gVoqMySwitchId = stoi(value);
 
-    if (gVoqMySwitchId < 0)
-    {
-        SWSS_LOG_ERROR("Invalid VOQ switch id %d configured", gVoqMySwitchId);
-        return false;
-    }
+        if (gVoqMySwitchId < 0)
+        {
+            SWSS_LOG_ERROR("Invalid VOQ switch id %d configured", gVoqMySwitchId);
+            return false;
+        }
 
-    if (!cfgDeviceMetaDataTable.hget("localhost", "max_cores", value))
-    {
-        //VOQ max cores is not configured.
-        SWSS_LOG_ERROR("VOQ max cores is not configured");
-        return false;
-    }
+        if (!cfgDeviceMetaDataTable.hget("localhost", "max_cores", value))
+        {
+            //VOQ max cores is not configured.
+            SWSS_LOG_ERROR("VOQ max cores is not configured");
+            return false;
+        }
 
-    if (value.size())
-        gVoqMaxCores = stoi(value);
+        if (value.size())
+            gVoqMaxCores = stoi(value);
 
-    if (gVoqMaxCores == 0)
-    {
-        SWSS_LOG_ERROR("Invalid VOQ max cores %d configured", gVoqMaxCores);
-        return false;
-    }
+        if (gVoqMaxCores == 0)
+        {
+            SWSS_LOG_ERROR("Invalid VOQ max cores %d configured", gVoqMaxCores);
+            return false;
+        }
 
-    if (!cfgDeviceMetaDataTable.hget("localhost", "hostname", value))
-    {
-        // hostname is not configured.
-        SWSS_LOG_ERROR("Host name is not configured");
-        return false;
-    }
-    gMyHostName = value;
+        if (!cfgDeviceMetaDataTable.hget("localhost", "hostname", value))
+        {
+            // hostname is not configured.
+            SWSS_LOG_ERROR("Host name is not configured");
+            return false;
+        }
+        gMyHostName = value;
 
-    if (!gMyHostName.size())
-    {
-        SWSS_LOG_ERROR("Invalid host name %s configured", gMyHostName.c_str());
-        return false;
-    }
+        if (!gMyHostName.size())
+        {
+            SWSS_LOG_ERROR("Invalid host name %s configured", gMyHostName.c_str());
+            return false;
+        }
 
-    if (!cfgDeviceMetaDataTable.hget("localhost", "asic_name", value))
-    {
-        // asic_name is not configured.
-        SWSS_LOG_ERROR("Asic name is not configured");
-        return false;
-    }
-    gMyAsicName = value;
+        if (!cfgDeviceMetaDataTable.hget("localhost", "asic_name", value))
+        {
+            // asic_name is not configured.
+            SWSS_LOG_ERROR("Asic name is not configured");
+            return false;
+        }
+        gMyAsicName = value;
 
-    if (!gMyAsicName.size())
+        if (!gMyAsicName.size())
+        {
+            SWSS_LOG_ERROR("Invalid asic name %s configured", gMyAsicName.c_str());
+            return false;
+        }
+    }
+    catch(const std::system_error& e)
     {
-        SWSS_LOG_ERROR("Invalid asic name %s configured", gMyAsicName.c_str());
+        SWSS_LOG_ERROR("System error: %s", e.what());
         return false;
     }
 

diff --git a/orchagent/port.h b/orchagent/port.h
@@ -113,7 +113,7 @@ class Port
     }
 
     std::string         m_alias;
-    Type                m_type;
+    Type                m_type = UNKNOWN;
     int                 m_index = 0;    // PHY_PORT: index
     uint32_t            m_mtu = DEFAULT_MTU;
     uint32_t            m_speed = 0;    // Mbps

diff --git a/orchagent/portsorch.cpp b/orchagent/portsorch.cpp
@@ -2713,7 +2713,11 @@ sai_status_t PortsOrch::removePort(sai_object_id_t port_id)
      */
     if (getPort(port_id, port))
     {
-        setPortAdminStatus(port, false);
+        /* Bring port down before removing port */
+        if (!setPortAdminStatus(port, false))
+        {
+            SWSS_LOG_ERROR("Failed to set admin status to DOWN to remove port %" PRIx64, port_id);
+        }
     }
     /* else : port is in default state or not yet created */
 
@@ -4457,7 +4461,7 @@ void PortsOrch::doTask()
         APP_LAG_TABLE_NAME,
         APP_LAG_MEMBER_TABLE_NAME,
         APP_VLAN_TABLE_NAME,
-        APP_VLAN_MEMBER_TABLE_NAME,
+        APP_VLAN_MEMBER_TABLE_NAME
     };
 
     for (auto tableName: tableOrder)

diff --git a/portsyncd/portsyncd.cpp b/portsyncd/portsyncd.cpp
@@ -46,33 +46,33 @@ void handlePortConfigFromConfigDB(ProducerStateTable &p, DBConnector &cfgDb, boo
 
 int main(int argc, char **argv)
 {
-    Logger::linkToDbNative("portsyncd");
-    int opt;
-
-    while ((opt = getopt(argc, argv, "v:h")) != -1 )
+    try
     {
-        switch (opt)
+        Logger::linkToDbNative("portsyncd");
+        int opt;
+
+        while ((opt = getopt(argc, argv, "v:h")) != -1 )
         {
-        case 'h':
-            usage();
-            return 1;
-        default: /* '?' */
-            usage();
-            return EXIT_FAILURE;
+            switch (opt)
+            {
+            case 'h':
+                usage();
+                return 1;
+            default: /* '?' */
+                usage();
+                return EXIT_FAILURE;
+            }
         }
-    }
 
-    DBConnector cfgDb("CONFIG_DB", 0);
-    DBConnector appl_db("APPL_DB", 0);
-    DBConnector state_db("STATE_DB", 0);
-    ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME);
+        DBConnector cfgDb("CONFIG_DB", 0);
+        DBConnector appl_db("APPL_DB", 0);
+        DBConnector state_db("STATE_DB", 0);
+        ProducerStateTable p(&appl_db, APP_PORT_TABLE_NAME);
 
-    WarmStart::initialize("portsyncd", "swss");
-    WarmStart::checkWarmStart("portsyncd", "swss");
-    const bool warm = WarmStart::isWarmStart();
+        WarmStart::initialize("portsyncd", "swss");
+        WarmStart::checkWarmStart("portsyncd", "swss");
+        const bool warm = WarmStart::isWarmStart();
 
-    try
-    {
         NetLink netlink;
         Select s;
 
@@ -136,6 +136,16 @@ int main(int argc, char **argv)
             }
         }
     }
+    catch (const swss::RedisError& e)
+    {
+        cerr << "Exception \"" << e.what() << "\" was thrown in daemon" << endl;
+        return EXIT_FAILURE;
+    }
+    catch (const std::out_of_range& e)
+    {
+        cerr << "Exception \"" << e.what() << "\" was thrown in daemon" << endl;
+        return EXIT_FAILURE;
+    }
     catch (const std::exception& e)
     {
         cerr << "Exception \"" << e.what() << "\" was thrown in daemon" << endl;