delete [] faults in destructor..

[Vishu]

Hi ,

I am having a problem with a very naieve looking code. This code runs
well on Sun Sparc, 5.9(compiled with g++); but faults at the line
"delete [] name" on Windows (I tried both VC 6.0 and .net 2003
compilers). The code is so basic, I dont suspect any platform specific
issue; which is why I am posting it here.. Can anyone shed some light??
///////////////////////////////////////////////////////////////////////////////////////////////////////
#include <iostream>

using namespace::std;
using std::cout;
using std::endl;
class abcd
{
public:
abcd(char *);
~abcd();
void print();
private:
char *name;
};
abcd::abcd(char * nameVal):name(NULL)
{
if(nameVal)
{
name = new char[strlen(nameVal)];
if(name)
{
memset(name,0,strlen(nameVal));//no need of
this..but still..
// memcpy(name,nameVal,strlen(nameVal));
strcpy(name,nameVal);
}
}
}
void abcd:rint()
{
if(name)
{
cout<<"Value is " << name <<endl;
}
}
abcd::~abcd()
{
if(name)
{
// free(name);
delete [] name;
cout << "Deleted successfully" <<endl;
name = NULL;
}
}

int main()
{
abcd * dude = new abcd("whatthehell");
dude->print();
delete dude;dude = NULL;
return 0;
}
///////////////////////////////////////////////////////////////////////

 

[Alf P. Steinbach]

* Vishu:

I am having a problem with a very naieve looking code. This code runs
well on Sun Sparc, 5.9(compiled with g++); but faults at the line
"delete [] name" on Windows (I tried both VC 6.0 and .net 2003
compilers). The code is so basic, I dont suspect any platform specific
issue; which is why I am posting it here.. Can anyone shed some light??
///////////////////////////////////////////////////////////////////////////////////////////////////////
#include <iostream>

using namespace::std;
using std::cout;
using std::endl;
class abcd
{
public:
abcd(char *);
~abcd();

Should have a copy constructor.

void print();
private:
char *name;
};
abcd::abcd(char * nameVal):name(NULL)
{
if(nameVal)
{
name = new char[strlen(nameVal)];
if(name)

This test is only in support of pre-standard compilers. With standard
compilers 'new' will throw a std::bad_alloc exception if it fails. So
with a standard compiler the test will only be executed when name != 0.

{
memset(name,0,strlen(nameVal));//no need of
this..but still..
// memcpy(name,nameVal,strlen(nameVal));
strcpy(name,nameVal);

Copying n+1 characters into an n-character array; Undefined Behavior.

}
}
}
void abcd:rint()
{
if(name)
{
cout<<"Value is " << name <<endl;
}
}
abcd::~abcd()
{
if(name)
{
// free(name);
delete [] name;
cout << "Deleted successfully" <<endl;
name = NULL;
}
}

int main()
{
abcd * dude = new abcd("whatthehell");
dude->print();
delete dude;dude = NULL;
return 0;
}
///////////////////////////////////////////////////////////////////////

The most practical solution to your problem is to use std::string
instead of doing that low-level dangerous pointer stuff.

 

[Phlip]

abcd(char *);

Always learn to use std::string _before_ you learn to use raw character
pointers. It will take care of all these details for you.

name = new char[strlen(nameVal)];

you need strlen(nameVal) + 1, to leave room for the '\0' on the end.

if(name)

'new' generally throws an exception if it fails, so you never need this kind
of check.

strcpy(name,nameVal);

And this wrote right up to strlen(nameVal), and then wrote a '\0' off the
end of the array. That caused "undefined behavior", which means anything
could happen. The program could appear to work correctly, or the nearest
toilet could explode.

In this case, the extra '\0' corrupted the heap, and delete[] crashed when
it tried to change that heap.

 

[Amal P]

Hi,
The problem is not platform specific. Your code is only having a
simple problem. You allocates the buffer for size 11 and then copies 12
bytes to it. When you put string data in the buffer the end of the
string is represented by a NULL character. So when you try to do
strcpy(name,nameVal); it copies the 12 bytes actually("whatthehell\0").
So the data gets copied to non allocated place in the memory. So the
delete is unable to delete the pointer. The only change that you have
to make in the progam is that,

name = new char[strlen(nameVal)]; --> name = new char[strlen(nameVal)
+ 1];
Then your code will work fine...
Now your new code will be,
// Code snippet
// DeleteProb.cpp : Defines the entry point for the console
application.
//

