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]

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

 

[Alf P. Steinbach]

* (e-mail address removed):

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;

Using malloc for allocation and delete[] for deallocation is Undefined
Behavior.

Code below would be better expressed using ScopeGuard.

But in order not to bring in that library (disclaimer: code not touched
by compiler's hands):

class Columns
{
private:
std::vector<char*> myColumns;

void dealloc()
{
for( size_t i = 0; i < myColumns.size(); ++i )
{
delete[] myColumns;
}
}

public:
Columns(): myColumns( ::lColumnCount )
{
try
{
for( int i = 0; i < lColumnCount; ++i )
{
myColumns = new char[SE_QUALIFIED_COLUMN_LEN];
strcpy( myColumns, ::CStringColumnsName );
}
}
catch( std::bad_alloc const& )
{
dealloc();
throw;
}
}

~Columns() { dealloc(); }

char** pFirst() { return &myColumns[0]; }
};


bool foo()
{
try
{

Columns columns;
someApiFunc( columns.pFirst() );
return true;
}
catch( ... )
{
return false;
}
}

foo() could be simpler if it signalled failure via exception rather than
via a bool result.


Cheers & hth.,

- Alf

 

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

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

The memory allocated here never gets freed.

 

[Alf P. Steinbach]

* (e-mail address removed):

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?

Perhaps your news-server hasn't propagated all replies in the thread.

I'm sure you'll find them in Google Groups, though.


Cheers & hth.,

- Alf

 

[Jim Langston]

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

 

[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);
}
free(columns);

 

[Alf P. Steinbach]

* (e-mail address removed):

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;

In this case you have a memory leak.

}
}

//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);
}
free(columns);

 

[Default User]

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

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

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

 

[Alf P. Steinbach]

* (e-mail address removed):

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.

You will have to supply an array of char pointers.

A std::vector<std::string>, as suggested in the article you're replying,
is not that.

 

[Daniel T.]

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.front(), CStringColumnName );
}

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

 

[Alf P. Steinbach]

* Daniel T.:

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.front(), CStringColumnName );
}

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

This is a better idea than the code I posted: it uses a little more
memory but is safer (wrt. maintainance) and shorter and just more clear.

However, the 'block' constructor arguments need to be fixed, and the
whole thing needs to be put in a try-catch in order to conform to the
OP's boolean return.

I'd just default-initialize the inner vectors, and .resize() them in the
string copy loop.


Cheers,

- Alf

 

[Daniel T.]

* Daniel T.:

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.front(), CStringColumnName );
}

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

This is a better idea than the code I posted: it uses a little more
memory but is safer (wrt. maintainance) and shorter and just more clear.

However, the 'block' constructor arguments need to be fixed, and the
whole thing needs to be put in a try-catch in order to conform to the
OP's boolean return.

Accepted about the try catch block, but what is wrong with the
constructor arguments, did I reverse them or something?

I'd just default-initialize the inner vectors, and .resize() them in the
string copy loop.

Another idea would be to allocate the block as a single vector<char>
rather than a vector of vectors, then partition the block into the
columns vector, like so:

vector< char > block( lColumnCount * SE_QUALIFIED_COLUMN_LEN );
vector< char* > columns( lColumnCount );
for ( int i = 0, j = 0; i < lColumnCount;
++i, j += SE_QUALIFIED_COLUMN_LEN ) {
columns = &block[j];
}
// and so on.

 

[Alf P. Steinbach]

* Daniel T.:

* Daniel T.:

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.front(), CStringColumnName );
}

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

This is a better idea than the code I posted: it uses a little more
memory but is safer (wrt. maintainance) and shorter and just more clear.

However, the 'block' constructor arguments need to be fixed, and the
whole thing needs to be put in a try-catch in order to conform to the
OP's boolean return.

Accepted about the try catch block, but what is wrong with the
constructor arguments, did I reverse them or something?

Well, I was wrong, but on a higher level (see below) I was right.

What I remembered was my practical experience with one particular
compiler, which would not accept the above, while the standard does
require that it is accepted, via a very very subtle special case rule.

Consider

#include <vector>
#include <iostream>

int main()
{
using namespace std;
vector< vector<int> > v( 3, 7 ); // The item discussed here.
cout << v.size() << ", " << v[0].size() << endl;
}

Trying to compile, various compilers:


<g++>
V:\> gnuc vc_project.cpp

V:\> a
3, 7
</g++>


<msvc 7.1>
V:\> msvc vc_project.cpp -o b.exe
vc_project.cpp
C:\Program Files\Microsoft Visual Studio .NET
2003\Vc7\include\vector(357) : error C2664: 'std::vector<_Ty>::_Construct_
n' : cannot convert parameter 2 from 'int' to 'const std::vector<_Ty> &'
with
[
_Ty=std::vector<int>
]
and
[
_Ty=int
]
Reason: cannot convert from 'int' to 'const std::vector<_Ty>'
with
[
_Ty=int
]
Constructor for class 'std::vector<_Ty>' is declared 'explicit'
with
[
_Ty=int
]
C:\Program Files\Microsoft Visual Studio .NET
2003\Vc7\include\vector(343) : see reference to function template
instantiation 'void
std::vector<_Ty>::_Construct<_Iter>(_Iter,_Iter,std::_Int_iterator_tag)'
being compiled
with
[
_Ty=std::vector<int>,
_Iter=int
]
vc_project.cpp(7) : see reference to function template
instantiation 'std::vector<_Ty>::vector<int>(_Iter,_Iter)
' being compiled
with
[
_Ty=std::vector<int>,
_Iter=int
]

