Fix: Check for engine variant loops during NewGRF initialization. (#11343)

Invalid NewGRFs could set up an engine variant loop that never ends. This
was checked for in some places that evaluated variants, but not all. In
most cases this would result in the engines not appearing, but could
potentially cause an infinite loop and crash.

Instead, during NewGRF initialization detect loops and remove invalid
variants before setting display flags.

src/airport_gui.cpp

@@ -544,13 +544,14 @@ public:
 		}
 		if (change_class) {
 			/* If that fails, select the first available airport
-			 * from a random class. */
+			 * from the first class where airports are available. */
 			for (AirportClassID j = APC_BEGIN; j < APC_MAX; j++) {
 				AirportClass *apclass = AirportClass::Get(j);
 				for (uint i = 0; i < apclass->GetSpecCount(); i++) {
 					const AirportSpec *as = apclass->GetSpec(i);
 					if (as->IsAvailable()) {
 						_selected_airport_class = j;
+						this->vscroll->SetCount(apclass->GetSpecCount());
 						this->SelectOtherAirport(i);
 						return;
 					}

src/build_vehicle_gui.cpp

@@ -1422,7 +1422,7 @@ struct BuildVehicleWindow : Window {
 			list.emplace_back(eid, e->info.variant_id, e->display_flags, 0);
 
 			if (rvi->railveh_type != RAILVEH_WAGON) num_engines++;
-			if (e->info.variant_id != eid && e->info.variant_id != INVALID_ENGINE) variants.push_back(e->info.variant_id);
+			if (e->info.variant_id != INVALID_ENGINE) variants.push_back(e->info.variant_id);
 			if (eid == this->sel_engine) sel_id = eid;
 		}
 
@@ -1654,13 +1654,14 @@ struct BuildVehicleWindow : Window {
 						Command<CMD_BUILD_VEHICLE>::Post(GetCmdBuildVehMsg(this->vehicle_type), CcBuildPrimaryVehicle, this->window_number, sel_eng, true, cargo, INVALID_CLIENT_ID);
 					}
 
-					/* Update last used variant and refresh if necessary. */
+					/* Update last used variant in hierarchy and refresh if necessary. */
 					bool refresh = false;
-					int recursion = 10; /* In case of infinite loop */
+					EngineID parent = sel_eng;
-					for (Engine *e = Engine::Get(sel_eng); recursion > 0; e = Engine::Get(e->info.variant_id), --recursion) {
+					while (parent != INVALID_ENGINE) {
+						Engine *e = Engine::Get(parent);
 						refresh |= (e->display_last_variant != sel_eng);
 						e->display_last_variant = sel_eng;
-						if (e->info.variant_id == INVALID_ENGINE) break;
+						parent = e->info.variant_id;
 					}
 					if (refresh) {
 						InvalidateWindowData(WC_REPLACE_VEHICLE, this->vehicle_type, 0); // Update the autoreplace window

src/engine.cpp

@@ -491,7 +491,7 @@ bool Engine::IsVariantHidden(CompanyID c) const
 	 * the last display variant rather than the actual parent variant. */
 	const Engine *re = this;
 	const Engine *ve = re->GetDisplayVariant();
-	while (!(ve->IsHidden(c)) && re->info.variant_id != INVALID_ENGINE && re->info.variant_id != re->index) {
+	while (!(ve->IsHidden(c)) && re->info.variant_id != INVALID_ENGINE) {
 		re = Engine::Get(re->info.variant_id);
 		ve = re->GetDisplayVariant();
 	}
@@ -607,7 +607,7 @@ void CalcEngineReliability(Engine *e, bool new_month)
 {
 	/* Get source engine for reliability age. This is normally our engine unless variant reliability syncing is requested. */
 	Engine *re = e;
-	while (re->info.variant_id != INVALID_ENGINE && re->info.variant_id != re->index && (re->info.extra_flags & ExtraEngineFlags::SyncReliability) != ExtraEngineFlags::None) {
+	while (re->info.variant_id != INVALID_ENGINE && (re->info.extra_flags & ExtraEngineFlags::SyncReliability) != ExtraEngineFlags::None) {
 		re = Engine::Get(re->info.variant_id);
 	}
 
@@ -706,7 +706,7 @@ void StartupOneEngine(Engine *e, TimerGameCalendar::Date aging_date, uint32_t se
 
 	/* Get parent variant index for syncing reliability via random seed. */
 	const Engine *re = e;
-	while (re->info.variant_id != INVALID_ENGINE && re->info.variant_id != re->index && (re->info.extra_flags & ExtraEngineFlags::SyncReliability) != ExtraEngineFlags::None) {
+	while (re->info.variant_id != INVALID_ENGINE && (re->info.extra_flags & ExtraEngineFlags::SyncReliability) != ExtraEngineFlags::None) {
 		re = Engine::Get(re->info.variant_id);
 	}
 

src/newgrf.cpp

@@ -9125,12 +9125,9 @@ static void FinaliseEngineArray()
 			}
 		}
 
-		/* Do final mapping on variant engine ID and set appropriate flags on variant engine */
+		/* Do final mapping on variant engine ID. */
 		if (e->info.variant_id != INVALID_ENGINE) {
 			e->info.variant_id = GetNewEngineID(e->grf_prop.grffile, e->type, e->info.variant_id);
-			if (e->info.variant_id != INVALID_ENGINE) {
-				Engine::Get(e->info.variant_id)->display_flags |= EngineDisplayFlags::HasVariants | EngineDisplayFlags::IsFolded;
-			}
 		}
 
 		if (!HasBit(e->info.climates, _settings_game.game_creation.landscape)) continue;
@@ -9162,6 +9159,26 @@ static void FinaliseEngineArray()
 			}
 		}
 	}
+
+	/* Check engine variants don't point back on themselves (either directly or via a loop) then set appropriate flags
+	 * on variant engine. This is performed separately as all variant engines need to have been resolved. */
+	for (Engine *e : Engine::Iterate()) {
+		EngineID parent = e->info.variant_id;
+		while (parent != INVALID_ENGINE) {
+			parent = Engine::Get(parent)->info.variant_id;
+			if (parent != e->index) continue;
+
+			/* Engine looped back on itself, so clear the variant. */
+			e->info.variant_id = INVALID_ENGINE;
+
+			GrfMsg(1, "FinaliseEngineArray: Variant of engine {:x} in '{}' loops back on itself", _engine_mngr[e->index].internal_id, e->GetGRF()->filename);
+			break;
+		}
+
+		if (e->info.variant_id != INVALID_ENGINE) {
+			Engine::Get(e->info.variant_id)->display_flags |= EngineDisplayFlags::HasVariants | EngineDisplayFlags::IsFolded;
+		}
+	}
 }
 
 /** Check for invalid cargoes */