Vectors in copy constructor causes heap corruption.

Maybe the problem is not in the class Event, but in the usage you make of it (you try to use an invalid event).

Without more code, we can't tell. You should show the full Event class, and also a minimalist usage that reproduces the problem.

There's nothing much else in the Event class to view though, it's all just a bunch of integer and boolean members, and they all get copied fine in the constructor, it's just the vector that causes problems.

I couldn't manage to reproduce the problem in a minimal program, as I don't even know where to start trying to =\

But in the program this goes in, this is usually the last frame in the stack before the ctor is called:

//...more value setting comes before this point
evC.y = it.y;
evC.name = "exEvent";
evC.solid = 2;

evList.push_back(evC);
++EVENT_COUNT;

it.cmdCount = 0;
++it.evCount;


evC and it are both Events, evList is a vector of Events.
The stack will point to ++it.evCount, saying that it is uninitiallized, though looking at the evC that drew values from it, and the copy constructor in the next stack frame, the source values are fine, and the crash still seems to come from the string vector... Again, I'm not really sure about what happens here...

Your thread title states that the copy construction causes heap corruption, but I don't think that's true. More likely, it reveals previously existing heap corruption as it makes a new allocation. The code you posted for the copy constructor is fine as far as I can tell (though it is unnecessary; the default copy constructor is probably fine).

Right... so... where do I look now, actually? What kind of operations do you reckon could cause this? Debugging seems to suggest the push_back method, but I just don't know, the values are valid before it and then corrupt after use, I don't really know where to look now...