V:\>_
<msvc 7.1>


<comeau online>
Your Comeau C/C++ test results are as follows:

Comeau C/C++ 4.3.9 (Mar 27 2007 17:24:47) for ONLINE_EVALUATION_BETA1
Copyright 1988-2007 Comeau Computing. All rights reserved.
MODE:strict errors C++ C++0x_extensions


In strict mode, with -tused, Compile succeeded (but remember, the Comeau
online compiler does not link).
Compiled with C++0x extensions enabled.
</comeau online>


So which compiler is right? Comeau usually is, and is also this time.

What happens is that instead of the explicit constructor taking size
argument, the templated constructor taking iterators is invoked for the
outer vector, and §23.1.1/9, general requirements for containers, says
that when invoked with integral argument type instead of the expected
some iterator type, it shall behave the same as (in this case) vector<
vector<int> >( static_cast<size_type>(3), static_cast< vector<int> >( 7
) ), or in other words effectively the same as

vector< vector<int> > v( 3, vector<int>( 7 ) );

But I wouldn't rely on the compiler / library implementation being smart
enough to figure that out.

I'd write it as what the standard requires it ends up as, namely this
latter declaration, because then everybody understand what it means,
including the author, the MSVC 7.1 compiler, and maintenance...

Cheers,

- Alf

 

[James Kanze]

* Daniel T.:

[...]
[...]

Well, I was wrong, but on a higher level (see below) I was right.

What I remembered was my practical experience with one
particular compiler, which would not accept the above, while
the standard does require that it is accepted, via a very very
subtle special case rule.

Note that there has been some discussion as to whether the fact
that the above is legal is due to an error in the wording of the
special case. The special case was designed to handle things
like:

std::vector< int > v( 10, 43 ) ;

Note that the best match for this constructor is the template
constructor taking (nominally) two iterators, which is probably
not what the user expected. (There is also a constructor taking
a size_t and a T---in this case an int. But of course, that
involves a conversion of int to size_t, where as the template
constructor taking two "iterators" is an exact match---the
template mechanism can't tell that int's can't be iterators.)

The result is that a special case was added to make this
constructor act as expected. I don't think that the original
intent of the wording was to also make Daniel's constructor well
formed (and in fact, as you point out, some compilers don't
accept it).

I believe that there was even a DR, although I'm not sure. At
any rate, I just checked the current draft: the wording of the
special case has been changed to make Daniel's version illegal
(so you're not really wrong, just living in the future.

Consider

#include <vector>
#include <iostream>

int main()
{
using namespace std;
vector< vector<int> > v( 3, 7 ); // The item discussed here.
cout << v.size() << ", " << v[0].size() << endl;
}

Trying to compile, various compilers:

It's a library issue, more than a compiler issue. If the two
arguments have the same type, and the first is not size_t, then
the template function taking two iterators is a better match.
Beyond that, it's up to the library to make it work "as if" the
(size_t, T) constructor had been called, with the arguments
"static_cast" to the correct type. (The latest draft changes
this, and says that only the first argument should be
static_cast to a size_type.) The 1998 version of the standard
also contains a note (non-normative) suggesting a simple
technique to implement this---a library which uses that
technique, however, will fail to compile Daniel's code.

My guess is that for the compilers where this code failed to
compile, the library implementation is based on that note.

So which compiler is right? Comeau usually is, and is also
this time.

For the moment. (Also, just curious, but does anyone know
what implementation of the library the Comeau on line uses?)

What happens is that instead of the explicit constructor taking size
argument, the templated constructor taking iterators is invoked for the
outer vector, and §23.1.1/9, general requirements for containers, says
that when invoked with integral argument type instead of the expected
some iterator type, it shall behave the same as (in this case) vector<
vector<int> >( static_cast<size_type>(3), static_cast< vector<int> >( 7
) ), or in other words effectively the same as

vector< vector<int> > v( 3, vector<int>( 7 ) );

But I wouldn't rely on the compiler / library implementation
being smart enough to figure that out.

I'd write it as what the standard requires it ends up as,
namely this latter declaration, because then everybody
understand what it means, including the author, the MSVC 7.1
compiler, and maintenance...

And libraries which will implement C++0x.

Totally agreed (and I would be even if the standards committee
had decided to live with the original text, on the grounds that
even if it wasn't what was meant, changing it now would break
too much codde).

 

[Alf P. Steinbach]

* James Kanze -> Alf P. Steinbach:

I believe that there was even a DR, although I'm not sure. At
any rate, I just checked the current draft: the wording of the
special case has been changed to make Daniel's version illegal
(so you're not really wrong, just living in the future.

Heh.


Cheers,

- Alf