Hi Zoran,

yes you are right, hm, I didn't check the whole function... /BR Hans

On 04/04/2018 09:57 AM, Zoran Milinkovic wrote:
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.


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

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




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 

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

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 ");
       } 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) {
            errStr->name.buf = fmtError;
            errStr->name.size = len;

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().

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
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*
                                     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
ccb, const char* errorString,
    +  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.


      va_copy(args, vl);
      len = vsnprintf(fmtError, errLen, errorString, 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

Reply via email to