Hi Hans,

Variable arguments with vsnprintf will work.
The size of the new buffer is resized in
if (len > errLen) {
   ...
   osafassert(vsnprintf(fmtError, len, errorString, vl) >= 0);
}
when there are variable arguments.

BR,
Zoran

-----Original Message-----
From: Hans Nordebäck 
Sent: den 4 april 2018 08:25
To: Vu Minh Nguyen <vu.m.ngu...@dektech.com.au>; Anders Widell 
<anders.wid...@ericsson.com>; ravisekhar.ko...@oracle.com; Zoran Milinkovic 
<zoran.milinko...@ericsson.com>; Lennart Lund <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.

/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);
>

------------------------------------------------------------------------------
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

Reply via email to