bugfix(pathfinder): Fix rare retail pathfinding crashes in forwardInsertionSortRetailCompatible() and putOnClosedList() - #3113
Conversation
a201f76 to
4679cb1
Compare
|
Just a small wording tweak that i noticed. |
Mauller
changed the title
| @@ -1727,6 +1727,15 @@ void PathfindCell::forwardInsertionSortRetailCompatible(PathfindCellList& list) | |||
| UnsignedInt cellCount = 0; | |||
| while (currentCell && cellCount < PATHFIND_CELLS_PER_FRAME && currentCell->m_info->m_totalCost <= m_info->m_totalCost) | |||
There was a problem hiding this comment.
can currentCell->m_info be null here?
There was a problem hiding this comment.
Not in the way you are likely thinking.
When cells are initially put onto the list they are externally checked to make sure they have info allocated to them first.
The only time it crashes at this point is if a cell initially put onto the list has dangling next pointers.
| while (currentCell && cellCount < PATHFIND_CELLS_PER_FRAME && currentCell->m_info->m_totalCost <= m_info->m_totalCost) | ||
| { | ||
| // Prevent a retail crash where a pathfindCell has an m_info with a dangling nextOpen pointer | ||
| if (currentCell->m_info->m_nextOpen && !currentCell->m_info->m_nextOpen->m_cell->m_info) { |
There was a problem hiding this comment.
Normally when these cell infos have dangling pointers it tends to be present.
| currentCell->m_info->m_nextOpen->m_cell = nullptr; | ||
| currentCell->m_info->m_nextOpen = nullptr; | ||
|
|
||
| s_useFixedPathfinding = true; |
There was a problem hiding this comment.
This switches to fixed pathfinding but continues the current search on lists even if they're broken retail ones - could subsequent fixed sorting can consume a stale tail?
There was a problem hiding this comment.
The pathfinding crash that hits this point will likely bail after this point due to running out of pathfinding resources.
The pathfinding will be in a fudged state and it doesn't really matter if the list is a mess.
I don't usually like to toggle these flags unless it's in a cleaner part of the code, but not toggling them actually causes the replays to mismatch.
4679cb1 to
4ddc8a3
Compare
…ed list has no associated m_info (#3113)
… dangling linked list pointer (#3113)
4ddc8a3 to
ee6c4f5
Compare
| while (currentCell && cellCount < PATHFIND_CELLS_PER_FRAME && currentCell->m_info->m_totalCost <= m_info->m_totalCost) | ||
| { | ||
| // Prevent a retail crash where a pathfindCell has an m_info with a dangling nextOpen pointer | ||
| if (currentCell->m_info->m_nextOpen && !currentCell->m_info->m_nextOpen->m_cell->m_info) { |
There was a problem hiding this comment.
nit: { on new line for consistency with the code around it.
There was a problem hiding this comment.
I want to do some testing before this gets merged.
Edit: just so I don't lose track of the desired changes:
- Removal of the failover code for these fixes because they're not necessary for the (VC6) replays.
- Attaching the VS22 reproduction replays & custom map to the PR description.
- Proposed changes to
Pathfinder::forceCleanCellsimproving the printing.
4 participants
Closes: #2799
Closes: #2637
This PR fixes a recently discovered, but rarer to happen, pair of retail pathfinding crashes.
These crashes do not occur in the non-retail crash-fixed pathfinding.
The crash in putOnClosedList() occurs due to a pathfinding cell being placed onto the closed list without an associated pathfindCellinfo.
The retail insertion sort crash occurs for a similar reason, a cell placed onto the list contains a pathfind info that has a dangling nextOpen pointer to another cell info that is malformed.