Memory Leaks - Can you help me find them in ths snippet

[nmehring]

I am writing some c++ code that interacts with a native C library and
have to do some dynamic memory allocation to support it. I am getting
memory leaks and I think this piece of code is the culprit. Can
anyone tell me what I might be doing wrong?

char **columns;
columns = (CHAR **)malloc (lColumnCount * sizeof (CHAR *));

if (NULL == columns) return FALSE; //memory allocation failed

//allocate space for the column names in the char**
int column_index;
for (column_index = 0; column_index < lColumnCount; column_index++)
{
// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

if (NULL == columns[column_index])
{
delete[] columns; //memory allocation failed
return FALSE;
}
}

//put the data in the char**
for (column_index = 0; column_index < lColumnCount; column_index++)
{
strcpy (columns[column_index], CStringColumnName[column_index]);
}

//do some stuff with columns
........

delete[] columns;

Nicole

[Victor Bazarov]

nm******@gmail.com wrote:

I am writing some c++ code that interacts with a native C library and
have to do some dynamic memory allocation to support it. I am getting
memory leaks and I think this piece of code is the culprit. Can
anyone tell me what I might be doing wrong?

char **columns;
columns = (CHAR **)malloc (lColumnCount * sizeof (CHAR *));
[..]

delete[] columns;

Don't EVER mix 'malloc/free' and 'new/delete' (or 'new[]/delete[]').
Those come in pairs, use them in pairs.

V
--
Please remove capital 'A's when replying by e-mail
I do not respond to top-posted replies, please don't ask

[nmehring]

Thanks, I was wondering if that was a problem. I went ahead and made
the change, but still have memory leaks. Anyone have any ideas for
me? I know it hits the "free(columns)" at the end of the code
snippet.

char **columns;
columns = (CHAR **)malloc (lColumnCount * sizeof (CHAR *));

if (NULL == columns) return FALSE; //memory allocation failed

//allocate space for the column names in the char**
int column_index;
for (column_index = 0; column_index < lColumnCount; column_index++)
{
// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

if (NULL == columns[column_index])
{
free(columns); //memory allocation failed
return FALSE;
}
}
//put the data in the char**
for (column_index = 0; column_index < lColumnCount; column_index++)
{
strcpy (columns[column_index], CStringColumnName[column_index]);
}
//do some stuff with columns
........

free(columns);

[Pete Becker]

On 2008-01-28 12:35:34 -0500, "nm******@gmail.com" <nm******@gmail.comsaid:

// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

The memory allocated here never gets freed.

--
Pete
Roundhouse Consulting, Ltd. (www.versatilecoding.com) Author of "The
Standard C++ Library Extensions: a Tutorial and Reference
(www.petebecker.com/tr1book)

[Jim Langston]

nm******@gmail.com wrote:

Thanks, I was wondering if that was a problem. I went ahead and made
the change, but still have memory leaks. Anyone have any ideas for
me? I know it hits the "free(columns)" at the end of the code
snippet.

count the malloc/news you make and the free/deletes.

char **columns;
columns = (CHAR **)malloc (lColumnCount * sizeof (CHAR *));

Here's 1 malloc

if (NULL == columns) return FALSE; //memory allocation failed

//allocate space for the column names in the char**
int column_index;
for (column_index = 0; column_index < lColumnCount; column_index++)
{
// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

Here's another malloc that's going to happen lColumnCount times.

if (NULL == columns[column_index])
{
free(columns); //memory allocation failed

This one is only on case of error, lets ignore this for now

return FALSE;
}
}
//put the data in the char**
for (column_index = 0; column_index < lColumnCount; column_index++)
{
strcpy (columns[column_index], CStringColumnName[column_index]);
}
//do some stuff with columns
.......

free(columns);

And here is one free. What about the lColumnCount frees you need to do?
You need a free/delete for every malloc/new you do. Before you do this
free(columns) you need to:

for (column_index = 0; column_index < lColumnCount; column_index++)
{
free( columns[column_index] );
}

--
Jim Langston
ta*******@rocketmail.com

[nmehring]

Thanks everyone, that solved the problem. Here is the corrected code:

char **columns;
columns = (CHAR **)malloc (lColumnCount * sizeof (CHAR *));

if (NULL == columns) return FALSE; //memory allocation failed

//allocate space for the column names in the char**
int column_index;
for (column_index = 0; column_index < lColumnCount; column_index++)
{
// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

if (NULL == columns[column_index])
{
free(columns); //memory allocation failed
return FALSE;
}
}

