widelands
fix crashes with ships finding no path to the destination
Type of Change
Bugfix
Issue(s) Closed
Fixes #6642 Fixes #6224 Fixes #6445 (eventually) Fixes #6696
How it Works
[x] prevent terraforming on portdock locations [x] prevent ships getting stuck on terraformed coast. [x] split fleets when destination gets unreachable [x ] prevent fleet detection from crossing narrow land bridges
Possible Regressions
shiprouting performance maybe ports without fleets
@cb/Nordfriese before putting this to review I'd like to hear your opinion on this.
<@>Nordfriese wrote in https://codeberg.org/wl/widelands/issues/5002#issuecomment-2933409:
IMO the way to go is to simply prevent terraforming a triangle when there is a portdock on one of its nodes.
I think terraforming should be prevented not only on nodes with port docks, but for all nodes where a port- space could have a port dock. Consider a situation where a portspace get invalid because of terraforming. If a player is building the port and the port space get invalid during the building the port.
Mirrored from Codeberg On Fri Mar 28 08:47:29 CET 2025, frankystone wrote:
<@>Nordfriese wrote in https://codeberg.org/wl/widelands/issues/5002#issuecomment-2933409:
IMO the way to go is to simply prevent terraforming a triangle when there is a portdock on one of its nodes.
I think terraforming should be prevented not only on nodes with port docks, but for all nodes where a port- space could have a port dock. Consider a situation where a portspace get invalid because of terraforming. If a player is building the port and the port space get invalid during the building the port.
Ok I tested this cornercase.
If no port construction is started and the last possible filed for a port dock has been teraformed, the portspace vanishes as we do recalculate the Area around the node curently terraformed. This is completely correct IMO.
However if a port construction got started and is finished after the last possible field has been terraformed we throw an exception (warehouse.cc:668). So we need to deal with this.
Detecting whether a field could potentially hold a portdock and prevent it as @CB/frankystone suggested during the terraforming might be ugly costly as we need to search an area of probably radius 5 to find the port space and afterwards need to iterate over all possible fields according to the portdock placing algorithm ans see whether on of it is our field.
Maybe we could store an array of all this fields in the beginning of the game which would make the checking potentially less costly, but we would need to update this evrytime we calcualte an area of the map.
Another option would be to simply let burn the port when no port dock is possible, as we do this already if the portdock is destroyed due to changing borders for example.
third option might be to degrade the port to a simple warehouse or allow ports without portdocks. but especially to allow ports without docks might have verybig effects on the code where we assume we have a dock.
From a gameplay perspective my favourites would be 3b (allow ports without dock), 1b (avoid prestored fields in terraform) and then 2.
Opinions? @widelands/developers @CB/widelands-developers
I prefer avoiding a certain radius around portspaces (prestoring fields if that's the optimal implementation), and I strongly dislike the other 2 proposals. Instead of them, you could introduce a new preliminary portdock immovable type that would be placed at the same time as any new port construction site, or just store future portdock fields similar to proposal 1b.
From a gameplay perspective my favourites would be 3b (allow ports without dock)
This will lead to confusion if a player has a port which will be never used by ships. And i guess this needs a lot of changes to the port's window, e.g. the tabs "workers/wares waiting for a ship" are superfluous then.
Mirrored from Codeberg On Sat Mar 29 12:55:33 CET 2025, Tóth András (tothxa) wrote:
I prefer avoiding a certain radius around portspaces (prestoring fields if that's the optimal implementation), and I strongly dislike the other 2 proposals. Instead of them, you could introduce a new preliminary portdock immovable type that would be placed at the same time as any new port construction site, or just store future portdock fields similar to proposal 1b
preventing all potential fields for all portspaces for the complete game would be too much as it would not allow terraforming (diking) on spots where you won't be building a port anyway as well. So I like the idea of only blocking the fields of active constructionsites. I will try to implement that.
I get some different problems with this than before. I attached 2 saves from my testing. The first one (from current master with trading merged) is when everything is set up, and the dikers are about to split the navigable water body. If I run it with this branch merged, when the split happens, I get these errors on stdout:
[03:23:25.187 game] ERROR: ShipFleet::connect_port: Could not reach all ports!
[03:23:25.187 game] ERROR: ShipFleet::connect_port: Could not reach all ports!
[03:23:25.187 game] ERROR: ShipFleet::connect_port: Could not reach all ports!
... (repeated many times)
[03:23:29.572 game] ERROR: ShipFleet::connect_port: Could not reach all ports!
[03:23:29.572 game] ERROR: ShipFleet::connect_port: Could not reach all ports!
[03:23:29.665 game] ERROR: ShipFleet::connect_port: Could not reach all ports!
[03:23:29.665 game] WARNING: start_task_movedock: Failed to find a path: ship at 19x 32 to port at: 36x 38
[03:23:29.675 game] WARNING: start_task_movedock: Failed to find a path: ship at 19x 32 to port at: 27x 47
[03:23:30.380 game] WARNING: start_task_movedock: Failed to find a path: ship at 30x 41 to port at: 17x 27
[03:23:55.776 game] WARNING: The dock on 27x 47 without a fleet!
[03:23:55.776 game] WARNING: The dock on 27x 47 without a fleet!
The other file is saved from this state. When loaded, it's an instant segfault (same in master and with this branch merged):
FATAL ERROR: Received signal 11 (Segmentation fault)
Backtrace:
src/widelands-naval/widelands-port-diker(+0x8c9e8b)[0x562d6bb7fe8b]
/lib/x86_64-linux-gnu/libc.so.6(+0x37970)[0x7fa8047de970]
/lib/x86_64-linux-gnu/libstdc++.so.6(__dynamic_cast+0x3b)[0x7fa804b952fb]
src/widelands-naval/widelands-port-diker(_ZN9Widelands9ShipFleet6Loader11load_finishEv+0x22)[0x562d6b7b0ad2]
src/widelands-naval/widelands-port-diker(_ZN9Widelands15MapObjectPacket11load_finishEv+0x5d)[0x562d6b8356bd]
src/widelands-naval/widelands-port-diker(_ZN9Widelands18WidelandsMapLoader17load_map_completeERNS_14EditorGameBaseENS_9MapLoader8LoadTypeE+0xa7e)[0x562d6bab749e]
src/widelands-naval/widelands-port-diker(_ZN9Widelands13GameMapPacket13read_completeERNS_4GameE+0x13)[0x562d6b7a5823]
src/widelands-naval/widelands-port-diker(_ZN9Widelands10GameLoader9load_gameEb+0x5a5)[0x562d6b7bf1e5]
src/widelands-naval/widelands-port-diker(_ZN9Widelands4Game13run_load_gameERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEES8_+0x3e0)[0x562d6bb10180]
src/widelands-naval/widelands-port-diker(_ZN6FsMenu8LoadGame10clicked_okEv+0x255)[0x562d6ba39fe5]
src/widelands-naval/widelands-port-diker(+0x838e76)[0x562d6baeee76]
src/widelands-naval/widelands-port-diker(_ZN2UI5TableIPvE17handle_mousepressEhii+0x11d)[0x562d6ba6867d]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(+0x7a14b1)[0x562d6ba574b1]
src/widelands-naval/widelands-port-diker(_ZN2UI5Panel13ui_mousepressEhii+0xe3)[0x562d6ba57bb3]
src/widelands-naval/widelands-port-diker(_ZN13WLApplication12handle_inputEPK13InputCallback+0x36e)[0x562d6bbbe61e]
src/widelands-naval/widelands-port-diker(_ZN2UI5Panel6do_runEv+0x340)[0x562d6ba6cf60]
src/widelands-naval/widelands-port-diker(_ZN6FsMenu8MainMenu9main_loopEv+0x2f)[0x562d6ba31fef]
src/widelands-naval/widelands-port-diker(_ZN13WLApplication3runEv+0x414)[0x562d6bbb9904]
src/widelands-naval/widelands-port-diker(main+0xa3)[0x562d6b676f93]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xeb)[0x7fa8047cb09b]
src/widelands-naval/widelands-port-diker(_start+0x2a)[0x562d6b6772fa]
OTOH, if you let it run, and allow the expedition to start, it also segfaults:
FATAL ERROR: Received signal 11 (Segmentation fault)
Backtrace:
src/widelands-naval/widelands-port-diker(+0x8c9e8b)[0x558b0f79be8b]
/lib/x86_64-linux-gnu/libc.so.6(+0x37970)[0x7fe9fc889970]
src/widelands-naval/widelands-port-diker(_ZN9Widelands9ShipFleet6updateERNS_14EditorGameBaseE+0x0)[0x558b0f3cbfb0]
src/widelands-naval/widelands-port-diker(_ZN9Widelands19ExpeditionBootstrap14input_callbackERNS_4GameEPNS_10InputQueueEtPNS_6WorkerEPv+0x15)[0x558b0f3f62e5]
src/widelands-naval/widelands-port-diker(_ZN9Widelands7Request15transfer_finishERNS_4GameERNS_8TransferE+0x91)[0x558b0f402a61]
src/widelands-naval/widelands-port-diker(_ZN9Widelands12WareInstance14enter_buildingERNS_4GameERNS_8BuildingE+0x1a1)[0x558b0f3d0761]
src/widelands-naval/widelands-port-diker(_ZN9Widelands7Carrier19deliver_to_buildingERNS_4GameERNS_3Bob5StateE+0xbd)[0x558b0f4db38d]
src/widelands-naval/widelands-port-diker(_ZN9Widelands7Carrier16transport_updateERNS_4GameERNS_3Bob5StateE+0x1ee)[0x558b0f4a40ae]
src/widelands-naval/widelands-port-diker(_ZN9Widelands3Bob6do_actERNS_4GameE+0x59)[0x558b0f514719]
src/widelands-naval/widelands-port-diker(_ZN9Widelands6CmdAct7executeERNS_4GameE+0xf9)[0x558b0f3be309]
src/widelands-naval/widelands-port-diker(_ZN9Widelands8CmdQueue9run_queueERK8DurationR4Time+0x11c)[0x558b0f3cf00c]
src/widelands-naval/widelands-port-diker(_ZN9Widelands4Game5thinkEv+0x10e)[0x558b0f7274ce]
src/widelands-naval/widelands-port-diker(_ZN15InteractiveBase16game_logic_thinkEv+0x163)[0x558b0f5c2323]
src/widelands-naval/widelands-port-diker(_ZN2UI5Panel12logic_threadEv+0x8b)[0x558b0f68944b]
/lib/x86_64-linux-gnu/libstdc++.so.6(+0xbbb2f)[0x7fe9fcc6ab2f]
/lib/x86_64-linux-gnu/libpthread.so.0(+0x7fa3)[0x7fe9fcda1fa3]
/lib/x86_64-linux-gnu/libc.so.6(clone+0x3f)[0x7fe9fc94b06f]
<@>tothxa thanks for the findings. I am working on it but it takes time.
I am still working on this to get it into 1.3. However to fix the excellent test case provided by @tothxa it turned out that we need to reinitialize the complete fleet, as we can't tell which part of the fleet is still reachable and which is not.
@cb/tothxa this did resolve the issues in the very good test case you provided. But I am not sure whether all potential side effects have been fixed as well.
So heavy testing would be highly appreciated @cb/wl/developers
I think this is already a major improvement, so further testing can be done after merging.
However there's still some problem with portdocks resulting in a heap use after free on game exit after a fleet split, see attached ASan log.
Another related observation, not causing a crash, only unexpected in-game behaviour, so can be ignored in this PR: ships, ship construction sites and waterways can be diked over, and continue working. (of course ships can't actually move anymore)
thanks for the feedback.
I just realized this morning that it might be good to pull the split fleet code out to a separate function and will try to do that in this PR the next days. So I'll try to find out what is causiong the ASAN however I do not have ASAN on my windows machine so it might be that I need some help there
ok I have finished to move this to a proper function.
Not sure though whether this might have an effect on the ASAN issue.
<@>tothxa maybe you could recheck the Issue
I still get the segfault on exit in the release build, but the debug build crashes immediately.
I investigated the previous log, and it looks like some portdock is added to ObjectManager::objects_ with a new serial without removing the old.
As far as I can tell, the new one is because the Fleet is removed in the middle of Fleet::split() after all ships, ports and interfaces are removed from it, because all three remove_...() functions have an if (empty()) { remove(egbase) } branch. So probably release builds get away with the UAF because the memory is not yet reused, but ASan detects it.
I debugged the map object duplication, backtrace attached. Looks like it's in yard->init() now... oh, I see, you fixed ports by replacing init() with init_fleet(). Would init_yard_interfaces() work for shipyards?
Currently there is no such function. But now I have at least a glue where to work. Unfortunstely I am afk until sunday, so this has to wait until then. Thanks for helping ne out here.
It's a member of ProductionSite:
logic/map_objects/tribes/productionsite.h:513:
void init_yard_interfaces(EditorGameBase& egbase);
logic/map_objects/tribes/productionsite.cc:1225:
void ProductionSite::init_yard_interfaces(EditorGameBase& egbase)
... though it's protected...
Seems to work.
As for keeping the Fleet object alive when everything is temporarily removed, I tried replacing the
if (empty()) {
remove(egbase);
}
calls with update(egbase) as a quick and dirty hack. It seems to work, but I'm not sure if it's a good idea. <@>Nordfriese?
Seems to work.
As for keeping the
Fleetobject alive when everything is temporarily removed, I tried replacing theif (empty()) { remove(egbase); }calls with
update(egbase)as a quick and dirty hack. It seems to work, but I'm not sure if it's a good idea. <@>Nordfriese?
I dont think, it is necessary to keep the fleet alive, as it normally might change when calling the fleet merge function as well. Furthermore if it stays alive we might end with everything in the same fleet again, or even more severe imho we might pile up empty fleets.
OK, then another possible solution to https://codeberg.org/wl/widelands/pulls/5021#issuecomment-5188053 is to move the fleet splitting function somewhere else, e.g. to Game.
<@>tothxa wrote in https://codeberg.org/wl/widelands/pulls/5021#issuecomment-5402478:
OK, then another possible solution to #6661 (comment) is to move the fleet splitting function somewhere else, e.g. to
Game.
I don't understand that. Is the ASAN issue in this comment still true? I thought we might have fought it. If yes Could you provide another backtrace
Oh now I think I got the issue probably. It seems we are trying to log the "debug messages" against a non existent map_object "fleet" using the molog function. My proposal would be to just change this to another debug logging function.
Yes, molog() is the known problem, but I think it shows that it's very risky to do this in a member function of Fleet.
BTW, I've just realised that the same problem may happen with ferry fleets, needing a very similar solution. I'll try to whip up a test case.
I believe it doesn't matter where molog is used, because if we can't ensure that the object exists it will be always dangerous. And logging against a n unrelated mapobject is not good either. So I believe not using molog in this case is the best way. I believe also that the best place for this function is the ship_fleet as we do have all other functions that do modify it here (merge, remove, add etc.) I agree on the fleetyard problem, so I will have a look whether we coudl do something hre as well.
I had a quick check, and I se no way to originally detect the need to split a ferry fleet. Only thing that can be done in this PR, taking into account the the tieline of the freeze, would be to in initiate a ferry fleet split when an ocean split is detected by the shiprouting.
on a deeper check I identified a possible check in https://github.com/widelands/widelands/blob/89ac74e925ae11fa173b424ad9ec4bc03eadd42d/src/logic/map_objects/tribes/worker_tasks/ferry_row.cc#L82
however it seems not to be called ever.
As I will be AFK from latest friday (holidays) I suggest to keep the ferries out of this PR and ope a new issue instead. for 1.4
<@>bunnybot wrote in https://codeberg.org/wl/widelands/pulls/5021#issuecomment-5457357:
Mirrored from GitHub On Tue Jun 17 22:40:25 CEST 2025, hessenfarmer wrote:
on a deeper check I identified a possible check in
89ac74e925/src/logic/map_objects/tribes/worker_tasks/ferry_row.cc (L82)however it seems not to be called ever. As I will be AFK from latest friday (holidays) I suggest to keep the ferries out of this PR and ope a new issue instead. for 1.4
have identified the issue why the check was not triggered. working now on the implementation, but maybe finished only after the freeze.
feries is more complicated than ships due to the special handling of waterways.
so no chance to incorporate this change here.
I have stashed the progress alredy made locally though.
<@>Nordfriese <@>tothxa if you want to finish this for the freeze you are welcome.
I am afk now until june 30th. thanks for the support so far