Edit:
I tried using the console to make sure about the corruption around the push_back method, and it does seem to crash inside it, the call stack shows:
GameEngine.exe!std::_Construct<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::basic_string<char,std::char_traits<char>,std::allocator<char> > >(std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Ptr="", const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & _Val="Number: (zIndex2` add` set` 1)0") Line 52 + 0x36 bytes C++
GameEngine.exe!std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > >::construct(std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Ptr="", const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & _Val="Number: (zIndex2` add` set` 1)0") Line 155 + 0xd bytes C++
GameEngine.exe!std::_Uninit_copy<std::basic_string<char,std::char_traits<char>,std::allocator<char> > *,std::basic_string<char,std::char_traits<char>,std::allocator<char> > *,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >(std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _First="Number: (zIndex2` add` set` 1)0", std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Last=<Bad Ptr>, std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Dest="", std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > & _Al={...}, std::_Nonscalar_ptr_iterator_tag __formal={...}, std::_Nonscalar_ptr_iterator_tag __formal={...}) Line 131 + 0x10 bytes C++
GameEngine.exe!stdext::unchecked_uninitialized_copy<std::basic_string<char,std::char_traits<char>,std::allocator<char> > *,std::basic_string<char,std::char_traits<char>,std::allocator<char> > *,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >(std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _First="Number: (zIndex2` add` set` 1)0", std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Last=<Bad Ptr>, std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Dest="", std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > & _Al={...}) Line 822 + 0x55 bytes C++
GameEngine.exe!std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >::_Ucopy<std::basic_string<char,std::char_traits<char>,std::allocator<char> > *>(std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _First="Number: (zIndex2` add` set` 1)0", std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Last=<Bad Ptr>, std::basic_string<char,std::char_traits<char>,std::allocator<char> > * _Ptr="") Line 1141 + 0x18 bytes C++
GameEngine.exe!std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > >::operator=(const std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > & _Right=[3]("Number: (zIndex2` add` set` 1)0","CreateEv: (zIndex2` ev` 7` Zombee` static` 2` 2` 16` 16` 8` Zombee)0","Wait: (set` 10000)0")) Line 595 + 0x1d bytes C++
> GameEngine.exe!Event::Event(const Event & it={...}) Line 647 C++
GameEngine.exe!std::_Construct<Event,Event>(Event * _Ptr=0x00ca73f0, const Event & _Val={...}) Line 52 + 0x36 bytes C++
GameEngine.exe!std::allocator<Event>::construct(Event * _Ptr=0x00ca73f0, const Event & _Val={...}) Line 155 + 0xd bytes C++
GameEngine.exe!std::_Uninit_copy<Event *,Event *,std::allocator<Event> >(Event * _First=0x003faae0, Event * _Last=0x003fb0bc, Event * _Dest=0x00ca73f0, std::allocator<Event> & _Al={...}, std::_Nonscalar_ptr_iterator_tag __formal={...}, std::_Nonscalar_ptr_iterator_tag __formal={...}) Line 131 + 0x10 bytes C++
GameEngine.exe!stdext::unchecked_uninitialized_copy<Event *,Event *,std::allocator<Event> >(Event * _First=0x003f9f28, Event * _Last=0x003fb0bc, Event * _Dest=0x00ca6838, std::allocator<Event> & _Al={...}) Line 822 + 0x55 bytes C++
GameEngine.exe!std::_Uninit_move<Event *,Event *,std::allocator<Event>,std::_Undefined_move_tag>(Event * _First=0x003f9f28, Event * _Last=0x003fb0bc, Event * _Dest=0x00ca6838, std::allocator<Event> & _Al={...}, std::_Undefined_move_tag __formal={...}, std::_Undefined_move_tag __formal={...}) Line 207 + 0x15 bytes C++
GameEngine.exe!stdext::_Unchecked_uninitialized_move<Event *,Event *,std::allocator<Event> >(Event * _First=0x003f9f28, Event * _Last=0x003fb0bc, Event * _Dest=0x00ca6838, std::allocator<Event> & _Al={...}) Line 864 + 0x51 bytes C++
GameEngine.exe!std::vector<Event,std::allocator<Event> >::_Umove<Event *>(Event * _First=0x003f9f28, Event * _Last=0x003fb0bc, Event * _Ptr=0x00ca6838) Line 1148 + 0x18 bytes C++
GameEngine.exe!std::vector<Event,std::allocator<Event> >::_Insert_n(std::_Vector_const_iterator<Event,std::allocator<Event> > _Where={index=-1414812757 name=<Bad Ptr> pic=0x003fc950 ...}, unsigned int _Count=1, const Event & _Val={...}) Line 1182 C++
GameEngine.exe!std::vector<Event,std::allocator<Event> >::insert(std::_Vector_const_iterator<Event,std::allocator<Event> > _Where={index=-1414812757 name=<Bad Ptr> pic=0x003fc950 ...}, const Event & _Val={...}) Line 878 C++
GameEngine.exe!std::vector<Event,std::allocator<Event> >::push_back(const Event & _Val={...}) Line 824 C++
GameEngine.exe!eventResolver(std::vector<Event,std::allocator<Event> > & evList=[15]({index=1 name="" pic=0xbaadf00d ...},{index=6 name="" pic=0xbaadf00d ...},{index=5 name="" pic=0xbaadf00d ...},{index=6001 name="" pic=0x01ec4ae0 ...},{index=5001 name="" pic=0x027b0008 ...},{index=3 name="" pic=0xbaadf00d ...},{index=2 name="" pic=0xbaadf00d ...},{index=3001 name="" pic=0x028f4008 ...},{index=1001 name="" pic=0x01ec4bf0 ...},{index=1001 name="" pic=0x023a0008 ...},{index= na,...), std::vector<Event,std::allocator<Event> > & gevList=[3]({index=-858993460 name="-1" pic=0xcccccccc ...},{index=-858993460 name="Zombee" pic=0xcccccccc ...},{index=-858993460 name="Damage" pic=0xcccccccc ...}), Event & it={...}, Chara & ch={...}, SDL_Surface * screen=0x003b6328, Project & p={...}, std::vector<vPoint,std::allocator<vPoint> > & pList=[1]({x=512 y=736 h=-858993460 ...}), std::vector<vNum,std::allocator<vNum> > & numList=[6]({x=1001.0000 name="zIndex" },{x=2001.0000 name="zIndex2" },{x=3001.0000 name="zIndex3" },{x=4001.0000 name="zIndex4" },{x=5001.0000 name="zIndex5" },{x=6001.0000 name="zIndex6" }), std::vector<vText,std::allocator<vText> > & strList=[0](), std::vector<vSwitch,std::allocator<vSwitch> > & sList=[2]({x=false name="Attacking" },{x=false name="Hit" }), std::vector<vPic,std::allocator<vPic> > & picList=[1]({x=0x003bff78 basePic=0x003bff78 name="Zombee" ...}), std::vector<vSound,std::allocator<vSound> > & sndList=[0](), int scale=2) Line 9527 C++
GameEngine.exe!Map::Play(Project project={...}, Chara ch={...}, std::basic_string<char,std::char_traits<char>,std::allocator<char> > data="~Properties{Name: [Zombees]Path: [D:\Mor'sC++\Phantasya Projects\Copy of Zombees\Zombees\Zombees.gpj]}~Settings{Font: [D:\Mor'sC++\Phantasya Projects\Copy of Zombees\\Zombees\font.ttf]TBox: [D:\Mor'sC++\Phantasya Projects\Copy of Zombees\\Zombees\messageBox.png]Hero: [D:\Mor'sC++\Phantasya Projects\Copy of Zombees\\Zombees\Kyou Sword.PNG]Width: [24]Height: [32]Frames: [4]ScreenW: [800]ScreenH:" ) Line 1027 + 0x76 bytes C++
GameEngine.exe!SDL_main(int argc=2, char * * argv=0x003f37d8) Line 124 C++
GameEngine.exe!_main() + 0xf5 bytes C
GameEngine.exe!__tmainCRTStartup() Line 586 + 0x17 bytes C


Lots of frames regarding the vectors and their operations, and the 2 frames that matter are the Event's copy constructor which points to the command list, like I stated above, and the EventResolver that points to the push_back method .


Edit 2:
Digging a little into the stack brought me this:
The frame above the eventResolver is push_back's code:
else
insert(end(), _Val);
}
Which calls the frame above it, insert, with the value to be put at the end of the vector, so far so good
This is the insert code from the stack:
iterator insert(const_iterator _Where, const _Ty& _Val)
{ // insert _Val at _Where
size_type _Off = size() == 0 ? 0 : _Where - begin();
_Insert_n(_Where, (size_type)1, _Val);
return (begin() + _Off);
}
Now, _Val still has the value that I need to push back, but the _Where iterator apparently points to an uninitialized Event, so that's probably where it starts going wrong. But why is that?

