Vectors in copy constructor causes heap corruption.

Started by
21 comments, last by KazenoZ 12 years, 8 months ago
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.
Advertisement
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.


"In order to understand recursion, you must first understand recursion."
My website dedicated to sorting algorithms
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...

This topic is closed to new replies.

Advertisement