#include "stdafx.h"

#include <iostream>

using namespace::std;
using std::cout;
using std::endl;
class abcd
{
public:
abcd(char *);
~abcd();
void print();
private:
char *name;
};

abcd::abcd(char * nameVal):name(NULL)
{
if(nameVal)
{
int nSize = strlen(nameVal);
name = new char[strlen(nameVal)+1];
if(name)
{
// memset(name,0,strlen(nameVal));//no need of
this..but still..
// memcpy(name,nameVal,strlen(nameVal));
strcpy(name,nameVal);
}
}
}

void abcd:rint()
{
if(name)
{
cout<<"Value is " << name <<endl;
}
}

abcd::~abcd()
{
if(name)
{
// free(name);
delete[] name;
cout << "Deleted successfully" <<endl;
name = NULL;
}

}

int main(int argc, char* argv[])
{
abcd * dude = new abcd("whatthehell"); //Nothing in C/C++ is
not a hell...
dude->print();
delete dude;dude = NULL;
return 0;
}

Best regards,
Amal P

 

[Amal P]

Hi,
The problem is not platform specific. Your code is only having a
simple problem. You allocates the buffer for size 11 and then copies 12
bytes to it. When you put string data in the buffer the end of the
string is represented by a NULL character. So when you try to do
strcpy(name,nameVal); it copies the 12 bytes actually("whatthehell\0").
So the data gets copied to non allocated place in the memory. So the
delete is unable to delete the pointer. The only change that you have
to make in the progam is that,

name = new char[strlen(nameVal)]; --> name = new char[strlen(nameVal)
+ 1];
Then your code will work fine...
Now your new code will be,
// Code snippet
// DeleteProb.cpp : Defines the entry point for the console
application.
//

#include "stdafx.h"

#include <iostream>

using namespace::std;
using std::cout;
using std::endl;
class abcd
{
public:
abcd(char *);
~abcd();
void print();
private:
char *name;
};

abcd::abcd(char * nameVal):name(NULL)
{
if(nameVal)
{
int nSize = strlen(nameVal);
name = new char[strlen(nameVal)+1];
if(name)
{
// memset(name,0,strlen(nameVal));//no need of
this..but still..
// memcpy(name,nameVal,strlen(nameVal));
strcpy(name,nameVal);
}
}
}

void abcd:rint()
{
if(name)
{
cout<<"Value is " << name <<endl;
}
}

abcd::~abcd()
{
if(name)
{
// free(name);
delete[] name;
cout << "Deleted successfully" <<endl;
name = NULL;
}

}

int main(int argc, char* argv[])
{
abcd * dude = new abcd("whatthehell"); //Nothing in C/C++ is a
hell...
dude->print();
delete dude;dude = NULL;
return 0;
}

Best regards,
Amal P

 

[LuB]

Vishu wrote:
....

name = new char[strlen(nameVal)];
if(name)
{
memset(name,0,strlen(nameVal));//no need of
this..but still..
// memcpy(name,nameVal,strlen(nameVal));
strcpy(name,nameVal);
}

If you're going to use these low-level char* methods, its a good habit
to use counted versions when available.

ie: strncpy(dst, src, len) ... strncat, etc ...

Remember, len must be long enough to include the terminating null
character. Generally, size_t allocSize = ::strlen(nameVal) + 1;

Hth,

/Luther

 

[Vishu]

Thanks folks. I learnt the lesson hard way, will convert to std::string
!!.
I am a victim of sophisticated debugging environment, when I saw the
content of 'name' in the watchwindow, it thought everything is fine..
"\0", completely fel off my mind..

Thanks !

Vishu.

 

[Richard Herring]

Thanks folks. I learnt the lesson hard way, will convert to std::string
!!.
I am a victim of sophisticated debugging environment, when I saw the
content of 'name' in the watchwindow, it thought everything is fine..
"\0", completely fel off my mind..

While you're at it, look up "rule of 3". Your original class was doomed
to produce even more undefined behaviour the first time you tried to
copy it.

 

[Old Wolf]

If you're going to use these low-level char* methods, its a good habit
to use counted versions when available.

ie: strncpy(dst, src, len) ... strncat, etc ...

strncpy does not 0-terminate dst when it runs out of space, so it is
usually not a good idea to use this function.