Dear Anders Widell,
Yes, as you mentioned, this patch offer a change for this behavior. The reason is as bellowing: * Currently, when API succeeds, memory is free in user defined callback, not really by MDS itself. Only in some cases API fails, MDS frees memory, in some cases MDS does not. So source code is not strictly same as PR document. * MDS do not have method to check user init memory by using the macro m_MDS_ALLOC_DIRECT_BUFF(x) so this behavior is non-defensive coding. * Personally, I think that memory should be init/free in the same level except specific reason, I cannot find reason in this case. Thank you and best regards, Hoang From: Anders Widell [mailto:anders.wid...@ericsson.com] Sent: Wednesday, March 8, 2017 9:49 PM To: Hoang Vo <hoang.m...@dektech.com.au>; mahesh.va...@oracle.com Cc: opensaf-devel@lists.sourceforge.net Subject: Re: [PATCH 1 of 2] mds: handle memory leak [#1860] Hi! I am trying to understand how these MDS APIs are intended to work. When I read Section 3.2.9.1.7 in OpenSAF_MDSv_PR.odt in the opensaf-internal-docs Mercurial repository, it looks like it is the responsibility of the MDS library to free the provided buffer that was passed in to MDS_DIRECT_SEND, no matter if the MDS API call succeeds or fails. Doesn't this patch change this behaviour in MDS? regards, Anders Widell On 03/06/2017 09:00 AM, Hoang Vo wrote: src/base/sysf_mem.c | 3 +++ src/mds/mds_c_sndrcv.c | 34 +++++++++++++++------------------- 2 files changed, 18 insertions(+), 19 deletions(-) Some error handling does not clean internal memory. Error handling in dirrect send case clear user memory seem inconsistence, mds should let creater manage its memory in error cases. action: implement as proposed. diff --git a/src/base/sysf_mem.c b/src/base/sysf_mem.c --- a/src/base/sysf_mem.c +++ b/src/base/sysf_mem.c @@ -1,6 +1,7 @@ /* -*- OpenSAF -*- * * (C) Copyright 2008 The OpenSAF Foundation + * Copyright Ericsson AB 2017 - All Rights Reserved. * * This program is distributed in the hope that it will be useful, but * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY @@ -428,6 +429,8 @@ USRBUF *sysf_alloc_pkt(unsigned char poo if (pool_id >= UB_MAX_POOLS) { m_PMGR_UNLK(&gl_ub_pool_mgr.lock); m_LEAP_DBG_SINK_VOID; + m_NCS_MEM_FREE(ub, NCS_MEM_REGION_IO_DATA_HDR, NCS_SERVICE_ID_OS_SVCS, 2); + ub = (USRBUF *)0; return NULL; } ud = (USRDATA *)gl_ub_pool_mgr.pools[pool_id].mem_alloc(sizeof(USRDATA), pool_id, priority); diff --git a/src/mds/mds_c_sndrcv.c b/src/mds/mds_c_sndrcv.c --- a/src/mds/mds_c_sndrcv.c +++ b/src/mds/mds_c_sndrcv.c @@ -1,6 +1,7 @@ /* -*- OpenSAF -*- * * (C) Copyright 2008 The OpenSAF Foundation + * Copyright Ericsson AB 2017 - All Rights Reserved. * * This program is distributed in the hope that it will be useful, but * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY @@ -420,10 +421,6 @@ static uint32_t mds_mcm_direct_send(NCSM memset(&req, 0, sizeof(req)); if ((info->info.svc_direct_send.i_priority < MDS_SEND_PRIORITY_LOW) || (info->info.svc_direct_send.i_priority > MDS_SEND_PRIORITY_VERY_HIGH)) { - if (info->info.svc_direct_send.i_direct_buff != NULL) { - m_MDS_FREE_DIRECT_BUFF(info->info.svc_direct_send.i_direct_buff); - info->info.svc_direct_send.i_direct_buff = NULL; - } m_MDS_LOG_ERR("MDS_SND_RCV: Priority defined is not in range\n"); return NCSCC_RC_FAILURE; } @@ -432,13 +429,11 @@ static uint32_t mds_mcm_direct_send(NCSM m_MDS_LOG_ERR("MDS_SND_RCV: Send Message Direct Buff is NULL\n"); return NCSCC_RC_FAILURE; } else if (info->info.svc_direct_send.i_direct_buff_len > MDS_DIRECT_BUF_MAXSIZE) { - mds_free_direct_buff(info->info.svc_direct_send.i_direct_buff); m_MDS_LOG_ERR("MDS_SND_RCV: Send Message Direct Buff Len is greater than SEND SIZE\n"); return NCSCC_RC_FAILURE; } if ((info->info.svc_direct_send.i_to_svc == 0) || (info->i_svc_id == 0)) { - m_MDS_FREE_DIRECT_BUFF(info->info.svc_direct_send.i_direct_buff); m_MDS_LOG_ERR("MDS_SND_RCV: Source or Dest service provided is Null, src svc_id = %s(%d), dest svc_id = %s(%d) \n", get_svc_names(info->i_svc_id), info->i_svc_id, get_svc_names(info->info.svc_direct_send.i_to_svc), info->info.svc_direct_send.i_to_svc); return NCSCC_RC_FAILURE; @@ -633,11 +628,6 @@ static uint32_t mds_mcm_direct_send(NCSM status = NCSCC_RC_FAILURE; break; } - if (status == MDS_INT_RC_DIRECT_SEND_FAIL) { - /* Free the MDS Direct Buff */ - m_MDS_FREE_DIRECT_BUFF(info->info.svc_direct_send.i_direct_buff); - status = NCSCC_RC_FAILURE; - } return status; } @@ -2014,6 +2004,7 @@ static uint32_t mcm_process_await_active return Sucess; */ MDTM_SEND_REQ req; + uint32_t rc = NCSCC_RC_SUCCESS; NCSMDS_CALLBACK_INFO cbinfo; MDS_BCAST_BUFF_LIST *bcast_ptr = NULL; @@ -2022,9 +2013,6 @@ static uint32_t mcm_process_await_active if (svc_cb->i_fail_no_active_sends) { m_MDS_LOG_ERR("MDS_SND_RCV: Returning in noactive state as the option is not to buffer the msgs"); - if (to_msg->msg_type == MSG_DIRECT_BUFF) { - m_MDS_FREE_DIRECT_BUFF(to_msg->data.info.buff); - } return MDS_RC_MSG_NO_BUFFERING; } @@ -2071,7 +2059,8 @@ static uint32_t mcm_process_await_active if (status != NCSCC_RC_SUCCESS) { m_MDS_LOG_ERR("MDS_SND_RCV: CALLBACK ENC failed for svc_id = %s(%d)\n", get_svc_names(svc_cb->svc_id), svc_cb->svc_id); - return NCSCC_RC_FAILURE; + rc = NCSCC_RC_FAILURE; + goto free_mem; } if (((snd_type == MDS_SENDTYPE_BCAST) || (snd_type == MDS_SENDTYPE_RBCAST))) { @@ -2080,7 +2069,8 @@ static uint32_t mcm_process_await_active to_msg->rem_svc_sub_part_ver, cbinfo.info.enc.o_msg_fmt_ver, MDS_SVC_ARCHWORD_TYPE_UNSPECIFIED)) { m_MDS_LOG_ERR("MDS_C_SNDRCV: Addition to bcast list failed in enc case"); - return NCSCC_RC_FAILURE; + rc = NCSCC_RC_FAILURE; + goto free_mem; } } req.msg.encoding = MDS_ENC_TYPE_FULL; @@ -2103,8 +2093,14 @@ static uint32_t mcm_process_await_active m_MDS_LOG_INFO("MDS_SND_RCV: Adding in await active tbl\n"); - return mds_await_active_tbl_add(send_hdl, req); - + rc = mds_await_active_tbl_add(send_hdl, req); + +free_mem: + if (rc != NCSCC_RC_SUCCESS) { + mds_mcm_free_msg_uba_start(req.msg); + } + + return rc; } /*************************************************************************** * @@ -2540,6 +2536,7 @@ static uint32_t mds_await_active_tbl_del bk_ptr->next_msg = mov_ptr->next_msg; } m_MDS_LOG_INFO("MDS_SND_RCV: Await active entry successfully deleted\n"); + mds_mcm_free_msg_uba_start(mov_ptr->req.msg); m_MMGR_FREE_AWAIT_ACTIVE(mov_ptr); return NCSCC_RC_SUCCESS; } @@ -6061,7 +6058,6 @@ static uint32_t mds_mailbox_proc(MDS_MCM m_MMGR_FREE_MSGELEM(msgelem); return status; } - return NCSCC_RC_FAILURE; } /*************************************************************************** * ------------------------------------------------------------------------------ Announcing the Oxford Dictionaries API! The API offers world-renowned dictionary content that is easy and intuitive to access. Sign up for an account today to start using our lexical data to power your apps and projects. Get started today and enter our developer competition. http://sdm.link/oxford _______________________________________________ Opensaf-devel mailing list Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