Default copy constructor will be fine for a class containing a std::vector of std::strings, no need to implement your own. What happens if you omit your own copy constructor completely?

Please show the entire event class - this is probably a violation of the rule of three somewhere.

Basically, I need an explicit copy constructor because I need to condition some of the members.
I.E
if(!it.noImg){
this->pic = it.pic;
this->basePic = it.basePic;
}

And not for the vector, but as to my understanding, don't I need to explicitly assign all members in case I do use a cctor?

Also, doesn't the Rule of Three state that if I have an explicit ctor, dtor or cctor, I need to make ones for the other 2 as well? In which case, I need to have a basic constructor to intialize my values correctly, so that would mean I need both a copy constructor and a destructor defined as well.
Either way, I have all 3 in the class =\

As par your question about removing the copy constructor, that doesn't do much good, the only difference it does is that now it doesn't show me the stack when it crashes =\

Do you mean to shallow copy that picture? How is it's lifecycle managed? If you delete it in the destructor you'd be in for a world of hurt.

That class looks like a blob ! Maybe fixing this will lead you to finding the problem (or to making it disappear). What is an Event by the way ? it seems to combine the attributes of an actual event, of an entity, and more ...

There is still the possibility that you are *using* the class incorrectly (ie: the problem is outside the class, like using a deleted Event). How about showing eventResolver ?

Also, doesn't the Rule of Three state that if I have an explicit ctor, dtor or cctor, I need to make ones for the other 2 as well? In which case, I need to have a basic constructor to intialize my values correctly, so that would mean I need both a copy constructor and a destructor defined as well.

No. Rule of three is that if you need any one of the following - copy constructor, destructor or assignment operator - you probably need all three. Constructors themselves are not really relevant to rule of three.

