strcpy and the copy constructor

[RonHiler]

My copy constructor is crashing my program, and I can't figure out
why. I'll try to make the code listing as short as I can. Here are
the two headers:

class BreakthroughClass
{
public:
BreakthroughClass();
virtual ~BreakthroughClass();
BreakthroughClass(const BreakthroughClass &source);
BreakthroughClass& operator=(const BreakthroughClass &source);

protected:
char Name[256];
char Description[1024];
int LocationX;
int LocationY;
vector <unsigned int> Prereqs;

friend class TechManagerClass;
};

class TechManagerClass
{
public:
TechManagerClass();
virtual ~TechManagerClass();

void InitializeTechs();

private:
vector <BreakthroughClass> Breakthroughs;

void BreakthroughLoadTitles(char *RCLabel);
void BreakthroughLoadData(char *RCLabel);

//single instance, do not implement these functions
TechManagerClass(const TechManagerClass &source);
TechManagerClass& operator=(const TechManagerClass &source);
};

TechManagerClass TechManager;

So Breakthroughs are a vector of BreakthroughClass contained in the
TechManagerClass, and TechManager is a global instance of
TechManagerClass.

When I call TechManager.InitializeTechs, I do this:

void TechManagerClass::InitializeTechs()
{
BreakthroughLoadTitles("SCIENCE");
BreakthroughLoadTitles("GOVERNMENT");
//and so on
}

Then:

void TechManagerClass::BreakthroughLoadTitles(char *RCLabel)
{
int i;
char FullLabel[256];
HRSRC ResourceLocation;
HANDLE Resource;
LPVOID ResourcePointer;
char* pChar; //temporary tranversing variables
BreakthroughClass NewBreakthrough;
char OutputString[1024];

i = 0;
while (1)
{
sprintf (FullLabel, "%s_%d", RCLabel, i);
sprintf(OutputString, "Loading %s Technology Breakthrough",
FullLabel);
OutputDebugString(OutputString);
ResourceLocation = FindResourceEx (NULL, RT_RCDATA, FullLabel,
MAKELANGID(LANG_NEUTRAL, SUBLANG_NEUTRAL));
if (ResourceLocation == NULL)
break;
Resource = LoadResource (NULL, ResourceLocation);
ResourcePointer = LockResource(Resource);

pChar = (char*)ResourcePointer;
strcpy(NewBreakthrough.Name, pChar);
OutputDebugString(NewBreakthrough.Name);
Breakthroughs.push_back(NewBreakthrough);
i++;
}
sprintf(OutputString, "Loaded %i %s Technology Breakthroughs", i,
RCLabel);
OutputDebugString(OutputString);
}

So I'm finding the entry in my resource file (which are named
SCIENCE_0, SCIENCE_1, GOVERNMENT_0, GOVERNMENT_1, and so on), copying
the name into my temporary NewBreakthrough variable, then doing a
push_back into Breakthroughs. This calls my copy constructor, and
here's where I have problems. I took everything out (for now) except
the one line, and this is the line causing the crash.

BreakthroughClass::BreakthroughClass(const BreakthroughClass &source)
{
strcpy (Name, source.Name);
}

I've done OutputDebugString on the Name entries right into the copy
constructor, and it seems to be coming out fine (I get the name of the
techs), so I don't see why the strcpy is crashing. What am I doing
wrong?

Thanks,

Ron

 

[Victor Bazarov]

My copy constructor is crashing my program, and I can't figure out
why. [...]

BreakthroughClass::BreakthroughClass(const BreakthroughClass &source)
{
strcpy (Name, source.Name);

What's 'Name'? Where does it point to?

}

I've done OutputDebugString on the Name entries right into the copy
constructor, and it seems to be coming out fine (I get the name of the
techs), so I don't see why the strcpy is crashing. What am I doing
wrong?

You're using character pointers when you should use 'std::string'.

V

 

[Howard]

Resource = LoadResource (NULL, ResourceLocation);
ResourcePointer = LockResource(Resource);

pChar = (char*)ResourcePointer;
strcpy(NewBreakthrough.Name, pChar);
OutputDebugString(NewBreakthrough.Name);


BreakthroughClass::BreakthroughClass(const BreakthroughClass &source)
{
strcpy (Name, source.Name);
}

I've done OutputDebugString on the Name entries right into the copy
constructor, and it seems to be coming out fine (I get the name of the
techs), so I don't see why the strcpy is crashing. What am I doing
wrong?

Is it possible that the string array of characters pointed to by pChar above
isn't null-terminated, but has some garbage after it that isn't shown by
your OutputDebugString call? If so, then there's a possibility that strcpy
has trashed memory. Alternatively, have you checked what's in source.Name
inside the copy constructor? Perhaps something is wrong with the string (or
the object itself) at that point?

-Howard

 

[Adrian]

My copy constructor is crashing my program, and I can't figure out
why. I'll try to make the code listing as short as I can. Here are
the two headers:

Since you delcare Name as a char array the default copy constructor will
work fine, it you change name to pointer then all bets are off. Also as
mentioned elsewhere use std::string

