From c6b76a0b72695a30c6f2fedea889740b0534d586 Mon Sep 17 00:00:00 2001 From: jerrrrycho Date: Sat, 3 Oct 2026 22:33:13 +0200 Subject: [PATCH] fix(sonar): addressing reliability/security issues --- McZapkie/Mover.cpp | 14 +++++----- application/scenarioloaderuilayer.cpp | 2 +- launcher/deferred_image.h | 29 +++++--------------- launcher/vehicle_picker.cpp | 6 ++--- launcher/vehicle_picker.h | 2 +- vehicle/Driver.cpp | 29 +++++++++++--------- vehicle/Train.cpp | 39 +++++++++++---------------- 7 files changed, 49 insertions(+), 72 deletions(-) diff --git a/McZapkie/Mover.cpp b/McZapkie/Mover.cpp index 06358b3c..ff60c27c 100644 --- a/McZapkie/Mover.cpp +++ b/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; - OK = true; - if (EIMCtrlType == 0 && ( SpeedCtrlAutoTurnOffFlag & 1 ) == 1 && MainCtrlActualPos != MainCtrlPos) - { - DecScndCtrl(2); - SpeedCtrlUnit.IsActive = false; - } + DecScndCtrl(2); + SpeedCtrlUnit.IsActive = false; } break; } diff --git a/application/scenarioloaderuilayer.cpp b/application/scenarioloaderuilayer.cpp index c4e7df3d..de0d4e64 100644 --- a/application/scenarioloaderuilayer.cpp +++ b/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); diff --git a/launcher/deferred_image.h b/launcher/deferred_image.h index 29a43075..85582264 100644 --- a/launcher/deferred_image.h +++ b/launcher/deferred_image.h @@ -1,7 +1,6 @@ #pragma once #include -#include #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() = 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; - } + deferred_image(deferred_image&&) = default; + deferred_image &operator=(deferred_image&&) = default; 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 - deferred_image(deferred_image &&other, std::scoped_lock const &) noexcept : path(std::move(other.path)), image(other.image) { } - - // guards lazy loading performed by the const accessors - mutable std::mutex mutex; - mutable std::string path; - mutable texture_handle image = 0; + std::string path; + texture_handle image = 0; }; diff --git a/launcher/vehicle_picker.cpp b/launcher/vehicle_picker.cpp index 58a11f59..9fe0a29a 100644 --- a/launcher/vehicle_picker.cpp +++ b/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 skinset_list; + std::vector skinset_list; if (display_by_groups) { std::vector 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 selected_vehicle; diff --git a/vehicle/Driver.cpp b/vehicle/Driver.cpp index b3a69ad2..4437950b 100644 --- a/vehicle/Driver.cpp +++ b/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 (currentorder != Wait_for_orders) + if (auto const currentorder { OrderCurrentGet() }; 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] ? - OrderList[OrderTop] < Shunt && OrderList[OrderTop] != NewOrder : - false) // jeśli coś robi + while (OrderTop >= 0 && OrderTop < maxorders - 1 + && OrderList[OrderTop] != Wait_for_orders && OrderList[OrderTop] < Shunt && OrderList[OrderTop] != NewOrder) // 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= 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 (dir.x * sem.x + dir.z * sem.z < 0) + 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 { // 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 } diff --git a/vehicle/Train.cpp b/vehicle/Train.cpp index fe153cae..a77c5749 100644 --- a/vehicle/Train.cpp +++ b/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 +#include #include #include #include @@ -647,9 +647,8 @@ bool TTrain::Init(TDynamicObject *NewDynamicObject, bool e3d) } DynamicSet(NewDynamicObject); - if (!e3d) - if (DynamicObject->Mechanik == nullptr) - return false; + if (!e3d && DynamicObject->Mechanik == nullptr) + 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 -void update_trains(std::deque const &Trains, std::size_t const Start, std::size_t const End, double const Dt, std::exception_ptr &Error) +// updates trains in range [Start, End) +void update_trains(std::deque 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); - } - } - catch (std::exception const &) - { - Error = std::current_exception(); + if (Trains[j]) + Trains[j]->Update(Dt); } } } // namespace @@ -12426,8 +12418,9 @@ void train_table::updateAsync(double dt) const size_t chunkSize = (total + threads - 1) / threads; std::vector workers; - std::vector errors(threads); + std::vector> 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 - workers.emplace_back(update_trains, std::cref(m_items), start, end, dt, std::ref(errors[i])); + std::packaged_task task([this, start, end, dt]() { update_trains(m_items, start, end, dt); }); + 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) - { - if (error) - std::rethrow_exception(error); - } + for (auto &result : results) + result.get(); // Teraz kasowanie (tylko w głównym wątku) for (TTrain *train : m_items)