Could you show the body of your destructor for event? I agree with Slavik81 - shallow copy of the SDL_Image is the first thing I'd question.

Event::~Event(){
this->commandList.clear();
this->moveList.clear();
if(!this->noImg && this->pic != NULL){
this->pic = NULL;
this->basePic = NULL;
this->noImg = true;
}
}

The pictures are managed in a picture list object, and each time one is needed it is shallow copied to the appropriate SDL_Surface*, the surface is only freed when the list's destructor is called when the pictures aren't needed anymore, and the respective SDL_Surface*es destruct by being set a NULL, so other pictures using the pointer remain intact.
Is that bad? I went to this way because deep copying still kept giving me crashes when freeing the surface every time an Event is destructed.

@Lumpa, I'm not sure it's really a blob, wouldn't collision checks and movement methods normally go inside the moving object's class? All the members relate to nothing but the Event itself, so I don't see how the class takes over the entire program either.
And yes, the Event is basically all of what you said, it's basically an object in the game that the user defines whether it is just a set of rules, just a static object, an interactable object or NPC or anything else.
It might've been better to go with inheritance, but the purpose was to leave it as open as possible to user editing, and not developer editing, so I figured it would be best not to right now. And just until now it seemed to work well.


I also don't think that I'm trying to operate on a deleted event, since while debugging, when the crash occurs, the whole vector's members are intact, and the problem doesn't even happen when messing with the Event vector, but with the string vector inside the Event vector's members, and even goes halfway in there.

I'd show the eventResolver, but it's a pretty big function that translates the strings and redirects to other functions, it'd be kind of messy to post. The sortEvents function also causes that crash, and is a lot smaller and comprehendable, I'll post that:
//This sorts the events and the Hero's alignment accordingly
void sortEvents(vector<Event> &events, Chara &Hero)
{
//If a new event has been added, or if the Hero moved, the events
//will be sorted, this is to save memory on sorting on every
//pass of the game loop.

//An extra event is added to represent the Hero's
//location.
Event ev;
ev.name = "HERO";
ev.solid = Hero.solid;
ev.y = Hero.y;
events.push_back(ev);
++EVENT_COUNT;

//Sorts the events
sort(events.begin(), events.end(), compareEvents);

//This now searches where the Hero was put in position
//and passes that info to the Hero's drawPos variable
//to be used in the drawing sequence.
for(unsigned int i = 0; i < EVENT_COUNT; i++){
if(events.name == "HERO"){
Hero.drawPos = i;
events.erase(events.begin()+i);
--EVENT_COUNT;
break;
}
}
}

compareEvents compares the events by their solidity and y values for priority on drawing later, the player's character can't be sorted by this way, and so I needed to enter a proxy for it.

Just for the sake of being clear, the basic constructor on the Event class defines the pic and basePic members as NULL at default and clears the commandList and moveList vectors.

Sorry no solution, but a few things that you might clarify:

What you are doing in Event destructor is useless, because a destructed Event will not be used again, so all these manipulations that don't release things are not necessary.

Is there a relation between EVENT_COUNT and the size of the event vector given in parameter ? There should be, or your for loop would be dangerous, but then why don't you just use events.size() ?

I also don't think that I'm trying to operate on a deleted event, since while debugging, when the crash occurs, the whole vector's members are intact, and the problem doesn't even happen when messing with the Event vector, but with the string vector inside the Event vector's members, and even goes halfway in there.[/quote]
Going as far as halfway does not prove much. If you're accessing deleted or corrupted memory, anything can happen. And if you use a delete/corrupted Event, it's precisely *inside* the Event that things are bad.


PS: I still don't see what you call and "event", functionally. For me, an event is something that occurs once. Even if you don't want to break things into many classes, a bit of separation wouldn't hurt, imho.

Edit: another idea: I don't know if a bad comparison function can corrupt a vector while sorting (it can do bad things to map keys at least), but what is compareEvents ? are you sure it is a "strict weak ordering" ?

Edit2: it seems that it could happen. So, show us compareEvents