//put the data in the char**
for (column_index = 0; column_index < lColumnCount; column_index++)
{
strcpy (columns[column_index], CStringColumnName[column_index]);
}
//do some stuff with columns
........

for( int i = 0; i < lColumnCount; ++i )
{
free(columns[i]);
}
free(columns);

[Default User]

nm******@gmail.com wrote:

Thanks everyone, that solved the problem. Here is the corrected code:

for (column_index = 0; column_index < lColumnCount; column_index++)
{
// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

if (NULL == columns[column_index])
{
free(columns); //memory allocation failed
return FALSE;
}
}

What happens if the allocation failure occurs when i != 0?


Brian

[Daniel T.]

"nm******@gmail.com" <nm******@gmail.comwrote:

I am writing some c++ code that interacts with a native C library and
have to do some dynamic memory allocation to support it. I am getting
memory leaks and I think this piece of code is the culprit. Can
anyone tell me what I might be doing wrong?

Yes. Remove the mallocs and deletes. Use std::vectors instead.

vector< vector< char columns( lColumnCount,
SE_QUALIFIED_COLUMN_LEN );
//put the data in the char**
for ( int column_index = 0; column_index < lColumnCount;
column_index++)
{
strcpy (&columns[column_index].front(),
CStringColumnName[column_index]);
}
//do some stuff with columns
//.......

Or maybe better:

vector< string columns( lColumnCount );
//put the data in the char**
for ( int column_index = 0; column_index < lColumnCount;
column_index++)
{
columns[column_index] = CStringColumnName[column_index];
}

//do some stuff with columns
//.......

Or maybe better still:

vector< string columns;

copy( CStringColumnName, CStringColumnName + lColumnCount,
back_inserter( columns ) );

//do some stuff with columns
//.......

[Daniel T.]

"nm******@gmail.com" <nm******@gmail.comwrote:

Thanks everyone, that solved the problem. Here is the corrected code:

The code is better, but still not correct. I say again, save yourself
the headache and use vectors and/or strings. Both vectors and strings
can be used with C libraries and you don't have to worry about memory
leaks when you use them.

vector< string columns;
copy( CStringColumnName, CStringColumnName + lColumnCount,
back_inserter( columns ) );

//do some stuff with columns
//.......

char **columns;
columns = (CHAR **)malloc (lColumnCount * sizeof (CHAR *));

if (NULL == columns) return FALSE; //memory allocation failed

//allocate space for the column names in the char**
int column_index;
for (column_index = 0; column_index < lColumnCount; column_index++)
{
// Allocate for the maximum owner.table.column notation
columns[column_index] = (char *)malloc(SE_QUALIFIED_COLUMN_LEN);

if (NULL == columns[column_index])
{
free(columns); //memory allocation failed
return FALSE;
}
}

//put the data in the char**
for (column_index = 0; column_index < lColumnCount; column_index++)
{
strcpy (columns[column_index], CStringColumnName[column_index]);
}
//do some stuff with columns
.......

for( int i = 0; i < lColumnCount; ++i )
{
free(columns[i]);
}
free(columns);

[nmehring]

Thank you, I will correct that additional leak if i>0

The reason I am using these datatypes is because I am utilizing this
3rd party library method:

extern LONG SDEAPI SE_stream_update_row (SE_STREAM stream,
const ACHAR *table,
LONG *sde_row_id,
SHORT num_columns,
const ACHAR **columns);

So I figured I had to use the char data type.

Nicole

[Daniel T.]

"nm******@gmail.com" <nm******@gmail.comwrote:

Thank you, I will correct that additional leak if i>0

The reason I am using these datatypes is because I am utilizing this
3rd party library method:

extern LONG SDEAPI SE_stream_update_row (SE_STREAM stream,
const ACHAR *table,
LONG *sde_row_id,
SHORT num_columns,
const ACHAR **columns);

So I figured I had to use the char data type.

Sorry, I thought you were using each column individually. i.e., I
thought the API was asking for char* and was being called multiple
times, not char** and called once.

However, I still recommend you use vectors rather than allocating the
memory yourself and hoping for the best.

Something like this would do nicely:

vector< vector< char
block( lColumnCount, SE_QUALIFIED_COLUMN_LEN );

for ( int i = 0; i < block.size(); ++i )
{
strcpy( &block[i].front(), CStringColumnName[i] );
}

vector< char* columns( block.size() );
for ( int i = 0; i != block.size(); ++i )
{
columns[i] = &block[i].front();
}
SE_stream_update_row( /* other params */, &columns[0] );