Hi Hans, Now, I got your idea of using RAII for ` fmtError`. Thanks for your comment. 😊
See my responses inline for your comment regarding variable arguments. Regards, Vu > -----Original Message----- > From: Hans Nordebäck [mailto:hans.nordeb...@ericsson.com] > Sent: Wednesday, April 4, 2018 1:25 PM > To: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>; 'Anders Widell' > <anders.wid...@ericsson.com>; 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] > > Hi Vu, > > not saying that you should change now, but an alternative can be to: > > instead of: > > char* fmtError = (char*) malloc(errLen); > osafassert(fmtError); > > a std::vector can be used, (or a std::array if fixed size): > > std::vector<char> fmtError(errLen, 0); > > : > > len = vsnprintf(fmtError.data(), errLen, errorString, args); > > : > > fmtError.resize(len); > > : > > errStr->name.buf = strdup(fmtError.data(); > > : > > Another thing I noticed in the beginning of this function: > > void ImmModel::setCcbErrorString(CcbInfo* ccb, const char* errorString, > va_list vl) { > int errLen = strlen(errorString) + 1; > > does not include the length of the variable arguments, vsnprintf will > work but the > > resulting string may be cut. [Vu] IMM seems already dealt with this case - the output was truncated, I think. It checked the returned value of 1st call ` vsnprintf`, then extending the destination buffer. But seems lacking `va_copy` and `va_end` in the second round. if (len > errLen) { char* newFmtError = (char*)realloc(fmtError, len); fmtError = newFmtError; * osafassert(vsnprintf(fmtError, len, errorString, vl) >= 0); * } Should be: va_copy(args, vl); osafassert(vsnprintf(fmtError, len, errorString, vl) >= 0); va_end(args); Please see the attached patch for the diff of this ticket - V2. /Vu > > /Regards HansN > > > On 04/04/2018 05:02 AM, Vu Minh Nguyen wrote: > > 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); > >
ticket-2825.diff
Description: Binary data
------------------------------------------------------------------------------ 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
