bugfix(tunnel): Restore retail compatibility after changes to TunnelTracker::onTunnelDestroyed() and TunnelContain::onCapture() - #3242
Conversation
Caball009
added
Bug
PR Summary by QodoRestore retail-compatible tunnel lifecycle and passenger cleanup
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1. Tunnel creation assertion inverted
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| Generals/Code/GameEngine/Source/Common/RTS/TunnelTracker.cpp | Removes the unsafe early return and safely clears containment links when retail-compatible bookkeeping leaves no remaining tunnel ID. |
| Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TunnelContain.cpp | Excludes the non-retail capture override from retail-compatible Generals builds. |
| GeneralsMD/Code/GameEngine/Source/Common/RTS/TunnelTracker.cpp | Corrects the previously reported assertions and iterator usage, separates retail destruction behavior, and versions tunnel-count serialization. |
| GeneralsMD/Code/GameEngine/Include/Common/TunnelTracker.h | Retains the explicit tunnel count only for retail-compatible CRC builds and derives it from the ID list otherwise. |
| GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Contain/TunnelContain.cpp | Applies the capture override only where retail compatibility does not prohibit it. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Tunnel destroyed] --> B{Retail-compatible CRC?}
B -->|Yes| C[Remove ID and preserve retail count behavior]
B -->|No| D[Remove tracked ID using list iterator]
C --> E{Tunnel count is zero?}
D --> F{ID list is empty?}
E -->|Yes| G[Cave in and clear occupants]
F -->|Yes| G
E -->|No| H[Reassign containedBy to remaining tunnel or null]
F -->|No| I[Reassign containedBy to remaining tunnel]
Reviews (12): Last reviewed commit: "Added complete retail incompatible versi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
1d42639 to
001ee15
Compare
001ee15 to
342ce12
Compare
|
Code review by qodo was updated up to the latest commit 342ce12 |
…mpatibility macro.
342ce12 to
a3b7cb8
Compare
a3b7cb8 to
a34726c
Compare
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3d34f16 to
ddc9b76
Compare
ddc9b76 to
8545c86
Compare
8545c86 to
4faa5ea
Compare
|
Code review by qodo was updated up to the latest commit 4faa5ea |
4faa5ea to
f7a763e
Compare
|
Code review by qodo was updated up to the latest commit 4faa5ea |
f7a763e to
d77b627
Compare
| } | ||
|
|
||
| if( m_tunnelCount == 0 ) | ||
| m_needsFullHealTimeUpdate = true; |
There was a problem hiding this comment.
This only needs to be set true if m_tunnelIDs is modified.
There was a problem hiding this comment.
I think it only needs to be set to true if !m_tunnelIDs.empty() after the erase operation. I'll change it to that.
| } | ||
| else | ||
| { | ||
| m_tunnelCount = m_tunnelIDs.size(); |
There was a problem hiding this comment.
Maybe this should only be set on Xfer Load?
There was a problem hiding this comment.
Why is that?
Ah, we may not want to modify m_tunnelCount on saving. Is there a downside to doing it like this, though?
There was a problem hiding this comment.
If everything goes well it essentially would be a no-op. But if the sizes are out of sync, then the Xfer Save silently repairs it, which is unexpected? ;-)
There was a problem hiding this comment.
True, it's unexpected, but it seems like a good thing to me :) If we keep it, I think I should add a comment, though. I'll just add an XFER_LOAD check, that seems better on second thought.
d77b627 to
322dbfd
Compare
4 participants
This PR makes 2 changes:
TunnelContain::onCaptureto Generals is not retail compatible so this needs to be behind theRETAIL_COMPATIBLE_CRCmacro.TunnelTracker::onTunnelDestroyedis not retail compatible so this had to be changed.m_containList) hold on to a pointer to their container. This can lead to use-after-free bugs and crashes. The early return is removed.The
m_tunnelCountinteger underflow cannot be fixed with retail compatibility enabled, but the code should be well-defined and safe now.See commits for clean diffs.
Object *validTunnel = TheGameLogic->findObjectByID( m_tunnelIDs.front() );I'm not entirely familiar with the STLPort implementation of
std::list<T>, but accessing the front element of an emptystd::listappears to result reliably inT{}for trivial types;INVALID_IDin this context. As a result, all contained objects have their container pointer updated tonullptr, neatly avoiding the use-after-free bugs and resulting crash.Crash reproduction with the current main branch (VC6 / VS22, RETAIL_COMPATIBLE_CRC == 1), using Zero Hour GLA campaign mission 3:
Video and call stack
gen_zh_md_gla03_campaign_tunnel_crash.mp4
TODO: