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
