I'm not sure about the dock damage. The vehicles exit the bay fully healed even when they are an opponent's unit in your garage. Also, the health bar in the garage displays fully green and doesn't reduce over time. I tried waiting sufficient time for the units to die if they were taking damage, saving the game, and then loading. Everything worked fine and the units exited the garage at full health.
Hooman, do you think that vehicles placed in a garage would need to be recorded in ScriptGlobal for save/load purposes? Currently, I am not tracking these units in ScriptGlobal like all the other units. If this is needed, I guess I would have assumed that the game would crash more consistently after loading?
pente, could you verify which difficulty the saved game you sent is using?
Below are the 2 helper functions to load vehicles in the garage that I use. I am not passing the vehicles outside of this function, so I'm pretty sure none of them should be somehow populating into a fight group or patrol or anything. Excuse the bad code as I was still very new to C++ and wrote this for my own consumption only.
void VehicleBuilder::CreateVehicleInGarage(Unit &garage, int bayIndex, map_id vehicleType, map_id cargo)
{
UnitDirection oldDirection = unitDirection;
unitDirection = UnitDirection::West;
Unit vehicle;
CreateVechLightsOn(vehicle, vehicleType, garage.Location(), cargo);
vehicle.PutInGarage(bayIndex, garage.Location().x, garage.Location().y);
unitDirection = oldDirection;
}
void VehicleBuilder::CreateVehicleInGarage(Unit &garage, int bayIndex, Truck_Cargo truckCargo, int cargoAmount)
{
UnitDirection oldDirection = unitDirection;
unitDirection = UnitDirection::West;
Unit vehicle;
CreateVechLightsOn(vehicle, map_id::mapCargoTruck, garage.Location(), map_id::mapNone);
if (cargoAmount > 0)
{
vehicle.SetTruckCargo(truckCargo, cargoAmount);
}
vehicle.PutInGarage(bayIndex, garage.Location().x, garage.Location().y);
unitDirection = oldDirection;
}
Thanks,
Brett
Hmm, that's some good debugging info. In particular, it's good to know the crash happens even on easy when the vehicles are owned by the same player as the garage. Perhaps there is something else in the code base which is interacting with the garages in an unexpected way. My guess would be Trigger or FightGroup related.
Pente, you seem to be doing a pretty thorough job trying to reproduce this bug. Would you be at all interested in trying some advanced debugging tools?
I'm taking a peek at the code.
This is probably unrelated, but in ReviewRogueFightGroupStatus, the logic here looks reversed. Is this to recycle the fight groups? I assume if TotalUnitCount() == 0 then it should set RogueFightGroupsAvailable = true. In particular, there is no code anywhere in the project aside from the initialization that sets RogueFightGroupsAvailable = true, meaning once used, the groups can never be used again.
if (scriptGlobal.RogueFightGroups[i].TotalUnitCount() == 0)
{
scriptGlobal.RogueFightGroupsAvailable[i] = false;
continue;
}
On another note, if TotalUnitCount() == 0 does mean the group is available for use, that can be used directly to track group availability rather than relying on a secondary array.
Of course, other code I saw seems to suggest the group tracking might be more of a countdown until some event happens, with no reuse. The intention of the code is not immediately obvious.
It took me a few tries, but with v1.0.1 off of github I managed to duplicate the garage bug (see dump below).
I have played around some more to see if I can find a way to reliably reproduce it. The steps are to start a new mission, save twice, and then click on a garage and press S. Saving only once never works, and more than twice doesn't help if twice wasn't enough. Loading is not necessary.
If Outpost has been closed for more than 5 minutes when I start it, and I immediately try to trigger the bug, the bug will happen. If Outpost crashed due to this bug, restarting Outpost and immediately trying the bug will always succeed. However if Outpost has been running normally for a while, the bug will usually not happen, even if I close and restarting Outpost.
I continue to think there is something funny going on in my system to explain this behavior.
I've attached two saves, which were made immediately after each other. In SGAME2 the bug does not happen, in SGAME3 it does. (Normal difficulty.)
Unhandled exception: page fault on read access to 0x00000001 in 32-bit code (0x004634cb).
Register dump:
CS:0023 SS:002b DS:002b ES:002b FS:0063 GS:006b
EIP:004634cb ESP:0034f9e0 EBP:04261c50 EFLAGS:00010202( R- -- I - - - )
EAX:00000001 EBX:00000000 ECX:04261cc8 EDX:00456ce0
ESI:00565d30 EDI:005663d4
Stack dump:
0x0034f9e0: 00000000 00565d30 00565d30 004d4fb0
0x0034f9f0: 00565d34 00000001 00456ce0 00565690
0x0034fa00: 04261cb0 0056d250 ffffffff 0047efb9
0x0034fa10: 0047efb9 04a04978 ffff0000 00000000
0x0034fa20: 0049d244 00000000 00577ee8 59960000
0x0034fa30: 00570010 00463153 00575d18 0045d1f7
Backtrace:
=>0 0x004634cb EntryPoint+0xffffffff() in outpost2 (0x04261c50)
1 0x04265850 (0x004cfca0)
2 0x0043ada0 EntryPoint+0xffffffff() in outpost2 (0x0041d630)
0x004634cb EntryPoint+0xffffffff in outpost2: call *0x0(%eax)
Modules:
Module Address Debug info Name (40 modules)
PE 370000- 3e0000 Deferred out2res
PE 400000- 5bc000 Export outpost2
PE 10000000-10011000 Deferred op2ext
PE 11000000-1102d000 Deferred c_euf
PE 20000000-2001a000 Deferred odasl
PE 7b430000-7b5ed000 Deferred kernel32
PE 7bc30000-7bc34000 Deferred ntdll
PE 7cde0000-7cde4000 Deferred dsound
PE 7d120000-7d123000 Deferred winealsa
PE 7d160000-7d164000 Deferred mmdevapi
PE 7d2f0000-7d2f8000 Deferred oleaut32
PE 7d520000-7d523000 Deferred api-ms-win-core-localization-l1-2-1
PE 7d550000-7d553000 Deferred api-ms-win-core-fibers-l1-1-1
PE 7d590000-7d593000 Deferred api-ms-win-core-synch-l1-2-0
PE 7d6c0000-7d6c3000 Deferred msvcr120
PE 7d7b0000-7d7b4000 Deferred uxtheme
PE 7d9d0000-7d9d3000 Deferred concrt140
PE 7da10000-7da14000 Deferred winex11
PE 7dce0000-7dce3000 Deferred api-ms-win-crt-heap-l1-1-0
PE 7dcf0000-7dcf3000 Deferred api-ms-win-crt-stdio-l1-1-0
PE 7dd10000-7dd13000 Deferred api-ms-win-crt-string-l1-1-0
PE 7dd20000-7dd23000 Deferred api-ms-win-crt-runtime-l1-1-0
PE 7dd30000-7dd33000 Deferred vcruntime140
PE 7dd80000-7dd84000 Deferred ucrtbase
PE 7de90000-7de94000 Deferred msvcrt
PE 7df90000-7df93000 Deferred msvcp140
PE 7e080000-7e084000 Deferred iphlpapi
PE 7e0b0000-7e0b4000 Deferred ws2_32
PE 7e0e0000-7e0e4000 Deferred wsock32
PE 7e100000-7e104000 Deferred imm32
PE 7e130000-7e133000 Deferred usp10
PE 7e1b0000-7e203000 Deferred comctl32
PE 7e300000-7e309000 Deferred msacm32
PE 7e340000-7e3bd000 Deferred winmm
PE 7e430000-7e434000 Deferred rpcrt4
PE 7e4f0000-7e518000 Deferred ole32
PE 7e650000-7e654000 Deferred advapi32
PE 7e6f0000-7e6f7000 Deferred gdi32
PE 7e850000-7e950000 Deferred user32
PE 7efe0000-7efe4000 Deferred version
Threads:
process tid prio (all id:s are in hex)
00000008 (D) Z:\home\user\games\outpost2\test\137\Outpost2.exe
00000036 15
0000002d 15
0000002c 0
0000002b 0
00000009 0 <==
0000000e services.exe
00000021 0
0000001a 0
00000013 0
00000010 0
0000000f 0
00000011 plugplay.exe
00000017 0
00000016 0
00000012 0
00000018 winedevice.exe
0000001c 0
0000001b 0
00000019 0
0000001d explorer.exe
00000027 0
00000026 0
00000025 0
0000001e 0
0000001f winedevice.exe
00000024 0
00000023 0
00000022 0
00000020 0
System information:
Wine build: wine-4.9
Platform: i386
Version: Windows 7
Host system: Linux
Host version: 4.20.4-gentoo
I spent some time on this with OllyDbg. Unfortunately I'd forgotten much of what I'd already posted on June 10th in this thread, and so mostly just reconfirmed it.
The bug appears when a garage stored unit has it's next pointer set to -1. This is generally a marker of an unused unit record. When the game is loaded, some patch up code is run to convert certain index fields back to pointers, as well as converting the unit type to a virtual function pointer. This patch up code skips over unit records that have a next pointer set to -1. This means units with a next pointer of -1 won't have a proper virtual function table pointer set. If any virtual method calls are made for that unit, the game will crash. Normally this is fine, as unit records with a next pointer of -1 are supposed to be unused, and so should never have any virtual methods called on them.
In the case of the garage bug, the stored units have their next pointer set to -1, yet the garage still references them. Hence there are referenced unit records which have been marked as unused, and have no valid virtual function table pointer set. When you view the contents of the garage, it attempts to call a virtual method on the stored units. The virtual method is to get a pointer to unit type information, so it can extract the name of the unit type for display purposes.
In the crashing saved game (saved_twice), the stored units have their next pointer set to -1. In the good saved game (saved_once), the stored units have a valid next pointer set.
Methods of interest:
00435CD0 Function: Map.UnitIndexesToPtrs() [[vtbl] and [next, prev, playerNext]]
0041CF40 Function: Unit:Building:Garage.OnLoad()
004632B0 Function: CommandPaneView:BuildingStorageBays.???() [DrawStorageBayPane]
It hasn't been observed how the stored unit records are marked as unused. There is however a bug in the code relating to some unique garage behavior.
When a unit is stored in a garage, the next pointer is set to -2. The game has lots of sentinel value checks for -1, but none for -2. Normally, if the field is not -1, it is expected to be a valid pointer to another unit record. In particular, the patch up code tries to convert pointers to indexes for all next pointers not set to -1. This calculation is not valid for the value -2, and so this corrupts the field when saving. The reverse conversion will also produce an unexpected and corrupted value.
It's not known if this corruption leads to further problems, or if it somehow gets corrected. In particular, I suspect the next pointer field would be restored to a proper value when a unit is unloaded from the garage. The field should remain largely unused while the unit remains in the garage, so the corrupted value may not be noticeable.
Given that this bug has only presented for this one level, and given that it does so consistently, there's likely something level specific that triggers this behavior. It may be caused in part by the corrupted next pointer field, or it may be caused through some other means. In particular, the garage stored units that end up with a next pointer field of -1 also have their flags field set to indicate an unused or dead unit. This suggests there may be some other mechanism involved that sets both the next pointer field and the flags field to indicate an unused unit record.
The problem may or may not relate to the corruption from the general bug, and most likely to something level specific. It's unlikely the problem -1 value would come from corruption alone.
Ahh gee, I didn't see your post until after we finished.
So the OllyDbg session went well. We were able to reproduce the bug while watching things unfold with OllyDbg. We spent a bit of time on initial setup getting the values we needed, and setting breakpoints at reasonable places. Brett did a quick run without OllyDbg to see if the bug could be reproduced by restarting the level, without having to go through the main menu, as that would save us quite a bit of time. After about 15-20 tries we found that was indeed possible. We then got setup in OllyDbg, and repeated the process. As the unit array potentially moved around each time the level was restarted, we found we had to clear and reset the hardware breakpoint on the field we were watching each round. This was a very tedious process. Brett managed to successfully reproduce the bug under OllyDbg after about 15-20 rounds. We kind of noticed something peculiar was up slightly before we witnessed what we were looking for. The next pointer on the unit record of a vehicle in a garage was first unexpectedly cleared to 0, and then shortly after was set to the -1 value we were looking for. As expected, the game crashed when we examined the garage contents after this.
The two writes before the crash were done in Map.UnitIndexToPtrs() and Map.SaveUnits. These were both functions that had been previously analysed, and were responsible for the translation between unit indexes and unit pointers during saving/loading. It was also related to the corruption of the value mentioned in earlier posts. Units have their next pointer set to -2 when loaded into a garage. This -2 value is not handled correctly by the conversion done during saving/loading. With a bit of luck, this corruption can lead the value to be converted back to 0. Normally a value of -1 is special cased to map to 0, and no valid value should map to 0. This means that during the reverse process, the 0 is special cased and maps to -1. Hence the -2 value is corrupted, and then converted 0, which is then mapped to -1, which indicates a dead unit, and so the game stops mapping the unit type back to a virtual function table, which leads to the crash.
To exhibit this bug, the -2 value must be corrupted in such a way that the reverse process maps the corrupted value back to 0. For this to happen, it depends on the address of the unit array. Each time the level is run, the unit array is reallocated, and may move around in memory. Certain addresses will trigger corruption in a way that the value gets mapped to 0. Hence the bug does not always exhibit itself every run.
This confirms the bug comes directly from the conversion process of the -2 value. There are no other factors at play here. This also shows the bug is in the game itself, and not part of the level. Any level with a garage, with units loaded into it, which is saved twice, runs the risk of corrupting the unit records of the stored units, and whether or not this happens depends on where in memory the unit array happened to be allocated.
The good news is, we can patch this. It shouldn't be too hard to write a replacement function that includes special casing for the -2 value as well as the -1 value. That should stop the corruption from happening during saving/loading. It won't fix already corrupted saved games, but it should prevent the problem from happening again. Perhaps we can roll something out in the next update of Outpost 2.
I may post further details later on about the exact sequence of events and values, and sample problem memory locations for the unit array. For now though, I'll leave the addresses of the two final writes that lead to values of 0 and -1 in the next field.
In Map.UnitIndexesToPtrs() the value 0 is unexpectedly stored at:
00435D34 MOV DWORD PTR SS:[EBP],EAX ; unit.ptr[j]* = [Unit*]nextUnit
In Map.SaveUnits(StreamIO* savedGameFile) the value -1 is stored at:
00435BC9 MOV DWORD PTR DS:[EDI],-1 ; Unit.ptr[j] = -1
Of particular note, is how unexpected the write of 0 was. The larger code sequence is:
00435D1B |/MOV EAX,DWORD PTR SS:[EBP] ; EAX = [Unit*]currentUnit.ptr[j]*
00435D1E ||CMP EAX,-1 ; Check if ([Unit*]ptr == -1) [IsDead]
00435D21 ||JE SHORT Outpost2.00435D39 ; -> Set unit.next* = 0
00435D23 ||SHL EAX,3 ; [EAX = unitIndex * 8]
00435D26 ||LEA EDX,DWORD PTR DS:[EAX+EAX*2] ; [EDX = unitIndex * 24]
00435D29 ||LEA EAX,DWORD PTR DS:[EDX+EDX*4] ; [EAX = unitIndex * 120]
00435D2C ||MOV EDX,DWORD PTR DS:[<Map.unitArray[]*>] ; EDX = Map.unitArray[]*
00435D32 ||ADD EAX,EDX ; EAX = &unitArray[unitIndex]
00435D34 ||MOV DWORD PTR SS:[EBP],EAX ; unit.ptr[j]* = [Unit*]nextUnit
00435D37 ||JMP SHORT Outpost2.00435D40 ; -> Skip instruction
00435D39 ||MOV DWORD PTR SS:[EBP],0 ; unit.ptr[j]* = 0 [null]
00435D40 ||ADD EBP,4 ; EBP = &ptr[j+1]
00435D43 ||DEC ECX ; numPtr--
00435D44 |\JNZ SHORT Outpost2.00435D1B
The bad write of 0 occurs at 00435D34. This is a calculated 0 value. It occurs when the corrupted field value is such that &Map.unitArray[corruptedIndex] == 0. For that to happen, we must have corruptedIndex * 120 = -&Map.unitArray, which is a rather bizarre thing to have happen. (Here sizeof(Unit) == 120). This is in contrast to the special cased write of 0 that would normally happen at 00435D39, which happens when an index value of -1 is converted to a nullptr.