Ships: Fix crash when doing expedition between adjacent harbor places by Noseey · Pull Request #1895 · Return-To-The-Roots/s25client

Noseey · 2026-02-17T13:46:42Z

Related Issue:
Closes #1784

Changes Summary
This fixes a crash when expeditions are done between harbor places that are directly adjacent to another (e. g. on autogenerated maps), so the ship doesn't need to move at all for reaching them and thus its route_ is empty. For more details, see related issue.

Note:
@Flamefire: Like discussed, could you add also a test for this if possible?

…Route to avoid crashes for lost and stuck ships.

.. assuming we have already arrived in case of DriveToHarbourPlace and ContinueExpedition. Consider a route check to be false in case it is empty.

Flamefire

Will look into adding a test, generally I think this works but might miss the notification, see comment. It might look odd though: Player clicks "next" and immediately receives a notification about a waiting ship.
But I guess that's ok and rate enough. Better than missing it and having a ship idle.
Or what do you think?

About the style: Compare those 3:

if (foo)
  return x;
else
  bar():
return y;

if (foo)
  return x;
else {
  bar():
  return y;
}

if (foo)
  return x;
bar():
return y;

I find the last one the clearest.

Flamefire · 2026-02-17T14:07:12Z

libs/s25main/nodeObjs/noShip.cpp

This check exists already at the top of the function, which got me confused.

I think the only situation would be a route recalculation by one of the 2 FindShipPathToHarbor
So either put a route_.empty() check there after curRouteIdx = 0; or something like:

Suggested change

// Already arrived? (e. g. adjacent harbor)

if(curRouteIdx == route_.size())

return Result::GoalReached;

else

StartDriving(route_[curRouteIdx++]);

return Result::Driving;

// FindShipPathToHarbor above found we arrived already?

if(route_.empty())

return Result::GoalReached;

