Sorry ignore my last comment. ( I see compt is to nullptr at the start of avnd_comptype_create().
On 20/4/17, 12:27 am, "Gary Lee" <[email protected]> wrote: Hi Nagu Thanks – good pickup. The changes in avnd_comptype_create() probably need to be reverted. It still needs to return null if not all fields are initialised. Otherwise, it might crash here: if ((comptype = avnd_comptype_create(immOmHandle, comp->saAmfCompType)) == nullptr) { LOG_ER("%s: avnd_comptype_create FAILED for '%s'", __FUNCTION__, comp->saAmfCompType.c_str()); goto done; } Gary On 20/4/17, 12:07 am, "Nagendra Kumar" <[email protected]> wrote: 1. avnd_comptype_delete() is getting called from comp_init() as well. There is chances that compt will be passed as NULL if avnd_comptype_create() fails, then Amfnd will crash at: avnd_comptype_delete()-> TRACE_ENTER2("'%s'", compt->name.c_str()); 2. Assert is not right thing to keep as it will push anyway for a crash. Please keep NULL check rather than assert. Thanks -Nagu > -----Original Message----- > From: Hoa Le [mailto:[email protected]] > Sent: 13 April 2017 15:50 > To: [email protected]; [email protected]; > [email protected]; [email protected]; > [email protected]; [email protected] > Cc: [email protected]; Hoa Le > Subject: [PATCH 1/1] amfnd: Fix illegal memory access in > avnd_comptype_delete [#2424] > > 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 | 35 ++++++++++++++++++----------------- > 1 file changed, 18 insertions(+), 17 deletions(-) > > diff --git a/src/amf/amfnd/compdb.cc b/src/amf/amfnd/compdb.cc > index cf6e3ff..d0b9f38 100644 > --- a/src/amf/amfnd/compdb.cc > +++ b/src/amf/amfnd/compdb.cc > @@ -757,15 +757,15 @@ static void > avnd_comptype_delete(amf_comp_type_t *compt) { > > TRACE("'%s'", compt->name.c_str()); > /* Free saAmfCtDefCmdEnv[i] before freeing saAmfCtDefCmdEnv */ > - if (compt->saAmfCtDefCmdEnv != nullptr) { > - arg_counter = 0; > - while ((argv = compt->saAmfCtDefCmdEnv[arg_counter++]) != nullptr) > - delete[] argv; > - delete[] compt->saAmfCtDefCmdEnv; > - } > + osafassert(compt->saAmfCtDefCmdEnv != nullptr); > + arg_counter = 0; > + while ((argv = compt->saAmfCtDefCmdEnv[arg_counter++]) != nullptr) > + delete[] argv; > + delete[] compt->saAmfCtDefCmdEnv; > > delete[] compt->saAmfCtRelPathInstantiateCmd; > > + osafassert(compt->saAmfCtDefInstantiateCmdArgv != nullptr); > /* Free saAmfCtDefInstantiateCmdArgv[i] before freeing > * saAmfCtDefInstantiateCmdArgv */ > arg_counter = 0; > @@ -775,6 +775,7 @@ static void > avnd_comptype_delete(amf_comp_type_t *compt) { > > delete[] compt->saAmfCtRelPathTerminateCmd; > > + osafassert(compt->saAmfCtDefTerminateCmdArgv != nullptr); > /* Free saAmfCtDefTerminateCmdArgv[i] before freeing > * saAmfCtDefTerminateCmdArgv */ > arg_counter = 0; > @@ -783,6 +784,8 @@ static void > avnd_comptype_delete(amf_comp_type_t *compt) { > delete[] compt->saAmfCtDefTerminateCmdArgv; > > delete[] compt->saAmfCtRelPathCleanupCmd; > + > + osafassert(compt->saAmfCtDefCleanupCmdArgv != nullptr); > /* Free saAmfCtDefCleanupCmdArgv[i] before freeing > saAmfCtDefCleanupCmdArgv */ > arg_counter = 0; > while ((argv = compt->saAmfCtDefCleanupCmdArgv[arg_counter++]) != > nullptr) > @@ -790,6 +793,8 @@ static void > avnd_comptype_delete(amf_comp_type_t *compt) { > delete[] compt->saAmfCtDefCleanupCmdArgv; > > delete[] compt->saAmfCtRelPathAmStartCmd; > + > + osafassert(compt->saAmfCtDefAmStartCmdArgv != nullptr); > /* Free saAmfCtDefAmStartCmdArgv[i] before freeing > saAmfCtDefAmStartCmdArgv */ > arg_counter = 0; > while ((argv = compt->saAmfCtDefAmStartCmdArgv[arg_counter++]) != > nullptr) > @@ -797,6 +802,8 @@ static void > avnd_comptype_delete(amf_comp_type_t *compt) { > delete[] compt->saAmfCtDefAmStartCmdArgv; > > delete[] compt->saAmfCtRelPathAmStopCmd; > + > + osafassert(compt->saAmfCtDefAmStopCmdArgv != nullptr); > /* Free saAmfCtDefAmStopCmdArgv[i] before freeing > saAmfCtDefAmStopCmdArgv */ > arg_counter = 0; > while ((argv = compt->saAmfCtDefAmStopCmdArgv[arg_counter++]) != > nullptr) > @@ -804,6 +811,8 @@ static void > avnd_comptype_delete(amf_comp_type_t *compt) { > delete[] compt->saAmfCtDefAmStopCmdArgv; > > delete[] compt->osafAmfCtRelPathHcCmd; > + > + osafassert(compt->osafAmfCtDefHcCmdArgv != nullptr); > arg_counter = 0; > while ((argv = compt->osafAmfCtDefHcCmdArgv[arg_counter++]) != > nullptr) > delete[] argv; > @@ -817,8 +826,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 +835,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 +844,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 +1001,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.7.4 ------------------------------------------------------------------------------ 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 [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
