fix(sonar): addressing reliability/security issues

McZapkie/Mover.cpp

@@ -2408,16 +2408,14 @@ bool TMoverParameters::IncMainCtrl(int CtrlSpeed)
 			if (CtrlSpeed > 1)
 			{
 				OK = IncMainCtrl(1) && IncMainCtrl(CtrlSpeed - 1); // a fail will propagate up the recursion chain. should this be || instead?
+				break;
 			}
-			else
+			++MainCtrlPos;
+			OK = true;
+			if (EIMCtrlType == 0 && ( SpeedCtrlAutoTurnOffFlag & 1 ) == 1 && MainCtrlActualPos != MainCtrlPos)
 			{
-				++MainCtrlPos;
+				DecScndCtrl(2);
-				OK = true;
+				SpeedCtrlUnit.IsActive = false;
-				if (EIMCtrlType == 0 && ( SpeedCtrlAutoTurnOffFlag & 1 ) == 1 && MainCtrlActualPos != MainCtrlPos)
-				{
-					DecScndCtrl(2);
-					SpeedCtrlUnit.IsActive = false;
-				}
 			}
 			break;
 		}

application/scenarioloaderuilayer.cpp

@@ -213,7 +213,7 @@ void scenarioloader_ui::render_()
 	const float spacing = 10.0f * scale; // odstęp między ikoną a tekstem
 
 	// Loading icon
-	const deferred_image *img = &m_loading_wheel_frames[38];
+	deferred_image *img = &m_loading_wheel_frames[38];
 	const auto loading_tex = img->get();
 	const auto loading_size = glm::vec2(img->size()) * glm::vec2(scale);
 

launcher/deferred_image.h

@@ -1,7 +1,6 @@
 #pragma once
 
 #include <limits>
-#include <mutex>
 
 #include "model/Texture.h"
 #include "rendering/renderer.h"
@@ -14,26 +13,16 @@ public:
 	deferred_image() = default;
 	deferred_image(const std::string &p) : path(p) { }
 	deferred_image(const deferred_image&) = delete;
-	deferred_image(deferred_image &&other) noexcept : deferred_image(std::move(other), std::scoped_lock(other.mutex)) { }
+	deferred_image(deferred_image&&) = default;
-	~deferred_image() = default;
+	deferred_image &operator=(deferred_image&&) = default;
-	deferred_image &operator=(deferred_image &&other) noexcept
-	{
-		if (this != &other) {
-			std::scoped_lock lock(mutex, other.mutex);
-			path = std::move(other.path);
-			image = other.image;
-		}
-		return *this;
-	}
 	operator bool() const
 	{
-		std::scoped_lock lock(mutex);
 		return image != null_handle || !path.empty();
 	}
 
