Fix client crashes: dangling creature pointer and missile null tile - #19
Merged
tomluchowski merged 2 commits intoAug 1, 2026
Merged
Conversation
Playing for a few minutes reliably ended in a segfault inside GameMode::mouseMoved(), dereferencing a freed Creature from RenderManager::rrNormalizeAmbient(). InputManager::mHighlightedCreature is a bare pointer to the creature the mouse hovers, kept so its ambient can be restored once the mouse leaves it. Only GameMode::mouseMoved() and Creature::setPosition() ever cleared it, and neither runs when the creature dies: client-side deletion goes through deleteYourself() and GameMap::processDeletionQueues(), which touch nothing in InputManager. Hovering a creature that then died left the pointer dangling, and the next mouse move read freed memory. Clear it from ~Creature(). Three null dereferences in the same area, none of them observed firing: Creature::setPosition() dereferenced getPositionTile() twice without checking it, but that returns null whenever the creature sits outside the map, as it does while held in the keeper hand or carried. ODFrameListener::frameStarted() dereferenced getOverlayStatus() for every creature, which is null while a creature's mesh is not created -- Creature::update() checks it for exactly that reason. The two loops around the render target update also walked the creature list twice and paired the results by index, which only holds while no creature gains or loses its overlay in between; remember the overlays actually hidden instead. Tile::setEverVisible() was declared to return bool and returned nothing, which is undefined behaviour. Its only caller ignores the result, so this was harmless in practice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> (cherry picked from commit 0302da9)
Playing on eventually ended in a segfault on the server thread, inside
MissileObject::doUpkeep(), reading address 0xf8. That is offset 248,
which is where Tile::mX sits.
doUpkeep() walks the tiles along the missile's path, remembering the
previous one in lastTile so that a missile stopped by a wall can be
placed on the tile it came from. lastTile starts null and is only
assigned at the bottom of the loop, so when the first tile of the path is
already a wall both branches of the wall case read through it. The loop
below that case already checks lastTile for null, which is what the wall
case should have done too.
That happens when a missile is launched straight at a wall, or when the
tile it occupies is filled in under it, both of which take a while to come
up: the log for this one shows it on turn 8858, right after
missile name=RenderedMovableEntity_Cannon_10_6102, hit wall on tile=[23,32]
which doUpkeep() logs two statements before the dereference.
There is no previous tile to fall back on in that case, so end the
missile where it already is.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit d908986)
Owner
|
Checked that out, seems both helpful and necessery .... |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two reliably reproducible segfaults, one commit each:
InputManager::mHighlightedCreaturewas left dangling when the hovered creature died, so the next mouse move read freed memory. Also fixes three null dereferences in the same area, andTile::setEverVisible()being declaredboolwhile returning nothing — which is what currently breaks the base branch's build whenOD_ENABLE_WARNINGSis on (-Werror=return-type).MissileObject::doUpkeep()read through a nulllastTilewhen the first tile of the missile's path is already a wall.Split out of #16 so each topic can be reviewed on its own. Merging all of the split PRs reproduces the tree of #16 exactly.
🤖 Generated with Claude Code