bugfix(object): Clean up stranded occupants and preserve teardown lookup by bobtista · Pull Request #3308 · TheSuperHackers/GeneralsGameCode

bobtista · 2026-09-17T09:21:09Z

Fixes Occupant of a Tunnel Network crashes the game when destroyed after its owner surrendered with asset transfer #3315
Related to Contained unit dereferences its destroyed container during combat after a surrender with asset transfer #3316, bugfix(tunnel): Restore retail compatibility after changes to TunnelTracker::onTunnelDestroyed() and TunnelContain::onCapture() #3242 and bugfix(object): Avoid potential crash due to dangling contain module when Troop Crawler is destroyed before reaching Reinforcement Pad #3165

In retail-compatible Generals, surrendering can transfer Tunnel Networks without updating their tunnel tracker. If those tunnels are later sold or destroyed, occupants can retain stale container pointers and crash when destroyed, including during match cleanup. These steps do not reliably reproduce the crash; it depends on what happens to the freed memory.

Now Object::onDestroy checks whether the container ID is still registered. If it is missing, the occupant is removed from its tunnel tracker and its container link is cleared. The object lookup table also stays available until match cleanup finishes deleting objects in both Generals and Zero Hour.

Note: It still reads through the stale pointer and cannot detect memory reused by another live object. Cleanup is also skipped if the stale container no longer exposes a contain module. Other uses of the dangling pointer are in #3316. The new destruction check is limited to retail-compatible Generals.

Teardown-order testing found no simulation regression in the tested replays. Propaganda Center ownership restoration was not exercised, and audible teardown effects were not verified.

generals_3315_tunnel_reset_crash.zip

Todo:

Test the reported VC6 replay with bugfix(dozeraiupdate): Fix issue where builders could resume completed tasks after being disabled #2793 excluded: no CRC mismatch, and cleanup finishes instead of crashing
Test an automated LAN recording where the ally continues playing after the transferred tunnels are sold: playback stays in sync
Test the poisoned-memory recording: fixed playback finishes without the destruction crash
Play the reported replay twice in one process with clean exits
Attach the reported VC6 replay
Compare both teardown orders in Zero Hour using real VC6: 27 retail replays across three back-to-back chains stayed in sync in both orders, including four 64k–69k-frame games and up to 6097 objects at reset

coderabbitai · 2026-09-17T09:21:27Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

Walkthrough

The change updates stale containment cleanup and reports removal success from tunnel trackers. GameLogic::reset now rebuilds the object hash after destroying all objects.

Changes

Stale containment cleanup

Layer / File(s)	Summary
Containment removal result `Generals/Code/GameEngine/Include/Common/TunnelTracker.h`, `Generals/Code/GameEngine/Source/Common/RTS/TunnelTracker.cpp`	`removeFromContain` now returns `true` when it removes an object and `false` when the object is absent.
Stale containment destruction `Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp`	`Object::onDestroy` checks container liveness by ID for Generals retail-compatible CRC builds. It removes stale containment through player tunnel trackers and clears containment after successful removal. Live containers use their contain module.
Reset hash rebuild `Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp`	`GameLogic::reset` clears and initializes `m_objHash` after `destroyAllObjectsImmediate()`. STLport uses `resize`; other builds use `reserve`.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix · Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant GameLogicReset
  participant ObjectDestructor
  participant GameLogic
  participant TunnelTracker
  GameLogicReset->>ObjectDestructor: destroy all objects
  ObjectDestructor->>GameLogic: find containing object by ID
  ObjectDestructor->>TunnelTracker: remove stale object from containment
  TunnelTracker-->>ObjectDestructor: report removal success
  GameLogicReset->>GameLogicReset: rebuild m_objHash

Merge Risk: 🟠 High · up to 09e91

Destroying or resetting occupants after a transferred tunnel is removed can still crash the game, so the liveness check must be made safe before merge.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The changes target `#3315` and preserve the `RTS_GENERALS && RETAIL_COMPATIBLE_CRC` condition. They also preserve the disabled `TunnelContain::onCapture` behavior and move `m_objHash` cleanup after `des…	Guard every access to `m_containedBy` with a liveness check that does not dereference the stale pointer. Only call `getContain()` and `getID()` after that check. Add or update automated regression coverage for occupant destruction and `Game…

