Hi Hans, Anders, Please see my responses inline, with [Vu].
P.s: Please ignore previous email. I pressed wrong keys... Regards, Vu > -----Original Message----- > From: Anders Widell [mailto:anders.wid...@ericsson.com] > Sent: Tuesday, April 3, 2018 7:07 PM > To: Hans Nordebäck <hans.nordeb...@ericsson.com>; Vu Minh Nguyen > <vu.m.ngu...@dektech.com.au>; ravisekhar.ko...@oracle.com; > zoran.milinko...@ericsson.com; lennart.l...@ericsson.com > Cc: opensaf-devel@lists.sourceforge.net > Subject: Re: [PATCH 1/1] imm: fix memory leaked in immnd [#2825] > > Ack with comments. There is actually a second memory leak further down > in this function: > > char* newFmtError = (char*)realloc(fmtError, len); > if (newFmtError == nullptr) { > TRACE_5("realloc error ,No memory "); > return; > } else { > > When realloc returns nullptr, the original memory is left untouched (not > deallocated). Thus, you need a free(fmtError) before return in the code > above. [Vu] Thanks. I will fix this. > > I agree with Hans that it would be better to use some RAII construction > instead, so that you don't need to free() before each return - it is > easy to forget. Maybe simply use std::string and resize() it to emulate > malloc/realloc? You don't have to do it now but think about it as an > improvement. [Vu] The ownership of the allocated memory later on is moved to the `global` variable `ccb`. You can see it at following code lines: if (strstr(errStr->name.buf, IMM_RESOURCE_ABORT) == errStr->name.buf) { free(errStr->name.buf); errStr->name.buf = fmtError; errStr->name.size = len; return; } Or in other case: else { (*errStrTail) = (ImmsvAttrNameList*)malloc(sizeof(ImmsvAttrNameList)); (*errStrTail)->next = NULL; (*errStrTail)->name.size = len; (*errStrTail)->name.buf = fmtError; } As IMMND is mixing C/C++ code, the `CccbInfo ccb` can be used in C code and deallocate memory using free(), therefore I keep using malloc() to avoid mix using new/free() or malloc()/delete(). > > regards, > Anders Widell > > On 04/03/2018 01:42 PM, Hans Nordebäck wrote: > > Hi Vu, > > > > few minor comments below. > > > > /Thanks HansN > > > > > > On 04/03/2018 11:43 AM, Vu Minh Nguyen wrote: > >> The allocated memory is not freed before returning from the function > >> ImmModel::setCcbErrorString(). > >> --- > >> src/imm/immnd/ImmModel.cc | 4 +++- > >> 1 file changed, 3 insertions(+), 1 deletion(-) > >> > >> diff --git a/src/imm/immnd/ImmModel.cc > b/src/imm/immnd/ImmModel.cc > >> index f7c8fc0..e01ff8c 100644 > >> --- a/src/imm/immnd/ImmModel.cc > >> +++ b/src/imm/immnd/ImmModel.cc > >> @@ -10910,7 +10910,6 @@ SaAisErrorT > >> ImmModel::deleteObject(ObjectMap::iterator& oi, SaUint32T reqConn, > >> void ImmModel::setCcbErrorString(CcbInfo* ccb, const char* > >> errorString, > >> va_list vl) { > >> int errLen = strlen(errorString) + 1; > >> - char* fmtError = (char*)malloc(errLen); > >> int len; > >> va_list args; > >> int isValidationErrString = 0; > >> @@ -10921,6 +10920,9 @@ void ImmModel::setCcbErrorString(CcbInfo* > >> ccb, const char* errorString, > >> return; > >> } > >> + char* fmtError = (char*)malloc(errLen); > >> + osafassert(fmtError); > > [HansN] in c++ new should be used instead of malloc. There is no need > > to check return value of new if "std::set_new_handler(new_handler)" > > has been called in advance, e.g. in the main function. (also fmtError > > is a local variable, it should be possible to use RAII and avoid > > explicit calls to delete). [Vu] Please see my responses previously and let me know your opinion. Thanks. /Vu > >> + > >> va_copy(args, vl); > >> len = vsnprintf(fmtError, errLen, errorString, args); > >> va_end(args); > > > ------------------------------------------------------------------------------ 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 Opensaf-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/opensaf-devel
