ack, I have only done code review and I think it looks ok, the comment "malloc'ed in avsv_edp_susi_asgn() " in sidb.cc is correct I guess but I couldn't directly find the related malloc.
/BR HansN -----Original Message----- From: Gary Lee [mailto:gary....@dektech.com.au] Sent: den 13 januari 2014 14:46 To: Hans Feldt; Hans Nordebäck; nagendr...@oracle.com; Gary Lee Cc: opensaf-devel@lists.sourceforge.net Subject: [PATCH 1 of 1] AMFND: errors reported by valgrind [#716] osaf/services/saf/amf/amfnd/amfnd.cc | 4 +- osaf/services/saf/amf/amfnd/cbq.cc | 4 +- osaf/services/saf/amf/amfnd/ckpt_updt.cc | 4 +- osaf/services/saf/amf/amfnd/comp.cc | 14 +- osaf/services/saf/amf/amfnd/compdb.cc | 6 +- osaf/services/saf/amf/amfnd/include/avnd_util.h | 2 + osaf/services/saf/amf/amfnd/pg.cc | 6 +- osaf/services/saf/amf/amfnd/sidb.cc | 15 +- osaf/services/saf/amf/amfnd/susm.cc | 2 +- osaf/services/saf/amf/amfnd/util.cc | 145 ++++++++++++++++++++++- 10 files changed, 169 insertions(+), 33 deletions(-) * replace call to malloc(AVSV_AMF_CBK_INFO) with new() in comp.cc, so it is in line with the rest of amfnd when allocating AVSV_AMF_CBK_INFO. This eliminates the mixing of malloc/new/delete. * add functions amf_cbk_copy() and amf_csi_attr_list_copy(), which are based on the avsv equivalent but "C++ aware". This is done to avoid mixing of malloc/new/delete when dealing with 'SaAmfProtectionGroupNotificationT' in 'AVSV_AMF_CBK_INFO'. * malloc/free are now used whenever a AVSV_ATTR_NAME_VAL structure is create or deleted. Previously, there were mixed usages of malloc/new/free/delete resulting in mismatches. * fix a few places where delete was used, instead of delete []. * in pg.cc, don't set cbk_info to NULL before deleting it. diff --git a/osaf/services/saf/amf/amfnd/amfnd.cc b/osaf/services/saf/amf/amfnd/amfnd.cc --- a/osaf/services/saf/amf/amfnd/amfnd.cc +++ b/osaf/services/saf/amf/amfnd/amfnd.cc @@ -297,7 +297,7 @@ TRACE_ENTER2("Type:%u, Hdl:%llu, Inv:%llu", cbk_info->type, cbk_info->hdl, cbk_info->inv); /* Create a callback record for storing purpose. */ - rc = avsv_amf_cbk_copy(&cbk_rec, cbk_info); + rc = amf_cbk_copy(&cbk_rec, cbk_info); if (NCSCC_RC_SUCCESS != rc) goto done; @@ -317,7 +317,7 @@ LOG_ER("Couldn't find comp %s in Inter/Ext Comp DB",cbk_info->param.hc.comp_name.value); /* free the callback info */ if (cbk_rec) - avsv_amf_cbk_free(cbk_rec); + amf_cbk_free(cbk_rec); goto done; } diff --git a/osaf/services/saf/amf/amfnd/cbq.cc b/osaf/services/saf/amf/amfnd/cbq.cc --- a/osaf/services/saf/amf/amfnd/cbq.cc +++ b/osaf/services/saf/amf/amfnd/cbq.cc @@ -704,7 +704,7 @@ /* populate the msg */ msg.type = AVND_MSG_AVA; msg.info.ava->type = AVSV_AVND_AMF_CBK_MSG; - rc = avsv_amf_cbk_copy(&msg.info.ava->info.cbk_info, rec->cbk_info); + rc = amf_cbk_copy(&msg.info.ava->info.cbk_info, rec->cbk_info); if (NCSCC_RC_SUCCESS != rc) goto done; @@ -930,7 +930,7 @@ /* free the callback info */ if (rec->cbk_info) - avsv_amf_cbk_free(rec->cbk_info); + amf_cbk_free(rec->cbk_info); /* free the record */ delete rec; diff --git a/osaf/services/saf/amf/amfnd/ckpt_updt.cc b/osaf/services/saf/amf/amfnd/ckpt_updt.cc --- a/osaf/services/saf/amf/amfnd/ckpt_updt.cc +++ b/osaf/services/saf/amf/amfnd/ckpt_updt.cc @@ -220,7 +220,7 @@ } /* free the csi attributes */ if (csi_rec->attrs.list) - delete csi_rec->attrs.list; + free(csi_rec->attrs.list); /* finally free this record */ delete csi_rec; @@ -994,7 +994,7 @@ } /* free the csi attributes */ if (csi_rec->attrs.list) - delete csi_rec->attrs.list; + free(csi_rec->attrs.list); /* finally free this record */ delete csi_rec; diff --git a/osaf/services/saf/amf/amfnd/comp.cc b/osaf/services/saf/amf/amfnd/comp.cc --- a/osaf/services/saf/amf/amfnd/comp.cc +++ b/osaf/services/saf/amf/amfnd/comp.cc @@ -1892,10 +1892,7 @@ goto done; /* allocate cbk-info memory */ - if ((0 == (cbk_info = static_cast<AVSV_AMF_CBK_INFO*>(calloc(1, sizeof(AVSV_AMF_CBK_INFO)))))) { - rc = NCSCC_RC_FAILURE; - goto done; - } + cbk_info = new AVSV_AMF_CBK_INFO(); /* fill the callback params */ switch (type) { @@ -1960,7 +1957,12 @@ /* copy the attributes */ memset(&attr, 0, sizeof(AVSV_CSI_ATTRS)); if ((SA_AMF_CSI_ADD_ONE == csi_desc.csiFlags) && (curr_csi->attrs.number != 0)) { - attr.list = new AVSV_ATTR_NAME_VAL[curr_csi->attrs.number]; + attr.list = static_cast<AVSV_ATTR_NAME_VAL*> + (calloc(curr_csi->attrs.number, sizeof(AVSV_ATTR_NAME_VAL))); + if (!attr.list) { + LOG_ER("%s: calloc FAILED", __FUNCTION__); + return NCSCC_RC_FAILURE; + } memcpy(attr.list, curr_csi->attrs.list, sizeof(AVSV_ATTR_NAME_VAL) * curr_csi->attrs.number); @@ -2022,7 +2024,7 @@ done: if ((NCSCC_RC_SUCCESS != rc) && cbk_info) - avsv_amf_cbk_free(cbk_info); + amf_cbk_free(cbk_info); TRACE_LEAVE2("%u", rc); return rc; diff --git a/osaf/services/saf/amf/amfnd/compdb.cc b/osaf/services/saf/amf/amfnd/compdb.cc --- a/osaf/services/saf/amf/amfnd/compdb.cc +++ b/osaf/services/saf/amf/amfnd/compdb.cc @@ -903,21 +903,21 @@ delete [] argv; delete [] compt->saAmfCtDefCleanupCmdArgv; - delete compt->saAmfCtRelPathAmStartCmd; + delete [] compt->saAmfCtRelPathAmStartCmd; /* Free saAmfCtDefAmStartCmdArgv[i] before freeing saAmfCtDefAmStartCmdArgv */ arg_counter = 0; while ((argv = compt->saAmfCtDefAmStartCmdArgv[arg_counter++]) != NULL) delete [] argv; delete [] compt->saAmfCtDefAmStartCmdArgv; - delete compt->saAmfCtRelPathAmStopCmd; + delete [] compt->saAmfCtRelPathAmStopCmd; /* Free saAmfCtDefAmStopCmdArgv[i] before freeing saAmfCtDefAmStopCmdArgv */ arg_counter = 0; while ((argv = compt->saAmfCtDefAmStopCmdArgv[arg_counter++]) != NULL) delete [] argv; delete [] compt->saAmfCtDefAmStopCmdArgv; - delete compt->osafAmfCtRelPathHcCmd; + delete [] compt->osafAmfCtRelPathHcCmd; arg_counter = 0; while ((argv = compt->osafAmfCtDefHcCmdArgv[arg_counter++]) != NULL) delete [] argv; diff --git a/osaf/services/saf/amf/amfnd/include/avnd_util.h b/osaf/services/saf/amf/amfnd/include/avnd_util.h --- a/osaf/services/saf/amf/amfnd/include/avnd_util.h +++ b/osaf/services/saf/amf/amfnd/include/avnd_util.h @@ -51,7 +51,9 @@ void dnd_msg_free(AVSV_DND_MSG *msg); void nda_ava_msg_free(AVSV_NDA_AVA_MSG *msg); void nda_ava_msg_content_free(AVSV_NDA_AVA_MSG *msg); +void amf_csi_attr_list_copy(SaAmfCSIAttributeListT *dattr, const +SaAmfCSIAttributeListT *sattr); void amf_csi_attr_list_free(SaAmfCSIAttributeListT *attrs); +uint32_t amf_cbk_copy(AVSV_AMF_CBK_INFO **o_dcbk, const +AVSV_AMF_CBK_INFO *scbk); void amf_cbk_free(AVSV_AMF_CBK_INFO* cbk_info); void nd2nd_avnd_msg_free(AVSV_ND2ND_AVND_MSG *msg); diff --git a/osaf/services/saf/amf/amfnd/pg.cc b/osaf/services/saf/amf/amfnd/pg.cc --- a/osaf/services/saf/amf/amfnd/pg.cc +++ b/osaf/services/saf/amf/amfnd/pg.cc @@ -769,7 +769,7 @@ if (chg_mem && m_AVND_PG_TRK_IS_CHANGES_ONLY(trk)) { /*** include only the modified member ***/ - pg_param->buf.notification = new SaAmfProtectionGroupNotificationT; + pg_param->buf.notification = new +SaAmfProtectionGroupNotificationT[1]; *pg_param->buf.notification = chg_mem->info; pg_param->buf.numberOfItems = 1; @@ -780,14 +780,12 @@ /* now send the cbk msg */ rc = avnd_pg_cbk_msg_send(cb, trk, cbk_info); - /* we will free the ckb info both in success/failure case */ - cbk_info = NULL; /* reset the is_syn flag */ trk->info.is_syn = false; if ((NCSCC_RC_SUCCESS != rc) && cbk_info) - avsv_amf_cbk_free(cbk_info); + amf_cbk_free(cbk_info); TRACE_LEAVE2("rc '%u'", rc); return rc; diff --git a/osaf/services/saf/amf/amfnd/sidb.cc b/osaf/services/saf/amf/amfnd/sidb.cc --- a/osaf/services/saf/amf/amfnd/sidb.cc +++ b/osaf/services/saf/amf/amfnd/sidb.cc @@ -852,9 +852,12 @@ * Free the memory alloced for this record. */ /* free the csi attributes */ - if (csi_rec->attrs.list) - delete csi_rec->attrs.list; - + if (csi_rec->attrs.list) { + // use of free() is required as it was + // malloc'ed in avsv_edp_susi_asgn() + free(csi_rec->attrs.list); + } + /* free the pg list TBD */ TRACE_1("Comp-CSI record deletion success, Comp=%s : CSI=%s",csi_rec->comp->name.value,csi_rec->name.value); @@ -974,9 +977,11 @@ while ((curr = siq->info.list) != 0) { siq->info.list = curr->next; if (curr->attrs.list) - delete curr->attrs.list; + free(curr->attrs.list); - delete curr; + // use of free() is required as it was + // malloc'ed in avsv_edp_susi_asgn() + free(curr); } /* free the rec */ diff --git a/osaf/services/saf/amf/amfnd/susm.cc b/osaf/services/saf/amf/amfnd/susm.cc --- a/osaf/services/saf/amf/amfnd/susm.cc +++ b/osaf/services/saf/amf/amfnd/susm.cc @@ -1032,7 +1032,7 @@ osafassert(t_csi); /* free the csi attributes */ if (t_csi->attrs.list) - delete t_csi->attrs.list; + free(t_csi->attrs.list); m_AVND_SU_SI_CSI_REC_REM(*si, *t_csi); m_AVND_COMPDB_REC_CSI_REM(*(t_csi->comp), *t_csi); diff --git a/osaf/services/saf/amf/amfnd/util.cc b/osaf/services/saf/amf/amfnd/util.cc --- a/osaf/services/saf/amf/amfnd/util.cc +++ b/osaf/services/saf/amf/amfnd/util.cc @@ -298,7 +298,7 @@ compcsi_info = susi_msg->msg_info.d2n_su_si_assign.list; susi_msg->msg_info.d2n_su_si_assign.list = compcsi_info->next; if (compcsi_info->attrs.list != NULL) { - delete(compcsi_info->attrs.list); + free(compcsi_info->attrs.list); compcsi_info->attrs.list = NULL; } delete compcsi_info; @@ -324,7 +324,7 @@ AVSV_D2N_PG_TRACK_ACT_RSP_MSG_INFO *info = &pg_msg->msg_info.d2n_pg_track_act_rsp; if (info->mem_list.numberOfItems) - delete info->mem_list.notification; + delete [] info->mem_list.notification; info->mem_list.notification = 0; info->mem_list.numberOfItems = 0; @@ -420,7 +420,7 @@ case AVSV_AVND_AMF_CBK_MSG: if (msg->info.cbk_info) { - avsv_amf_cbk_free(msg->info.cbk_info); + amf_cbk_free(msg->info.cbk_info); msg->info.cbk_info = 0; } break; @@ -433,6 +433,50 @@ } /**************************************************************************** + Name : amf_csi_attr_list_copy + + Description : This routine copies the csi attribute list. + + Arguments : dattr - ptr to the destination csi-attr list + sattr - ptr to the src csi-attr list + + Return Values : NCSCC_RC_SUCCESS / NCSCC_RC_FAILURE + + Notes : None. +*********************************************************************** +*******/ void amf_csi_attr_list_copy(SaAmfCSIAttributeListT *dattr, + const SaAmfCSIAttributeListT *sattr) +{ + uint32_t cnt; + + if (!dattr || !sattr) + goto done; + + dattr->attr = new SaAmfCSIAttributeT[sattr->number]; + + for (cnt = 0; cnt < sattr->number; cnt++) { + /* alloc memory for attr name & value */ + size_t attrNameSize = strlen((char*)sattr->attr[cnt].attrName) + 1; + dattr->attr[cnt].attrName = new SaUint8T[attrNameSize]; + + size_t attrValueSize = strlen((char*)sattr->attr[cnt].attrValue) + 1; + dattr->attr[cnt].attrValue = new SaUint8T[attrNameSize]; + + /* copy the attr name & value */ + strncpy((char*)dattr->attr[cnt].attrName, + (char*)sattr->attr[cnt].attrName, attrNameSize); + strncpy((char*)dattr->attr[cnt].attrValue, + (char*)sattr->attr[cnt].attrValue, attrValueSize); + + /* increment the attr name-val pair cnt that is copied */ + dattr->number++; + } + +done: + return; +} + +/********************************************************************** +****** Name : amf_csi_attr_list_free Description : This routine frees the csi attribute list. @@ -452,18 +496,103 @@ /* free the attr name-val pair */ for (cnt = 0; cnt < attrs->number; cnt++) { - delete attrs->attr[cnt].attrName; - delete attrs->attr[cnt].attrValue; + delete [] attrs->attr[cnt].attrName; + delete [] attrs->attr[cnt].attrValue; } /* for */ /* finally free the attr list ptr */ if (attrs->attr) - delete attrs->attr; + delete [] attrs->attr; return; } /**************************************************************************** + Name : amf_cbk_copy + + Description : This routine copies the AMF callback info message. + + Arguments : o_dcbk - double ptr to the dest cbk-info (o/p) + scbk - ptr to the source cbk-info + + Return Values : NCSCC_RC_SUCCESS/NCSCC_RC_FAILURE. + + Notes : None +*********************************************************************** +*******/ uint32_t amf_cbk_copy(AVSV_AMF_CBK_INFO **o_dcbk, const +AVSV_AMF_CBK_INFO *scbk) { + uint32_t rc = NCSCC_RC_SUCCESS; + + if (!o_dcbk || !scbk) + return NCSCC_RC_FAILURE; + + /* allocate the dest cbk-info */ + *o_dcbk = new AVSV_AMF_CBK_INFO(); + + /* copy the common fields */ + memcpy(*o_dcbk, scbk, sizeof(AVSV_AMF_CBK_INFO)); + + switch (scbk->type) { + case AVSV_AMF_HC: + case AVSV_AMF_COMP_TERM: + case AVSV_AMF_CSI_REM: + case AVSV_AMF_PXIED_COMP_INST: + case AVSV_AMF_PXIED_COMP_CLEAN: + break; + + case AVSV_AMF_PG_TRACK: + /* memset notify buffer */ + memset(&(*o_dcbk)->param.pg_track.buf, 0, +sizeof(SaAmfProtectionGroupNotificationBufferT)); + + /* copy the notify buffer, if any */ + if (scbk->param.pg_track.buf.numberOfItems) { + (*o_dcbk)->param.pg_track.buf.notification = + new +SaAmfProtectionGroupNotificationT[scbk->param.pg_track.buf.numberOfItem +s]; + + for (SaUint32T i = 0; i < scbk->param.pg_track.buf.numberOfItems; ++i) { + (*o_dcbk)->param.pg_track.buf.notification[i] = + scbk->param.pg_track.buf.notification[i]; + } + (*o_dcbk)->param.pg_track.buf.numberOfItems = scbk->param.pg_track.buf.numberOfItems; + } + break; + + case AVSV_AMF_CSI_SET: + /* memset avsv & amf csi attr lists */ + memset(&(*o_dcbk)->param.csi_set.attrs, 0, sizeof(AVSV_CSI_ATTRS)); + memset(&(*o_dcbk)->param.csi_set.csi_desc.csiAttr, 0, +sizeof(SaAmfCSIAttributeListT)); + + /* copy the avsv csi attr list */ + if (scbk->param.csi_set.attrs.number) { + (*o_dcbk)->param.csi_set.attrs.list = static_cast<AVSV_ATTR_NAME_VAL*> + (calloc(scbk->param.csi_set.attrs.number, sizeof(AVSV_ATTR_NAME_VAL))); + if ((*o_dcbk)->param.csi_set.attrs.list == NULL) { + LOG_ER("%s: calloc FAILED", __FUNCTION__); + return NCSCC_RC_FAILURE; + } + + for (uint32_t i = 0; i < scbk->param.csi_set.attrs.number; ++i) { + (*o_dcbk)->param.csi_set.attrs.list[i] = + scbk->param.csi_set.attrs.list[i]; + } + (*o_dcbk)->param.csi_set.attrs.number = scbk->param.csi_set.attrs.number; + } + + /* copy the amf csi attr list */ + if (scbk->param.csi_set.csi_desc.csiAttr.number) { + amf_csi_attr_list_copy(&(*o_dcbk)->param.csi_set.csi_desc.csiAttr, + &scbk->param.csi_set.csi_desc.csiAttr); + } + break; + + default: + osafassert(0); + } + + return rc; +} + +/********************************************************************** +****** Name : amf_cbk_free Description : This routine frees callback information. @@ -490,13 +619,13 @@ case AVSV_AMF_PG_TRACK: /* free the notify buffer */ if (cbk_info->param.pg_track.buf.numberOfItems) - delete cbk_info->param.pg_track.buf.notification; + delete [] cbk_info->param.pg_track.buf.notification; break; case AVSV_AMF_CSI_SET: /* free the avsv csi attr list */ if (cbk_info->param.csi_set.attrs.number) - delete cbk_info->param.csi_set.attrs.list; + free(cbk_info->param.csi_set.attrs.list); /* free the amf csi attr list */ amf_csi_attr_list_free(&cbk_info->param.csi_set.csi_desc.csiAttr); ------------------------------------------------------------------------------ CenturyLink Cloud: The Leader in Enterprise Cloud Services. Learn Why More Businesses Are Choosing CenturyLink Cloud For Critical Workloads, Development Environments & Everything In Between. Get a Quote or Start a Free Trial Today. http://pubads.g.doubleclick.net/gampad/clk?id=119420431&iu=/4140/ostg.clktrk _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
