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" <[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