I know that about the destructor, I only defined one after this error began, since the call stack pointed to it, so I created my own to see what in it crashes, that didn't really help though.

EVENT_COUNT is a global variable storing the events.size() value, when I was trying to optimize it turned out that calling events.size() many times per game loop slowed me down alot, so instead I'm using a simple integer that stores it outside, and it seems to work better, though I'm not sure why the call to size() took that much effort from the machine, shouldn't it be linear complexity?

Event is probably the wrong name to go by, I just called it that automatically by habit from my old days with RPG Maker, it's supposed to be somewhat like that.

This is compareEvents' code:
bool compareEvents(Event a, Event b){
if(a.solid == b.solid){
return a.y < b.y;
}
static const int lookup[] = {1, 0, 2, 1};
return lookup[a.solid] < lookup[b.solid];
}

I'm pretty sure that's strict weak.
lookup's order is what it is to comply with solid's definition:

//Solidity of the event
//0 = Solid, 1 = Passes under, 2 = Passes over, 3 = Halfsolid.
int solid;[/quote]

vector::size() complexity is constant, so you'd have to call it a lot to notice performance problems (a suspicious lot).

Sorry I'm out of ideas. You should really strip this down, if not for your readers (including yourself), this could help providing a minimal reproducing case.

Are you sure that all your events have valid "solid" values. Consider adding an assert in your ccomparison function to ensure you aren't stepping outside the bounds of the array:

template<class T, size_t N>
size_t array_size(T (&)[N])
{
return N;
}

bool compareEvents(const Event &a, const Event &b)
{
if(a.solid == b.solid)
{
return a.y < b.y;
}

static const int lookup[] = {1, 0, 2, 1};

assert(a.solid >= 0 && a.solid < array_length(lookup));
assert(b.solid >= 0 && b.solid < array_length(lookup));

return lookup[a.solid] < lookup[b.solid];
}

Note that your event class is rather large, I'd recommend passing it by const reference to functions.

I'm pretty sure they have valid values, the solid value assignments are hardcoded, and don't have any incremental locations. I added the assertion anyway, but it doesn't do any good.

Also, I'm not sure how it's supposed to work for me with the const reference, I need the event class to be able to be changed, doesn't const lock it from editing?


I'm lost again by this point... Isn't there anything that might hint you guys at something?

I'm pretty sure they have valid values, the solid value assignments are hardcoded, and don't have any incremental locations.

It could get a bad value if you implement a copy constructor and forget to copy this field.

Also, I'm not sure how it's supposed to work for me with the const reference, I need the event class to be able to be changed, doesn't const lock it from editing?

You can at least easily add this in the comparison method as shown by rip-off.
For other places, if you don't want to refactor ... you can try passing around a pointer or shared_ptr of Event, but I'm not sure adding this complexity is a good idea at this point. You should first fix the problem before you think of optimizing.

Also, I'm not sure how it's supposed to work for me with the const reference, I need the event class to be able to be changed, doesn't const lock it from editing?
[/quote]
Passing a parameter as a const reference doesn't "lock" the original object. Declaring an object const "sortof" does (in standardese) it isn't legal to make changes to an object that was originally declared declared const.

Can you post your EventResolver class header/source, and your Event class source?

When it comes to a class with say 10 members where you find yourself needing to implement either the copy-constructor, assignment operator, or destructor for justf one of the members in that class, the wrong thing to do is exactly that.
The right thing to do in that case is to implement a separare 'smart' class that contains just that member and has the required copy-constructor, assignment operator and destructor for just that member. Then you include this 'wrapper' smart-class as a member in your larger class and presto, you're done. Of course when you can use an existing smart-class for that one member instead then even better.

I just found the problem, and as always it was just a very simple, stupid, thing I've been missing...

The game didn't crash because of corrupted data, it crashed because of the constant need to resize the vector... just using a reserve function at the creation time of the vector fixed the problem.
This would have been very viable for this system due to the constant growth and no way to determine a good size to reserve, but atleast it's a start, and I can atleast approximate that a user won't need more than a 100...


Thanks for the help guys...