✅ Passed checks (3 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	The changed files support `#3315`. The `TunnelTracker` return value reports stale-list removal, and the `GameLogic::reset` hash-table order supports object-liveness handling during cleanup. No unrelated…
Title check	✅ Passed	The title clearly identifies the main fix: cleaning up stranded occupants while preserving teardown lookup. It is concise and directly related to the changeset.
Description check	✅ Passed	The description explains the stranded-container crash, the retail-compatible scope, the object lookup change, cleanup behavior, limitations, and test results. It is directly related to the changeset.

Full details: Linked Issues check

Explanation

The changes target #3315 and preserve the RTS_GENERALS && RETAIL_COMPATIBLE_CRC condition. They also preserve the disabled TunnelContain::onCapture behavior and move m_objHash cleanup after destroyAllObjectsImmediate. However, Object::onDestroy still evaluates m_containedBy->getContain() before the liveness check. The stale-container branch then evaluates m_containedBy->getID(). Both operations dereference the freed container before the code can recover it, so the destruction and reset crash in #3315 is not reliably prevented.

Resolution

Guard every access to m_containedBy with a liveness check that does not dereference the stale pointer. Only call getContain() and getID() after that check. Add or update automated regression coverage for occupant destruction and GameLogic::reset after transferred tunnel destruction.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 05462f18-5541-4952-bf16-ef691cac9291

📥 Commits

Reviewing files that changed from the base of the PR and between 2632833 and e013859.

📒 Files selected for processing (1)

Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TunnelContain.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

…estroyed container

coderabbitai

Actionable comments posted: 1

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1cb11c8f-3dfa-44f4-a2f0-6908eeb4faff

📥 Commits

Reviewing files that changed from the base of the PR and between a4f6a80 and 09e91b4.

📒 Files selected for processing (4)

Generals/Code/GameEngine/Include/Common/TunnelTracker.h
Generals/Code/GameEngine/Source/Common/RTS/TunnelTracker.cpp
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp
Generals/Code/GameEngine/Source/GameLogic/System/GameLogic.cpp

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.

The use of GameLogic::findObjectByID relies on this.

Caball009 · 2026-09-28T15:01:11Z

Please clean up the PR description. The issue reproduction steps are inaccurate, because the crash is much rarer than this.

It's currently a wall of text that I think can be reduced by at least half without losing important information; it also references things like the VC6 replay that's not included anywhere.

Edit: Feel free to reorganize the commits if that helps for clean diffs.

greptile-apps · 2026-10-01T20:24:07Z

Confidence Score: 4/5

[Medium risk] Fixes tunnel occupant cleanup during object destruction.

The PR does not appear safe to merge while destruction can still dereference a freed container.

Findings

Stale container still dereferenced ▶

Summary

The PR adds cleanup for stranded tunnel occupants in retail-compatible Generals and keeps object-ID lookup tables available until match object destruction finishes.

Tunnel-tracker removal now reports whether it found the occupant.
Both games defer clearing their object lookup tables until after destruction.

_{Reviews (2) · Last reviewed commit: "refactor(gamelogic): Clear the Zero Hour..."}

greptile-apps · 2026-10-01T20:24:12Z

+#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC
+		// TheSuperHackers @bugfix bobtista / Caball009 17/09/2026 Remove stranded tunnel occupants during destruction.
+		// The lookup detects unregistered IDs, but cannot detect reuse of the freed container memory.
+		if (!TheGameLogic->findObjectByID(m_containedBy->getID()))


Stale container still dereferenced When a transferred tunnel has been freed but an occupant still points to it, this check reads the freed tunnel to get its ID. The condition above also reads it to get its contain module. If that memory is inaccessible, destruction can crash before the new cleanup runs. If the memory has been reused by a registered object with a contain module, the lookup can instead take the normal removal path and leave the occupant in its tunnel tracker.

Yes, it still reads through the stale pointer and cannot detect memory reused by another live object. Those limitations are in the description. The underlying dangling-pointer issue remains in #3316.

Agreed. The PR description explicitly documents both limitations, and #3316 tracks the broader dangling-m_containedBy issue during gameplay. This change is intentionally a teardown-only safeguard, so I don’t have an additional actionable concern on this line.

xezon · 2026-10-02T11:35:02Z

 	if( m_containedBy && m_containedBy->getContain() )
 	{
-		m_containedBy->getContain()->removeFromContain( this );
+#if RTS_GENERALS && RETAIL_COMPATIBLE_CRC


I am skeptical about this change because it explicitly addresses Generals, indicating that this is no issue in Zero Hour, which then begs the question why and can we merge the Zero Hour code responsible for fixing this instead of having a separate fix for Generals that eventually will conflict with Zero Hour when merging?

TunnelContain::onCapture is not retail compatible in Generals, hence Generals specific issues.

Will all the code of this change disappear on merge?

Only if the merge coincides with abandoning retail compatibility.

Ok can you review this change?

…stroying all objects

coderabbitai Bot requested a review from Caball009 September 17, 2026 09:22

coderabbitai Bot reviewed Sep 17, 2026

View reviewed changes

Comment thread Generals/Code/GameEngine/Source/GameLogic/Object/Contain/TunnelContain.cpp Outdated

bobtista force-pushed the bobtista/bugfix/tunnel-transfer-dangling-container branch from e013859 to 79cd96b Compare September 17, 2026 19:29

bobtista changed the title ~~bugfix(tunnel): Fix crash when a transferred tunnel dies with occupants~~ bugfix(object): Fix crash when destroying an occupant of an already destroyed container Sep 17, 2026

bugfix(object): Fix crash when destroying an occupant of an already d…

a4f6a80

…estroyed container

bobtista force-pushed the bobtista/bugfix/tunnel-transfer-dangling-container branch from 79cd96b to a4f6a80 Compare September 17, 2026 19:40

This was referenced Sep 17, 2026

Occupant of a Tunnel Network crashes the game when destroyed after its owner surrendered with asset transfer #3315

Open

Contained unit dereferences its destroyed container during combat after a surrender with asset transfer #3316

Open

coderabbitai Bot reviewed Sep 18, 2026

View reviewed changes

Comment thread Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp Outdated

Caball009 added 2 commits September 18, 2026 02:57

refactor: clear the object hash table after destroyAllObjectsImmediate

c0efa7a

The use of GameLogic::findObjectByID relies on this.

refactor: Improve performance

a25c611

Caball009 force-pushed the bobtista/bugfix/tunnel-transfer-dangling-container branch from 09e91b4 to a25c611 Compare September 18, 2026 00:58

bobtista added 2 commits September 22, 2026 14:53

refactor(object): Extract tunnel containment cleanup

59f318f

refactor(generals): Keep tunnel cleanup private and clarify its limits

d3c76f9

bobtista changed the title ~~bugfix(object): Fix crash when destroying an occupant of an already destroyed container~~ bugfix(generals): Clean up stranded tunnel occupants on destruction Sep 24, 2026

bobtista marked this pull request as ready for review October 1, 2026 20:20

greptile-apps Bot reviewed Oct 1, 2026

View reviewed changes

Caball009 mentioned this pull request Oct 1, 2026

bugfix(contain): Avoid Generals crashes for attacking units from previously transferred Tunnel Network #3341

Draft

1 task

xezon reviewed Oct 2, 2026

View reviewed changes

refactor(gamelogic): Clear the Zero Hour object lookup table after de…

c7c29a6

…stroying all objects

bobtista changed the title ~~bugfix(generals): Clean up stranded tunnel occupants on destruction~~ bugfix(object): Clean up stranded occupants and preserve teardown lookup Oct 2, 2026

Conversation

bobtista commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Caball009 commented Sep 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greptile-apps Bot commented Oct 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Uh oh!

greptile-apps Bot Oct 1, 2026

Choose a reason for hiding this comment

Uh oh!

bobtista Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

xezon Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Caball009 Oct 2, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bobtista commented Sep 17, 2026 •

edited

Loading

coderabbitai Bot commented Sep 17, 2026 •

edited

Loading

Caball009 commented Sep 28, 2026 •

edited

Loading

greptile-apps Bot commented Oct 1, 2026 •

edited

Loading