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