[devel] [PATCH 1/1] amfnd: Fix illegal memory access in avnd_comptype_delete [#2424]

[Hoa Le]

Problem:
- There are some unsafe memory accesses which may cause segfault in
avnd_comptype_delete() function.

Fix:
- Check if a pointer is valid before accessing it.
- Minor update in avnd_comptype_create() function to avoid unnecessary
function call to avnd_comptype_delete().
---
 src/amf/amfnd/compdb.cc | 95 +++++++++++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

diff --git a/src/amf/amfnd/compdb.cc b/src/amf/amfnd/compdb.cc
index cf6e3ff..decf3ab 100644
--- a/src/amf/amfnd/compdb.cc
+++ b/src/amf/amfnd/compdb.cc
@@ -766,48 +766,67 @@ static void avnd_comptype_delete(amf_comp_type_t *compt) {
 
   delete[] compt->saAmfCtRelPathInstantiateCmd;
 
-  /* Free saAmfCtDefInstantiateCmdArgv[i] before freeing
-   * saAmfCtDefInstantiateCmdArgv */
-  arg_counter = 0;
-  while ((argv = compt->saAmfCtDefInstantiateCmdArgv[arg_counter++]) != 
nullptr)
-    delete[] argv;
-  delete[] compt->saAmfCtDefInstantiateCmdArgv;
+  if (compt->saAmfCtDefInstantiateCmdArgv != nullptr) {
+    /* Free saAmfCtDefInstantiateCmdArgv[i] before freeing
+     * saAmfCtDefInstantiateCmdArgv */
+    arg_counter = 0;
+    while ((argv = compt->saAmfCtDefInstantiateCmdArgv[arg_counter++]) !=
+            nullptr)
+      delete[] argv;
+    delete[] compt->saAmfCtDefInstantiateCmdArgv;
+  }
 
   delete[] compt->saAmfCtRelPathTerminateCmd;
 
-  /* Free saAmfCtDefTerminateCmdArgv[i] before freeing
-   * saAmfCtDefTerminateCmdArgv */
-  arg_counter = 0;
-  while ((argv = compt->saAmfCtDefTerminateCmdArgv[arg_counter++]) != nullptr)
-    delete[] argv;
-  delete[] compt->saAmfCtDefTerminateCmdArgv;
+  if (compt->saAmfCtDefTerminateCmdArgv != nullptr) {
+    /* Free saAmfCtDefTerminateCmdArgv[i] before freeing
+     * saAmfCtDefTerminateCmdArgv */
+    arg_counter = 0;
+    while ((argv = compt->saAmfCtDefTerminateCmdArgv[arg_counter++]) != 
nullptr)
+      delete[] argv;
+    delete[] compt->saAmfCtDefTerminateCmdArgv;
+  }
 
   delete[] compt->saAmfCtRelPathCleanupCmd;
-  /* Free saAmfCtDefCleanupCmdArgv[i] before freeing saAmfCtDefCleanupCmdArgv 
*/
-  arg_counter = 0;
-  while ((argv = compt->saAmfCtDefCleanupCmdArgv[arg_counter++]) != nullptr)
-    delete[] argv;
-  delete[] compt->saAmfCtDefCleanupCmdArgv;
+
+  if (compt->saAmfCtDefCleanupCmdArgv != nullptr) {
+    /* Free saAmfCtDefCleanupCmdArgv[i] before freeing
+     * saAmfCtDefCleanupCmdArgv */
+    arg_counter = 0;
+    while ((argv = compt->saAmfCtDefCleanupCmdArgv[arg_counter++]) != nullptr)
+      delete[] argv;
+    delete[] compt->saAmfCtDefCleanupCmdArgv;
+  }
 
   delete[] compt->saAmfCtRelPathAmStartCmd;
-  /* Free saAmfCtDefAmStartCmdArgv[i] before freeing saAmfCtDefAmStartCmdArgv 
*/
-  arg_counter = 0;
-  while ((argv = compt->saAmfCtDefAmStartCmdArgv[arg_counter++]) != nullptr)
-    delete[] argv;
-  delete[] compt->saAmfCtDefAmStartCmdArgv;
+
+  if (compt->saAmfCtDefAmStartCmdArgv != nullptr) {
+    /* Free saAmfCtDefAmStartCmdArgv[i] before freeing
+     *  saAmfCtDefAmStartCmdArgv */
+    arg_counter = 0;
+    while ((argv = compt->saAmfCtDefAmStartCmdArgv[arg_counter++]) != nullptr)
+      delete[] argv;
+    delete[] compt->saAmfCtDefAmStartCmdArgv;
+  }
 
   delete[] compt->saAmfCtRelPathAmStopCmd;
-  /* Free saAmfCtDefAmStopCmdArgv[i] before freeing saAmfCtDefAmStopCmdArgv */
-  arg_counter = 0;
-  while ((argv = compt->saAmfCtDefAmStopCmdArgv[arg_counter++]) != nullptr)
-    delete[] argv;
-  delete[] compt->saAmfCtDefAmStopCmdArgv;
+
+  if (compt->saAmfCtDefAmStopCmdArgv != nullptr) {
+    /* Free saAmfCtDefAmStopCmdArgv[i] before freeing saAmfCtDefAmStopCmdArgv 
*/
+    arg_counter = 0;
+    while ((argv = compt->saAmfCtDefAmStopCmdArgv[arg_counter++]) != nullptr)
+      delete[] argv;
+    delete[] compt->saAmfCtDefAmStopCmdArgv;
+  }
 
   delete[] compt->osafAmfCtRelPathHcCmd;
-  arg_counter = 0;
-  while ((argv = compt->osafAmfCtDefHcCmdArgv[arg_counter++]) != nullptr)
-    delete[] argv;
-  delete[] compt->osafAmfCtDefHcCmdArgv;
+
+  if (compt->osafAmfCtDefHcCmdArgv != nullptr) {
+    arg_counter = 0;
+    while ((argv = compt->osafAmfCtDefHcCmdArgv[arg_counter++]) != nullptr)
+      delete[] argv;
+    delete[] compt->osafAmfCtDefHcCmdArgv;
+  }
 
   delete compt;
 
@@ -817,8 +836,7 @@ static void avnd_comptype_delete(amf_comp_type_t *compt) {
 static amf_comp_type_t *avnd_comptype_create(SaImmHandleT immOmHandle,
                                              const std::string &dn) {
   SaImmAccessorHandleT accessorHandle;
-  amf_comp_type_t *compt;
-  int rc = -1;
+  amf_comp_type_t *compt = nullptr;
   unsigned int i;
   unsigned int j;
   const char *str;
@@ -827,8 +845,6 @@ static amf_comp_type_t *avnd_comptype_create(SaImmHandleT 
immOmHandle,
 
   TRACE_ENTER2("'%s'", dn.c_str());
 
-  compt = new amf_comp_type_t();
-
   (void)amf_saImmOmAccessorInitialize(immOmHandle, accessorHandle);
 
   if (amf_saImmOmAccessorGet_o2(immOmHandle, accessorHandle, dn, nullptr,
@@ -838,6 +854,8 @@ static amf_comp_type_t *avnd_comptype_create(SaImmHandleT 
immOmHandle,
     goto done;
   }
 
+  compt = new amf_comp_type_t();
+
   compt->name = dn;
 
   if (immutil_getAttr(const_cast<SaImmAttrNameT>("saAmfCtCompCategory"),
@@ -993,14 +1011,7 @@ static amf_comp_type_t *avnd_comptype_create(SaImmHandleT 
immOmHandle,
                       &compt->saAmfCtDefDisableRestart) != SA_AIS_OK)
     compt->saAmfCtDefDisableRestart = false;
 
-  rc = 0;
-
 done:
-  if (rc != 0) {
-    avnd_comptype_delete(compt);
-    compt = nullptr;
-  }
-
   (void)immutil_saImmOmAccessorFinalize(accessorHandle);
 
   TRACE_LEAVE();
-- 
2.9.3


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Opensaf-devel mailing list
Opensaf-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/opensaf-devel