RTTR_Assert(curRouteIdx < route_.size();

StartDriving(route_[curRouteIdx++]);

return Result::Driving;

(the else was also misleading as it can be omitted after return and looked like return Result::Driving; would always be executed)

This check exists already at the top of the function, which got me confused.

Yes, in the end it is just another "are we there yet?" check , just after the route recalculations. I found this to be more in line with the rest of the code and since curRouteIdx is set to 0 in these cases, it would also work for empty routes in my opinion. But I can change it back to an check against it being empty.

About the else, I agree, that was not necessary.

My issue as I was reading the function was: This check already exists, why do it again? And what is it about adjacent harbor? DriveToHarbourPlace isn't only called from a harbor so without the background knowledge of this issue it might be confusing.

Only after checking all uses of route_ I remembered that FindShipPathToHarbor changes it's parameter and wanted to keep that knowledge.

Maybe:

Suggested change

// Already arrived? (e. g. adjacent harbor)

if(curRouteIdx == route_.size())

return Result::GoalReached;

else

StartDriving(route_[curRouteIdx++]);

return Result::Driving;

// Already arrived after possibly recalculating route (FindShipPathToHarbor)

if(curRouteIdx == route_.size())

return Result::GoalReached;

RTTR_Assert(curRouteIdx < route_.size();

StartDriving(route_[curRouteIdx++]);

return Result::Driving;

The other idea was to have the check local to where it matters: Above

-curRouteIdx = 0; +curRouteIdx = 0; +if(route_.empty()) + return Result::GoalReached;

And then have only RTTR_Assert(curRouteIdx < route_.size(); here

I probably haven't taken into account the history of the example map when adding this additional check. For the already stuck ships (due to them driving regardless the access to routes_ was OOB), the curRouteIdx was 1.

With a proper check in place in noShip::ContinueExpedition, this should no longer be possible. I would say, just to be save, we change the check on the top of this function to >= if(curRouteIdx >= route_.size()) and remove the additional check at the bottom. What do you say?

just to be save

When curRouteIdx > route_.size() we already had an OOB and things are likely damaged already. So that situation cannot happen in a state we can recover from anyways.
It most I'd add an RTTR_Assert(curRouteIdx < route_size()) when == failed, i.e. not returned

So while I appreciate being careful, accounting for a situation that already caused fatal issues is a bit to much for me.

Flamefire · 2026-02-17T14:10:24Z

libs/s25main/nodeObjs/noShip.cpp

Misleading indentation, and check can be against "empty"
And we don't need to set the state (see check at top)

Suggested change

if(curRouteIdx == route_.size())

{

state = State::ExpeditionWaiting;

return;

} else

state = State::ExpeditionDriving;

StartDriving(route_[curRouteIdx++]);

if(!route_.empty())

{

state = State::ExpeditionDriving;

StartDriving(route_[curRouteIdx++]);

}

I also think we need to handle the "arrived" as an event to e.g. notify the player. Maybe simply:

Suggested change

if(curRouteIdx == route_.size())

{

state = State::ExpeditionWaiting;

return;

} else

state = State::ExpeditionDriving;

StartDriving(route_[curRouteIdx++]);

state = State::ExpeditionDriving;

HandleState_ExpeditionDriving();

I think you misunderstood: Simply replace the original StartDriving by HandleState_ExpeditionDriving without checking the route-index/size
This is already handled by HandleState_ExpeditionDriving so we can avoid the work here which is also very clean: Advance the state and call the associated state handler instead of duplicating some of its code

Ah, I see, so that the check is not needed here either.

Flamefire · 2026-02-17T14:16:56Z

libs/s25main/pathfinding/FreePathFinderImpl.h

Not sure where else this is used but I'd say an empty route is valid: It just doesn't change the current point, i.e. *dest = pos

Or would that cause issues anywhere?

Apart from the DriveToHarbourPlace, it seems to be also used in two functions for checking the Trade Route, being TradeRoute::GetNextDir (which checks the path route size before calling the function) and TradePathCache::pathExists, which I need check further.

Just checked loosely and found:

s25client/libs/s25main/world/TradeRoute.cpp

Lines 38 to 39 in 6e12273

if(world.CheckTradeRoute(curPos, path.route, curRouteIdx, player))

nextDir = path.route[curRouteIdx];

So when this returns true we expect route[pos] to be valid, which would not be the case when route is empty.
However RTTR_Assert(pos < route.size()); already assumes we have a valid index, as a precondition. And indeed we cannot have pos >= size and hence empty is never true, see the other discussion about handling situations after OOB access.
Hence we can remove this and ensure our assumption/assertion holds.

…pedition

…nDriving

Flamefire · 2026-02-18T13:36:20Z

Amazing how after all the changes to the changes and discussion the fix is a single line 😄
Thanks for the productive dialog!

I'll try to write a test in the next couple days to cover this. Did you try the original save/replay?

Noseey · 2026-02-18T15:13:56Z

I'll try to write a test in the next couple days to cover this. Did you try the original save/replay?

Sounds good!
Yes, I did check it with the provided savegames in both debug (commented out asserts) and release build:

https://github.com/user-attachments/files/21091348/auto_154x192.Auto-Save.zip does still crash, as we have removed the check for empty vectors in FreePathFinder::CheckRoute and there are still the stuck ships with positions != 0 on the map and flagged as lost from the previous OOB access. But like established above, with the current fix this should not lead to stuck ships in the first place.
https://github.com/user-attachments/files/25328542/antia-getting-stuck.zip does work now and it is no longer possible to get the ship "Antia" stuck when continuing the expedition from Harbor place 8 to 9.

Noseey added 5 commits February 16, 2026 14:01

Pathfinding: add check if route is not empty in FreePathFinder::Check…

583cc34

…Route to avoid crashes for lost and stuck ships.

Pathfinding: consider empty routes before starting expeditions

eda3660

Ships: check empty routes

b8aec9c

.. assuming we have already arrived in case of DriveToHarbourPlace and ContinueExpedition. Consider a route check to be false in case it is empty.

Ship: check route_.size against curRouteIdx instead of Empty

d26a789

Merge branch 'Return-To-The-Roots:master' into fix_ship_stuck_crash

aa19096

Flamefire requested changes Feb 17, 2026

View reviewed changes

Noseey added 6 commits February 17, 2026 18:38

Ships: rework checks if arrived in DriveToHarbourPlace and ContinueEx…

c515522

…pedition

Ship: rework ContinueExpedition to notify in case of early arrival

f32e8a6

Remove unnecessary checks

c01534f

Whitespace fix

aa39a7f

ContinueExpedition - exchange StartDriving with HandleState_Expeditio…

f4938aa

…nDriving

Whitespace fix

2294b7c

-    // Already arrived? (e. g. adjacent harbor)
-    if(curRouteIdx == route_.size())
-        return Result::GoalReached;
-    else
-        StartDriving(route_[curRouteIdx++]);
-    return Result::Driving;
+    // FindShipPathToHarbor above found we arrived already?
+    if(route_.empty())
+        return Result::GoalReached;
+    RTTR_Assert(curRouteIdx < route_.size();
+    StartDriving(route_[curRouteIdx++]);
+    return Result::Driving;

-    if(curRouteIdx == route_.size())
-    {
-        state = State::ExpeditionWaiting;
-        return;
-    } else
-        state = State::ExpeditionDriving;
-    StartDriving(route_[curRouteIdx++]);
+    if(!route_.empty())
+    {
+        state = State::ExpeditionDriving;
+        StartDriving(route_[curRouteIdx++]);
+    }

	if(world.CheckTradeRoute(curPos, path.route, curRouteIdx, player))
	nextDir = path.route[curRouteIdx];

Conversation

Noseey commented Feb 17, 2026

Uh oh!

Flamefire left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Flamefire commented Feb 18, 2026

Uh oh!

Noseey commented Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

Noseey commented Feb 18, 2026 •

edited

Loading