Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault when city removal causes boat displacement and unit removed with Lua during unit_moved event #2477

Open
XHawk87 opened this issue Dec 30, 2024 · 4 comments
Labels
bug Something isn't working server This issue requires changes to the server

Comments

@XHawk87
Copy link
Contributor

XHawk87 commented Dec 30, 2024

Describe the bug
The server crashes if a city is disbanded, for example by migration, and the city contained a boat which was forced to move off of the city tile, and during the unit_moved Lua event the boat is destroyed.

To Reproduce
Steps to reproduce the behaviour:

  1. Load the saved game
  2. Start the game
  3. See crash

Expected behaviour
No crash.

Screenshots
image

Platform and version (please complete the following information):

  • OS: Linux
  • Freeciv21 version: 3.1-rc.1
  • Ruleset/Longturn game (if applicable): Chaos

Additional context
Discord discussion

@XHawk87 XHawk87 added bug Something isn't working Untriaged This issue or PR needs triaging labels Dec 30, 2024
@XHawk87
Copy link
Contributor Author

XHawk87 commented Dec 30, 2024

I'm looking into this bug. I found that it's possible to use bounce_unit() for all but one feature of the custom bounce code, displaying a city tile link for the disbanded city.

const char *ctl = city_tile_link(pcity);

freeciv21/server/citytools.cpp

Lines 1748 to 1753 in 74984fa

notify_player(unit_owner(punit), tile1, E_UNIT_RELOCATED,
ftc_server,
_("Moved %s out of disbanded city %s "
"since it cannot stay on %s."),
unit_link(punit), ctl,
terrain_name_translation(tile_terrain(pcenter)));

How would you go about solving this? Should I pass in some extra data with the bounce_reason somehow?

/// Why do we need to bounce a unit?
enum class bounce_reason {
generic, ///< We just need to do it
terrain_change, ///< We need to do it because of changing terrain
};
void bounce_unit(struct unit *punit, bool verbose,
bounce_reason reason = bounce_reason::generic,
int max_distance = 2);

@XHawk87
Copy link
Contributor Author

XHawk87 commented Dec 30, 2024

Actually, single-responsibility. I don't think bounce_unit() should also be trying to communicate, it should just be bouncing the unit and reporting success or failure. We should have something else that's solely responsible for communicating the outcome.

@XHawk87
Copy link
Contributor Author

XHawk87 commented Jan 2, 2025

See related refactor #2478

@XHawk87
Copy link
Contributor Author

XHawk87 commented Jan 2, 2025

See another related refactor #2481

@lmoureaux lmoureaux added server This issue requires changes to the server and removed Untriaged This issue or PR needs triaging labels Jan 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working server This issue requires changes to the server
Projects
None yet
Development

No branches or pull requests

2 participants