I recently had way to much spare time and to do something semi productive some of it was spent on debugging spring.
This is my findings so far:
QuadField.cpp line 92
"baseQuads[*qi].units.erase(ui);"
should probably be
"baseQuads[*qi].teamUnits[unit->allyteam].erase(ui);"
And
CobEngine.cpp line 132
"sleeping.pop();" should be execute before "cur->Tick(deltaTime);" because "cur" is a pointer to a object in a priority_queue and "Tick(deltaTime)" can and will modify the "wakeTime" which corrupts the priority_queues sort order which in turn leads to memory corruption.
And a huge THANKS to the SY team for all the great work on this game.
Bug hunting
Moderator: Moderators
Bug hunting part two
After some more digging I found another small bug in ├óÔé¼┼ôCobEngine.cpp├óÔé¼┬Ø this time on line 38 when ├óÔé¼┼ôsleeping.pop()├óÔé¼┬Ø accesses the deleted element.
The next bug is in ├óÔé¼┼ôTransportUnit.cpp├óÔé¼┬Ø line 46 when ├óÔé¼┼ôti->unit->xsize├óÔé¼┬Ø is used but all that remains of the ├óÔé¼┼ôCUnit├óÔé¼┬Ø object is a nearly destructed ├óÔé¼┼ôCObject├óÔé¼┬Ø.
This could sometimes crash the game when someone destroyed a loaded transport.
I think the best solution is to add a ├óÔé¼┼ôunitSize├óÔé¼┬Ø member to the "TransportedUnit" struct.
For the last part in the trilogy we have ├óÔé¼┼ôGuiHandler.cpp├óÔé¼┬Ø line 800, the if statement on this line begins with ├óÔé¼┼ôinCommand>0├óÔé¼┬Ø which assumes that the only way to deselect a unit is with the mouse. Naturally that├óÔé¼Ôäós not true so if you select a unit and press attack but instead changes focus to an empty group the game crashes.
One solution is to replace the test with ├óÔé¼┼ôinCommand < commands.size()├óÔé¼┬Ø but I don├óÔé¼Ôäót think that├óÔé¼Ôäós an optimal solution.
The next bug is in ├óÔé¼┼ôTransportUnit.cpp├óÔé¼┬Ø line 46 when ├óÔé¼┼ôti->unit->xsize├óÔé¼┬Ø is used but all that remains of the ├óÔé¼┼ôCUnit├óÔé¼┬Ø object is a nearly destructed ├óÔé¼┼ôCObject├óÔé¼┬Ø.
This could sometimes crash the game when someone destroyed a loaded transport.
I think the best solution is to add a ├óÔé¼┼ôunitSize├óÔé¼┬Ø member to the "TransportedUnit" struct.
For the last part in the trilogy we have ├óÔé¼┼ôGuiHandler.cpp├óÔé¼┬Ø line 800, the if statement on this line begins with ├óÔé¼┼ôinCommand>0├óÔé¼┬Ø which assumes that the only way to deselect a unit is with the mouse. Naturally that├óÔé¼Ôäós not true so if you select a unit and press attack but instead changes focus to an empty group the game crashes.
One solution is to replace the test with ├óÔé¼┼ôinCommand < commands.size()├óÔé¼┬Ø but I don├óÔé¼Ôäót think that├óÔé¼Ôäós an optimal solution.
-
Doomweaver
- Posts: 704
- Joined: 30 Oct 2004, 14:14
-
adeveloper.clan-sy
- Posts: 24
- Joined: 28 Apr 2005, 00:26
I think the delete is applied to the const CCobThread* reference which is returned by sleeping.top() so actually the pointer still remains at the top (though it points to a deleted object). The sleeping.pop() then removes the pointer.After some more digging I found another small bug in ├óÔé¼┼ôCobEngine.cpp├óÔé¼┬Ø this time on line 38 when ├óÔé¼┼ôsleeping.pop()├óÔé¼┬Ø accesses the deleted element.
I can not find the code you are referring to, in "GuiHandler.cpp line 800".
Yes you are totally right but I am using the visual studio 2005 beta and Microsoft seems to have changed their priorey_quean implementation.
In the new version of the library the pointer is dereferenced during the call to pop, but as long as VC 2003 is the main development platform it should be fine.
And now I will try to figure out exactly why someone would like to access the top element through pop.
Edit: A small suggestion
Units should not be able to issue attack orders on themselves.
Sometimes they start spinning and shooting in random directions behaving like they are on crack.
And bombers try to use the distance to the enemy in a division which leads to a divide with zero.
I tried to implement it by adding
if(c.id==CMD_ATTACK && c.params.size()==1 && uh->units[int(c.params[0])] == owner)
c.id=CMD_STOP;
around line 330 in CommandAI.cpp but I don├óÔé¼Ôäót think it is a good fix because I haven't fully comprehended the command system yet.
In the new version of the library the pointer is dereferenced during the call to pop, but as long as VC 2003 is the main development platform it should be fine.
And now I will try to figure out exactly why someone would like to access the top element through pop.
Edit: A small suggestion
Units should not be able to issue attack orders on themselves.
Sometimes they start spinning and shooting in random directions behaving like they are on crack.
And bombers try to use the distance to the enemy in a division which leads to a divide with zero.
I tried to implement it by adding
if(c.id==CMD_ATTACK && c.params.size()==1 && uh->units[int(c.params[0])] == owner)
c.id=CMD_STOP;
around line 330 in CommandAI.cpp but I don├óÔé¼Ôäót think it is a good fix because I haven't fully comprehended the command system yet.
What flags are you using?Shodan wrote:Yes you are totally right but I am using the visual studio 2005 beta and Microsoft seems to have changed their priorey_quean implementation.
In the new version of the library the pointer is dereferenced during the call to pop, but as long as VC 2003 is the main development platform it should be fine.
And now I will try to figure out exactly why someone would like to access the top element through pop.
When I tried compiling Spring from the CVS, it gives a ton of errors with VS 2005 beta2. I've managed to remove most of the errors, but over 3000 warnings with warnings dropped down to level 1...
"mmsystem.h" introduces a #define for "PlaySound" which conflicts with CSound's PlaySound. This occurs when not using dsound (undefining USE_DSOUND).
Ok this is a bit too late but who cares.
I can├óÔé¼Ôäót find the file you are referring to and my crystal ball is a bit rusty today so could you enlighten me with the error messages you are getting?
Hopefully this can be to some help.
Shodans guide to spring in visual studio 2005
Part one: The Spring engine
1. Download the most recent source code.
2. Open the solution and go to "Settings->C/C++->Advanced->Disable Specific Warnings" and add
4430 (missing type specifier - int assumed. Note: C++ does not support default-int)
And
4335 (Mac file format detected: please convert the source file to either DOS or UNIX format)
3. Now most of spring will compile fine but there are still a few small issues remaining:
AVIGenerator.cpp line 291 cast "m_sFile" to a const TCHAR* ├óÔé¼┼ô(const TCHAR*)m_sFile├óÔé¼┬Ø to resolve the ambiguous.
PathFinder.h line 93 add "typedef value_type & reference;" and "typedef const value_type & const_reference;"
AdvSky.cpp line 772 cast the first argument to pow to a float.
Part two: Liberty dependencies
1. Spring utilizes freetype and jpeg so download them from there respective website "www.freetype.org" and "www.jpeg.org".
2. Unpack the archives, now from the spring solution select "File->Add->Existing Project" go to "(wear you extracted freetype)\builds\win32\visualc"
and add it to the project.
3. Add the files from the jpeg library to the solution and create "jconfig.h" with the correct flags.
4. Ok I know that this part leaves a lot to be desired, so feel free to ask anything.
I can├óÔé¼Ôäót find the file you are referring to and my crystal ball is a bit rusty today so could you enlighten me with the error messages you are getting?
Hopefully this can be to some help.
Shodans guide to spring in visual studio 2005
Part one: The Spring engine
1. Download the most recent source code.
2. Open the solution and go to "Settings->C/C++->Advanced->Disable Specific Warnings" and add
4430 (missing type specifier - int assumed. Note: C++ does not support default-int)
And
4335 (Mac file format detected: please convert the source file to either DOS or UNIX format)
3. Now most of spring will compile fine but there are still a few small issues remaining:
AVIGenerator.cpp line 291 cast "m_sFile" to a const TCHAR* ├óÔé¼┼ô(const TCHAR*)m_sFile├óÔé¼┬Ø to resolve the ambiguous.
PathFinder.h line 93 add "typedef value_type & reference;" and "typedef const value_type & const_reference;"
AdvSky.cpp line 772 cast the first argument to pow to a float.
Part two: Liberty dependencies
1. Spring utilizes freetype and jpeg so download them from there respective website "www.freetype.org" and "www.jpeg.org".
2. Unpack the archives, now from the spring solution select "File->Add->Existing Project" go to "(wear you extracted freetype)\builds\win32\visualc"
and add it to the project.
3. Add the files from the jpeg library to the solution and create "jconfig.h" with the correct flags.
4. Ok I know that this part leaves a lot to be desired, so feel free to ask anything.
Even more bugs deluxe edition
BFReadmap.cpp line 23 and PathEstimator.cpp line 642. This isn├óÔé¼Ôäót really a bug but changing ├óÔé¼┼ôfind├óÔé¼┬Ø to ├óÔé¼┼ôfind_last_of├óÔé¼┬Ø will prevent errors with filenames containing more then one dot.
BuilderCAI.cpp line 434. This line should probably be ├óÔé¼┼ôif(id>=MAX_UNITS){├óÔé¼┬Ø.
BFReadmap.cpp line 357. This code will read random data from ram and use it in deep craters, this creates a nice effect but can potentially lead to instability. To fix it a new case should be created for craters deeper then 1024, maybe something like this:
else {
tempMem[(y*xsize+x)*4+0] = waterHeightColors[1023*4+0];
tempMem[(y*xsize+x)*4+1] = waterHeightColors[1023*4+1];
tempMem[(y*xsize+x)*4+2] = waterHeightColors[1023*4+2];
tempMem[(y*xsize+x)*4+3] = 0;
}
I haven├óÔé¼Ôäót really looked into this but ├óÔé¼┼ôneedPath├óÔé¼┬Ø now defaults to false and this can create an access violation in PathFinder.cpp on line 385 since ├óÔé¼┼ôfoundPath.path├óÔé¼┬Ø is empty.
Maybe you are aware of this but the fix for patrolling construction units with weapons doesn├óÔé¼Ôäót completely eliminate all problems. Now ├óÔé¼┼ôAddDeathDependence├óÔé¼┬Ø will get a null pointer and ├óÔé¼┼ôCBuilderCAI::FinishCommand├óÔé¼┬Ø should properly clean up after the dead unit setting ├óÔé¼┼ôtargetDied├óÔé¼┬Ø to false and so, maybe by always call CMobileCAI::FinishCommand.
Buggi: I will see if I can do something about mapconv but it would be easier to come up with a fix if I had some more specific information or perhaps a way to reproduce it.
BFReadmap.cpp line 23 and PathEstimator.cpp line 642. This isn├óÔé¼Ôäót really a bug but changing ├óÔé¼┼ôfind├óÔé¼┬Ø to ├óÔé¼┼ôfind_last_of├óÔé¼┬Ø will prevent errors with filenames containing more then one dot.
BuilderCAI.cpp line 434. This line should probably be ├óÔé¼┼ôif(id>=MAX_UNITS){├óÔé¼┬Ø.
BFReadmap.cpp line 357. This code will read random data from ram and use it in deep craters, this creates a nice effect but can potentially lead to instability. To fix it a new case should be created for craters deeper then 1024, maybe something like this:
else {
tempMem[(y*xsize+x)*4+0] = waterHeightColors[1023*4+0];
tempMem[(y*xsize+x)*4+1] = waterHeightColors[1023*4+1];
tempMem[(y*xsize+x)*4+2] = waterHeightColors[1023*4+2];
tempMem[(y*xsize+x)*4+3] = 0;
}
I haven├óÔé¼Ôäót really looked into this but ├óÔé¼┼ôneedPath├óÔé¼┬Ø now defaults to false and this can create an access violation in PathFinder.cpp on line 385 since ├óÔé¼┼ôfoundPath.path├óÔé¼┬Ø is empty.
Maybe you are aware of this but the fix for patrolling construction units with weapons doesn├óÔé¼Ôäót completely eliminate all problems. Now ├óÔé¼┼ôAddDeathDependence├óÔé¼┬Ø will get a null pointer and ├óÔé¼┼ôCBuilderCAI::FinishCommand├óÔé¼┬Ø should properly clean up after the dead unit setting ├óÔé¼┼ôtargetDied├óÔé¼┬Ø to false and so, maybe by always call CMobileCAI::FinishCommand.
Buggi: I will see if I can do something about mapconv but it would be easier to come up with a fix if I had some more specific information or perhaps a way to reproduce it.
All good bugs except the last one. Its true that targetDied wasnt reset and that could lead to problems for later attack orders (they would get ignored possibly starting a loop) but where would AddDeathDependece get a null pointer?
You must be a real monster code reader to have found all those ones though :)
You must be a real monster code reader to have found all those ones though :)
Yes that null pointer was one of my private bugs, and here comes a new line of suspects.
UnitDefHandler.cpp line 86. This doesn├óÔé¼Ôäót take restricted units in account, easily fixed by setting the yardmap pointer on all units to null or by only allocating space for the number of units actually used.
Game.cpp line 1057 ought to be "delete []".
RadarHandler.cpp line 108. Changing xsize to ysize eliminates some problems on non-square maps.
UnitDefHandler.cpp line 86. This doesn├óÔé¼Ôäót take restricted units in account, easily fixed by setting the yardmap pointer on all units to null or by only allocating space for the number of units actually used.
Game.cpp line 1057 ought to be "delete []".
RadarHandler.cpp line 108. Changing xsize to ysize eliminates some problems on non-square maps.