#include <iostream>
#include <cstring>

class Test
{
public:
Test(char *fred) { std::strncpy(data, fred, sizeof(data)-1); };
void display() const { std::cout << data << std::endl; };
void change(char *fred) { std::strncpy(data, fred, sizeof(data)-1); };
private:
char data[256];

};

int main()
{
Test t1("string 1");
Test t2("string 2");

t1.display();
t2.display();
t2=t1;

t1.display();
t2.display();

t1.change("new string");
t1.display();
t2.display();

return 0;
}

 

[RonHiler]

Is it possible that the string array of characters pointed to by pChar above
isn't null-terminated, but has some garbage after it that isn't shown by
your OutputDebugString call? If so, then there's a possibility that strcpy
has trashed memory. Alternatively, have you checked what's in source.Name
inside the copy constructor? Perhaps something is wrong with the string (or
the object itself) at that point?

Yeah, I looked at those possibilities, and it would seem to make
sense, but I just don't see how it can be possible. Here is one entry
from my resource file as an example:

SCIENCE_0 RCDATA //BREAKTHROUGH
BEGIN
"Chemistry\0",
"Chemistry breakthrough description to be written...\0",
0, 0, //LOCATION IN TECH TREE
1, //NUMBER OF PREREQS
"Physics\0" //PREREQ LIST
END

Clearly my strings are null terminated. This seems to be born out by
the OutputDebugStrings, which aren't showing any problems. I checked
the strlen(), to see if there were characters not showing up with ODS,
but the ODS output seemed perfectly normal. Here is a snippet of what
I got:

ODS: Loading SCIENCE_0 Technology Breakthrough Process MDestiny.exe
(0xD34)
ODS: Chemistry Process MDestiny.exe (0xD34)
ODS: Chemistry 9 Process MDestiny.exe (0xD34)
ODS: Loading SCIENCE_1 Technology Breakthrough Process MDestiny.exe
(0xD34)
ODS: Physics Process MDestiny.exe (0xD34)
ODS: Physics 7 Process MDestiny.exe (0xD34)
ODS: Loading SCIENCE_2 Technology Breakthrough Process MDestiny.exe
(0xD34)
ODS: Loaded 2 SCIENCE Technology Breakthroughs Process MDestiny.exe
(0xD34)

The first line shows the name of the resource entry being parsed
The second line shows the technology name entry at the
BreakthroughLoadTitles() level (in the temporary NewBreakthrough
variable).
The third line is actually inside the copy constructor, checking
source.Name, with the string and its length being shown.
This repeats for the next resource entry (SCIENCE_1). Then a third
entry is parsed but not found (SCIENCE_2, which causes the break in
the while statement), and the final line lets me know there were 2
total entries parsed for that catagory.

Everything SEEMS to be exactly as it should be, right into the copy
constructor. I'm really pulling my hair out over this one

Ron

 

[John Harrison]

Yeah, I looked at those possibilities, and it would seem to make
sense, but I just don't see how it can be possible. Here is one entry
from my resource file as an example:

SCIENCE_0 RCDATA //BREAKTHROUGH
BEGIN
"Chemistry\0",
"Chemistry breakthrough description to be written...\0",
0, 0, //LOCATION IN TECH TREE
1, //NUMBER OF PREREQS
"Physics\0" //PREREQ LIST
END

Clearly my strings are null terminated. This seems to be born out by
the OutputDebugStrings, which aren't showing any problems. I checked
the strlen(), to see if there were characters not showing up with ODS,
but the ODS output seemed perfectly normal. Here is a snippet of what
I got:

ODS: Loading SCIENCE_0 Technology Breakthrough Process MDestiny.exe
(0xD34)
ODS: Chemistry Process MDestiny.exe (0xD34)
ODS: Chemistry 9 Process MDestiny.exe (0xD34)
ODS: Loading SCIENCE_1 Technology Breakthrough Process MDestiny.exe
(0xD34)
ODS: Physics Process MDestiny.exe (0xD34)
ODS: Physics 7 Process MDestiny.exe (0xD34)
ODS: Loading SCIENCE_2 Technology Breakthrough Process MDestiny.exe
(0xD34)
ODS: Loaded 2 SCIENCE Technology Breakthroughs Process MDestiny.exe
(0xD34)

The first line shows the name of the resource entry being parsed
The second line shows the technology name entry at the
BreakthroughLoadTitles() level (in the temporary NewBreakthrough
variable).
The third line is actually inside the copy constructor, checking
source.Name, with the string and its length being shown.
This repeats for the next resource entry (SCIENCE_1). Then a third
entry is parsed but not found (SCIENCE_2, which causes the break in
the while statement), and the final line lets me know there were 2
total entries parsed for that catagory.

Everything SEEMS to be exactly as it should be, right into the copy
constructor. I'm really pulling my hair out over this one

Ron

Unless I missed it you never said where you are calling InitializeTechs
from. Is it possible that you are running into the so called 'static
initialization order fiasco'. In other words because you are calling
InitializeTechs from the constructor of another global variable you are
inadvertently using the Breakthroughs vector before it has been constructed?

