Hi Hoa Le, I could see that there is NULL check below assert() you had added. So, let it be also a NULL check and if this is NULL, you can throw some log. As ticket mention that Amfnd has crashed accessing NULL pointer, if you add assert, it will crash anyway(in case it hits in some scenarios). So, I would prefer NULL check with logs.
Thanks -Nagu From: Hoa Le [mailto:[email protected]] Sent: 21 April 2017 07:15 To: Gary Lee; Nagendra Kumar; [email protected]; [email protected]; Praveen Malviya; [email protected] Cc: [email protected] Subject: Re: [PATCH 1/1] amfnd: Fix illegal memory access in avnd_comptype_delete [#2424] Hi, Thanks for your comments, 1. As pointed out by Gary, there is a NULL check at the beginning of avnd_comptype_delete() function, so it is still safe if we pass a NULL comptype to this function when avnd_comptype_create() fails. static void avnd_comptype_delete(amf_comp_type_t *compt) { int arg_counter; char *argv; TRACE_ENTER(); if (!compt) { TRACE_LEAVE(); return; } 2. I noticed that there is an osafassert for every comptype attribute in avnd_comptype_create(), so the comptype attributes will never be NULL after this function ends. In addition, I know that NULL check is safer in a release build and assert is useful for debug purpose. So if we have a long term plan to change osafassert for debug build only, I would like to keep these asserts in the function. And if we plan to remove all osafassert, I think NULL check is the better choice here. Please share your comment on the second point so we can process this issue. Thank you very much. -- Best regards, Hoa Le On 04/19/2017 09:33 PM, Gary Lee wrote: 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" HYPERLINK "mailto:[email protected]";<[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" HYPERLINK "mailto:[email protected]";<[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: HYPERLINK "mailto:[email protected]"[email protected]; HYPERLINK "mailto:[email protected]"[email protected]; > HYPERLINK "mailto:[email protected]"[email protected]; HYPERLINK "mailto:[email protected]"[email protected]; > HYPERLINK "mailto:[email protected]"[email protected]; HYPERLINK "mailto:[email protected]"[email protected] > Cc: HYPERLINK "mailto:[email protected]"[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
