Change gAsicInstance to type string with max length limit (sonic-net#…

…1526) **What I did** 1. Change gAsicInstance to type string with max length limit 2. Replace malloc with calloc to prevent risk of integer overflow, in test code **Why I did it** 1. Remove `calloc` manual memory management. 2. Graceful release memory before exiting. 3. Save memory if input is very long **How I verified it** Ad-hoc test with different input arguments.
theasianpianist · Dec 9, 2020 · 64576f0 · 64576f0
1 parent c7ee75f
commit 64576f0
Show file tree

Hide file tree

Showing 2 changed files with 18 additions and 8 deletions.
diff --git a/orchagent/main.cpp b/orchagent/main.cpp
@@ -56,7 +56,7 @@ bool gLogRotate = false;
 bool gSaiRedisLogRotate = false;
 bool gSyncMode = false;
 sai_redis_communication_mode_t gRedisCommunicationMode = SAI_REDIS_COMMUNICATION_MODE_REDIS_ASYNC;
-char *gAsicInstance = NULL;
+string gAsicInstance;
 
 extern bool gIsNatSupported;
 
@@ -170,8 +170,17 @@ int main(int argc, char **argv)
             gBatchSize = atoi(optarg);
             break;
         case 'i':
-            gAsicInstance = (char *)calloc(strlen(optarg)+1, sizeof(char));
-            memcpy(gAsicInstance, optarg, strlen(optarg));
+            {
+                // Limit asic instance string max length
+                size_t len = strnlen(optarg, SAI_MAX_HARDWARE_ID_LEN);
+                // Check if input is longer and warn
+                if (len == SAI_MAX_HARDWARE_ID_LEN && optarg[len+1] != '\0')
+                {
+                    SWSS_LOG_WARN("ASIC instance_id length > SAI_MAX_HARDWARE_ID_LEN, LIMITING !!");
+                }
+                // If longer, trancate into a string
+                gAsicInstance.assign(optarg, len);
+            }
             break;
         case 'm':
             gMacAddress = MacAddress(optarg);
@@ -287,11 +296,12 @@ int main(int argc, char **argv)
 
     sai_switch_api->set_switch_attribute(gSwitchId, &attr);
 
-    if (gAsicInstance)
+    if (!gAsicInstance.empty())
     {
         attr.id = SAI_SWITCH_ATTR_SWITCH_HARDWARE_INFO;
-        attr.value.s8list.count = (uint32_t)(strlen(gAsicInstance)+1);
-        attr.value.s8list.list = (int8_t*)gAsicInstance;
+        attr.value.s8list.count = (uint32_t)gAsicInstance.size();
+        // TODO: change SAI definition of `list` to `const char *`
+        attr.value.s8list.list = (int8_t *)const_cast<char *>(gAsicInstance.c_str());
         attrs.push_back(attr);
     }
 

diff --git a/tests/mock_tests/aclorch_ut.cpp b/tests/mock_tests/aclorch_ut.cpp
@@ -498,7 +498,7 @@ namespace aclorch_test
                     switch (meta->attrvaluetype)
                     {
                         case SAI_ATTR_VALUE_TYPE_INT32_LIST:
-                            new_attr.value.s32list.list = (int32_t *)malloc(sizeof(int32_t) * attr.value.s32list.count);
+                            new_attr.value.s32list.list = (int32_t *)calloc(attr.value.s32list.count, sizeof(int32_t));
                             new_attr.value.s32list.count = attr.value.s32list.count;
                             m_s32list_pool.emplace_back(new_attr.value.s32list.list);
                             break;
@@ -555,7 +555,7 @@ namespace aclorch_test
                     switch (meta->attrvaluetype)
                     {
                         case SAI_ATTR_VALUE_TYPE_INT32_LIST:
-                            new_attr.value.s32list.list = (int32_t *)malloc(sizeof(int32_t) * attr.value.s32list.count);
+                            new_attr.value.s32list.list = (int32_t *)calloc(attr.value.s32list.count, sizeof(int32_t));
                             new_attr.value.s32list.count = attr.value.s32list.count;
                             m_s32list_pool.emplace_back(new_attr.value.s32list.list);
                             break;