If this sounds at all likely check out the FAQ

http://www.parashift.com/c++-faq-lite/ctors.html#faq-10.11

john

 

[RonHiler]

Unless I missed it you never said where you are calling InitializeTechs
from. Is it possible that you are running into the so called 'static
initialization order fiasco'. In other words because you are calling
InitializeTechs from the constructor of another global variable you are
inadvertently using the Breakthroughs vector before it has been constructed?

Thanks for the thought, but no, I call InitializeTechs from inside my
WinMain function, so that's not it

 

[RonHiler]

Since you delcare Name as a char array the default copy constructor will
work fine, it you change name to pointer then all bets are off.

It does not. The defualt constructor gives the same crash. And I'm
not using the default constructor for three reasons
1) I will be changing the char arrays to pointers and using 'new' to
allocate memory for them in the copy constructor (otherwise I'll be
wasting a lot of space, as I will have about 800 techs by the end).
For now they are arrays just to keep things simple until I get this
part debugged
2) The class in question contains vectors (see the header for
BreakthroughClass), and it's been my experience that default copy
constructors do not handle vectors well.
3) I consider it bad form to use default copy constructors in ANY
case, even when it seems perfectly safe. I will either prototype them
as private (and not define the function) if the class is meant to be
singly instanced, or, when I HAVE to have a copy constructor (such as
this case where the class is used as a vector member), I create my
own.

Also as
mentioned elsewhere use std::string

for purposes of performance, I dislike std::string.

I've discovered an odd thing. Having decided the source data was fine
(based on the ODS output), I turned my attention to where the data was
going. And, as it turns out, the code seems to work just fine when I
add a single line to the constructor for TechManagerClass

TechManagerClass::TechManagerClass()
{
Breakthroughs.clear();
Breakthroughs.reserve(100);
}

The addition of the reserve command seems to do the trick, though I'm
not sure why that should be. It appears as though the memory for the
vectors is not being allocated before the strcpy command in the CC.
Which seems very strange to me, as what I know about vectors suggests
that ought not be the case (I thought the first thing push_back() did
was allocate memory if needed).

Does someone know the reason for this? Is it because of the large
memory footprint of the class (1296 bytes per member if I calculate
that right)?

I've also considered the possibility that I'm just hiding the problem,
that in fact strcpy is still leaking (somehow) but now it has memory
space to leak into (because of the reserved space), and I'll run right
back into the same issue once I get more techs input into the resource
file. Can someone comment?

Thanks guys for the responses. I know it's hard to debug someone
else's code, and I appreciate the efforts.

Ron

 

[John Harrison]

It does not. The defualt constructor gives the same crash. And I'm
not using the default constructor for three reasons
1) I will be changing the char arrays to pointers and using 'new' to
allocate memory for them in the copy constructor (otherwise I'll be
wasting a lot of space, as I will have about 800 techs by the end).
For now they are arrays just to keep things simple until I get this
part debugged
2) The class in question contains vectors (see the header for
BreakthroughClass), and it's been my experience that default copy
constructors do not handle vectors well.
3) I consider it bad form to use default copy constructors in ANY
case, even when it seems perfectly safe. I will either prototype them
as private (and not define the function) if the class is meant to be
singly instanced, or, when I HAVE to have a copy constructor (such as
this case where the class is used as a vector member), I create my
own.


for purposes of performance, I dislike std::string.

I've discovered an odd thing. Having decided the source data was fine
(based on the ODS output), I turned my attention to where the data was
going. And, as it turns out, the code seems to work just fine when I
add a single line to the constructor for TechManagerClass

TechManagerClass::TechManagerClass()
{
Breakthroughs.clear();
Breakthroughs.reserve(100);
}

The addition of the reserve command seems to do the trick, though I'm
not sure why that should be. It appears as though the memory for the
vectors is not being allocated before the strcpy command in the CC.
Which seems very strange to me, as what I know about vectors suggests
that ought not be the case (I thought the first thing push_back() did
was allocate memory if needed).

Does someone know the reason for this? Is it because of the large
memory footprint of the class (1296 bytes per member if I calculate
that right)?

I've also considered the possibility that I'm just hiding the problem,
that in fact strcpy is still leaking (somehow) but now it has memory
space to leak into (because of the reserved space), and I'll run right
back into the same issue once I get more techs input into the resource
file. Can someone comment?

Thanks guys for the responses. I know it's hard to debug someone
else's code, and I appreciate the efforts.

I think this all points to heap corruption occurring somewhere else in your
code. push_back will allocate memory, but when you call reserve it no longer
needs to (at least not for a while). Heap corruption problems are notorious
being hard to find because they cause a crash sometime after the corruption
has actually occurred.

Is it possible for you to post a complete program that has this behaviour?
Try and cut out everything extraneous and then post the reduced program
here. Often going through this process will help you find the problem
because you'll remove some piece of code that you think has no bearing and
suddenly the problem goes away, therefore you've a pretty good idea that the
code just removed was the cause of the problem.

John