-	GLuint get() const
+	// loads the texture on first use
+	GLuint get()
 	{
-		std::scoped_lock lock(mutex);
 		if (!path.empty()) {
             image = GfxRenderer->Fetch_Texture(path, true);
 			path.clear();
@@ -52,7 +41,6 @@ public:
 
 	glm::ivec2 size() const
 	{
-		std::scoped_lock lock(mutex);
 		if (image != null_handle) {
             auto &tex = GfxRenderer->Texture(image);
 			return glm::ivec2(tex.get_width(), tex.get_height());
@@ -61,11 +49,6 @@ public:
 	}
 
 private:
-	// moves content of the other image, while the caller holds its lock
+	std::string path;
-	deferred_image(deferred_image &&other, std::scoped_lock<std::mutex> const &) noexcept : path(std::move(other.path)), image(other.image) { }
+	texture_handle image = 0;
-
-	// guards lazy loading performed by the const accessors
-	mutable std::mutex mutex;
-	mutable std::string path;
-	mutable texture_handle image = 0;
 };

launcher/vehicle_picker.cpp

@@ -37,7 +37,7 @@ void ui::vehiclepicker_panel::render_contents()
 	if (ImGui::RadioButton(STR_C("texture group"), display_by_groups))
 		display_by_groups = true;
 
-	std::vector<const skin_set*> skinset_list;
+	std::vector<skin_set*> skinset_list;
 
 	if (display_by_groups) {
 		std::vector<const std::string*> model_list;
@@ -114,7 +114,7 @@ void ui::vehiclepicker_panel::render_contents()
 				for (int i = clipper.DisplayStart; i < clipper.DisplayEnd; i++) {
 					auto &desc = model_list[i];
 
-					const deferred_image *image = nullptr;
+					deferred_image *image = nullptr;
 					for (auto const &skinset : desc->matching_skinsets) {
 						if (skinset->mini) {
 							image = &skinset->mini;
@@ -256,7 +256,7 @@ bool ui::vehiclepicker_panel::skin_filter(const skin_set *skin, std::vector<sear
 	return alternative_present ? !any : false;
 }
 
-bool ui::vehiclepicker_panel::selectable_image(const char *desc, bool selected, const deferred_image* image, const skin_set *pickable)
+bool ui::vehiclepicker_panel::selectable_image(const char *desc, bool selected, deferred_image *image, const skin_set *pickable)
 {
 	ImGui::PushID(pickable);
 

launcher/vehicle_picker.h

@@ -14,7 +14,7 @@ class vehiclepicker_panel : public ui_panel
 	void render_contents() override;
 
 private:
-	bool selectable_image(const char *desc, bool selected, const deferred_image *image, const skin_set *pickable = nullptr);
+	bool selectable_image(const char *desc, bool selected, deferred_image *image, const skin_set *pickable = nullptr);
 
 	vehicle_type selected_type = vehicle_type::none;
 	std::shared_ptr<const vehicle_desc> selected_vehicle;

vehicle/Driver.cpp

@@ -4987,8 +4987,7 @@ TController::PrepareDirection() {
 
 void TController::JumpToNextOrder( bool const Ignoremergedchangedirection )
 { // wykonanie kolejnej komendy z tablicy rozkazów
-    auto const currentorder { OrderCurrentGet() };
+    if (auto const currentorder { OrderCurrentGet() }; currentorder != Wait_for_orders)
-    if (currentorder != Wait_for_orders)
     {
         if( (currentorder & Change_direction) != 0 // jeśli zmiana kierunku
 		    && currentorder != Change_direction && false == Ignoremergedchangedirection ) { // ale nałożona na coś
@@ -5056,16 +5055,21 @@ void TController::OrderNext(TOrders NewOrder)
     OrderTop = OrderPos; // ale może jest czymś zajęty na razie
     if (NewOrder >= Shunt) // jeśli ma jechać
     { // ale może być zajęty chwilowymi operacjami
-        while (OrderList[OrderTop] != Wait_for_orders && OrderList[OrderTop] < Shunt) // jeśli coś robi
+        while (OrderTop >= 0 && OrderTop < maxorders - 1
+            && OrderList[OrderTop] != Wait_for_orders && OrderList[OrderTop] < Shunt) // jeśli coś robi
             ++OrderTop; // pomijamy wszystkie tymczasowe prace
     }
     else
     { // jeśli ma ustawioną jazdę, to wyłączamy na rzecz operacji
-        while (OrderList[OrderTop] ?
+        while (OrderTop >= 0 && OrderTop < maxorders - 1
-                   OrderList[OrderTop] < Shunt && OrderList[OrderTop] != NewOrder :
+            && OrderList[OrderTop] != Wait_for_orders && OrderList[OrderTop] < Shunt && OrderList[OrderTop] != NewOrder) // jeśli coś robi
-                   false) // jeśli coś robi
             ++OrderTop; // pomijamy wszystkie tymczasowe prace
     }
+    if (OrderTop < 0 || OrderTop >= maxorders)
+    {
+        ErrorLog("Commands overflow: order \"" + Order2Str(NewOrder) + "\" dropped");
+        return;
+    }
     OrderList[OrderTop++] = NewOrder; // dodanie rozkazu jako następnego
 #if LOGORDERS
     OrdersDump( "OrderNext" ); // normalnie nie ma po co tego wypisywać
@@ -5078,11 +5082,13 @@ void TController::OrderPush(TOrders NewOrder)
 	                                                         // ale nie jedzie
 		++OrderTop;
 	// niektóre operacje muszą zostać najpierw dokończone => zapis na kolejnej
+    if (OrderTop < 0 || OrderTop >= maxorders)
+    {
+        ErrorLog("Commands overflow: order \"" + Order2Str(NewOrder) + "\" dropped");
+        return;
+    }
     if (OrderList[OrderTop] != NewOrder) // jeśli jest to samo, to nie dodajemy
         OrderList[OrderTop++] = NewOrder; // dodanie rozkazu na stos
-    // if (OrderTop<OrderPos) OrderTop=OrderPos;
-    if (OrderTop >= maxorders)
-        ErrorLog("Commands overflow: The program will now crash");
 #if LOGORDERS
     OrdersDump( "OrderPush: [" + Order2Str( NewOrder ) + "]" ); // normalnie nie ma po co tego wypisywać
 #endif
@@ -5398,8 +5404,7 @@ TCommandType TController::BackwardScan( double const Range )
         // najpierw sprawdzamy, czy semafor czy inny znak został przejechany
         auto const sl{e->input_location()}; // położenie komórki pamięci
         auto const pos{pVehicles[end::rear]->RearPosition()}; // pozycja tyłu
-        auto const sem{sl - pos}; // wektor do komórki pamięci od końca składu
+        if (auto const sem{sl - pos}; dir.x * sem.x + dir.z * sem.z < 0) // wektor do komórki pamięci od końca składu
-        if (dir.x * sem.x + dir.z * sem.z < 0)
         {
             // jeśli został minięty
             // iloczyn skalarny jest ujemny, gdy sygnał stoi z tyłu
@@ -5411,7 +5416,7 @@ TCommandType TController::BackwardScan( double const Range )
         scanvel = e->input_value(1); // prędkość przy tym semaforze
 #if LOGBACKSCAN
         // przeliczamy odległość od semafora - potrzebne by były współrzędne początku składu
-        scandist = std::max(glm::length(sem) - 2.0, 0.0); // 2m luzu przy manewrach wystarczy, ujemnych nie ma po co wysyłać
+        scandist = std::max(glm::length(sl - pos) - 2.0, 0.0); // 2m luzu przy manewrach wystarczy, ujemnych nie ma po co wysyłać
 #endif
     }
 

vehicle/Train.cpp

@@ -30,7 +30,7 @@ http://mozilla.org/MPL/2.0/.
 #include "Console.h"
 #include "application/application.h"
 #include "rendering/renderer.h"
-#include <exception>
+#include <future>
 #include <thread>
 #include <cmath>
 #include <algorithm>
@@ -647,9 +647,8 @@ bool TTrain::Init(TDynamicObject *NewDynamicObject, bool e3d)
 	}
 
 	DynamicSet(NewDynamicObject);
-	if (!e3d)
+	if (!e3d && DynamicObject->Mechanik == nullptr)
-		if (DynamicObject->Mechanik == nullptr)
+		return false;
-			return false;
 
 	DynamicObject->MechInside = true;
 
@@ -12401,20 +12400,13 @@ uint16_t TTrain::id()
 
 namespace
 {
-// updates trains in range [Start, End); an exception is stored for the calling thread to rethrow
+// updates trains in range [Start, End)
-void update_trains(std::deque<TTrain *> const &Trains, std::size_t const Start, std::size_t const End, double const Dt, std::exception_ptr &Error)
+void update_trains(std::deque<TTrain *> const &Trains, std::size_t const Start, std::size_t const End, double const Dt)
 {
-	try
+	for (std::size_t j = Start; j < End; ++j)
 	{
-		for (std::size_t j = Start; j < End; ++j)
+		if (Trains[j])
-		{
+			Trains[j]->Update(Dt);
-			if (Trains[j])
-				Trains[j]->Update(Dt);
-		}
-	}
-	catch (std::exception const &)
-	{
-		Error = std::current_exception();
 	}
 }
 } // namespace
@@ -12426,8 +12418,9 @@ void train_table::updateAsync(double dt)
 	const size_t chunkSize = (total + threads - 1) / threads;
 
 	std::vector<std::jthread> workers;
-	std::vector<std::exception_ptr> errors(threads);
+	std::vector<std::future<void>> results; // przekazują wyjątki z wątków do głównego wątku
 	workers.reserve(threads);
+	results.reserve(threads);
 
 	for (int i = 0; i < threads; ++i)
 	{
@@ -12437,19 +12430,17 @@ void train_table::updateAsync(double dt)
 		if (start >= end)
 			break; // brak więcej danych
 
-		// wyjątek przekazywany jest do głównego wątku
+		std::packaged_task<void()> task([this, start, end, dt]() { update_trains(m_items, start, end, dt); });
-		workers.emplace_back(update_trains, std::cref(m_items), start, end, dt, std::ref(errors[i]));
+		results.emplace_back(task.get_future());
+		workers.emplace_back(std::move(task));
 	}
 
 	// Poczekaj aż wszystkie wątki skończą
 	for (auto &worker : workers)
 		worker.join();
 
-	for (auto const &error : errors)
+	for (auto &result : results)
-	{
+		result.get();
-		if (error)
-			std::rethrow_exception(error);
-	}
 
 	// Teraz kasowanie (tylko w głównym wątku)
 	for (TTrain *train : m_items)