From 3ab3cdeb2dc392e5b903c057af6af1f25fd0f54f Mon Sep 17 00:00:00 2001 From: jerrrrycho Date: Sat, 3 Oct 2026 21:52:10 +0200 Subject: [PATCH] fix(sonar): addressing reliability/security issues --- .github/workflows/build.yml | 6 ++-- .github/workflows/build_bin.yml | 14 ++++----- .github/workflows/sonar.yml | 38 ++++++++++++------------ McZapkie/hamulce.cpp | 10 +++---- launcher/deferred_image.h | 17 +++++------ network/message.h | 22 +++++++------- utilities/Logs.cpp | 4 +-- vehicle/Driver.cpp | 52 ++++++++++++++++----------------- vehicle/Driver.h | 8 +++++ vehicle/Train.cpp | 47 ++++++++++++++--------------- 10 files changed, 111 insertions(+), 107 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6e4a74ba..c9ce73bd 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -23,13 +23,13 @@ jobs: env: BUILD_WRAPPER_OUT_DIR: bw-output steps: - - uses: actions/checkout@v5 + - uses: actions/checkout@fbc6f3992d24b796d5a048ff273f7fcc4a7b6c09 # v5.1.0 with: fetch-depth: 0 submodules: recursive - name: Set up Python 3.14 - uses: actions/setup-python@v6 + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 with: python-version: '3.14' @@ -106,7 +106,7 @@ jobs: -C "$GITHUB_WORKSPACE" . - name: Upload analysis input - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: sonar-input path: /tmp/workspace.tar.gz diff --git a/.github/workflows/build_bin.yml b/.github/workflows/build_bin.yml index f0b1fdec..ac1fa562 100644 --- a/.github/workflows/build_bin.yml +++ b/.github/workflows/build_bin.yml @@ -33,14 +33,14 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v6 + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 with: fetch-depth: 0 submodules: recursive - name: Setup Python 3.14 id: python - uses: actions/setup-python@v6 + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 with: python-version: "3.14" architecture: "x64" @@ -104,7 +104,7 @@ jobs: -Force - name: Upload Windows build - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: maszyna-windows-x64-${{ env.CONFIG_WIN }} path: | @@ -113,7 +113,7 @@ jobs: retention-days: 14 - name: Upload Windows debug symbols - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: maszyna-windows-x64-symbols-${{ env.CONFIG_WIN }} path: | @@ -133,14 +133,14 @@ jobs: steps: - name: Checkout repository - uses: actions/checkout@v6 + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.1.0 with: fetch-depth: 0 submodules: recursive - name: Setup Python 3.14 id: python - uses: actions/setup-python@v6 + uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0 with: python-version: "3.14" @@ -244,7 +244,7 @@ jobs: cp -R shaders/. build/bin/shaders/ - name: Upload Linux build - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 with: name: maszyna-linux-x64-${{ env.CONFIG_LINUX }} path: | diff --git a/.github/workflows/sonar.yml b/.github/workflows/sonar.yml index 6916872c..b2951189 100644 --- a/.github/workflows/sonar.yml +++ b/.github/workflows/sonar.yml @@ -64,15 +64,13 @@ jobs: if: github.event.action == 'completed' && github.event.workflow_run.conclusion == 'success' runs-on: ubuntu-24.04 permissions: - contents: read # checkout trusted config + contents: read # fetch trusted config actions: read # download artifact from the Build run statuses: write # post the commit status pull-requests: read # verify PR head sha via the API env: BUILD_WRAPPER_OUT_DIR: bw-output - # .trusted/ holds the checkout of master used to restore the scanner config; - # it is not part of the analysed change. - SONAR_EXCLUSIONS: betterRenderer/**,ref/**,imgui/**,stb/**,build/**,bw-output/**,.trusted/** + SONAR_EXCLUSIONS: betterRenderer/**,ref/**,imgui/**,stb/**,build/**,bw-output/** SONAR_SCANNER_JAVA_OPTS: -Xmx5g steps: - name: Post pending status @@ -85,17 +83,23 @@ jobs: -f description="Analysis in progress" \ -f target_url="$RUN_URL" - # Trusted copy of the repo config, used to overwrite anything the PR - # might have tampered with (sonar-project.properties etc). - - name: Checkout trusted config from master - uses: actions/checkout@v5 - with: - ref: master - path: .trusted - persist-credentials: false + # Trusted copy of the scanner config, used to overwrite anything the PR + # might have tampered with. Fetched as a single file through the API rather + # than with actions/checkout: nothing gets checked out in this privileged + # context. Kept outside the workspace, so the archive can't overwrite it. + - name: Fetch trusted config from master + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + set -euo pipefail + url="repos/${{ github.repository }}/contents/sonar-project.properties?ref=master" + if ! gh api -H "Accept: application/vnd.github.raw" "$url" > /tmp/trusted-sonar-project.properties; then + echo "::warning::no sonar-project.properties on master" + rm -f /tmp/trusted-sonar-project.properties + fi - name: Download analysis input - uses: actions/download-artifact@v4 + uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 with: name: sonar-input path: /tmp/sonar-input @@ -105,13 +109,7 @@ jobs: - name: Unpack analysis input run: | set -euo pipefail - # The archive holds the whole PR workspace and is unpacked over this one, - # .trusted/ included. Move the trusted config out of reach first, or a PR - # carrying its own .trusted/sonar-project.properties would have it planted - # here and restored below as if it came from master. - if [ -f "$GITHUB_WORKSPACE/.trusted/sonar-project.properties" ]; then - cp "$GITHUB_WORKSPACE/.trusted/sonar-project.properties" /tmp/trusted-sonar-project.properties - fi + # The archive holds the whole PR workspace and is unpacked over this one. # Nothing may be written outside the workspace: reject absolute members and # any member reaching through a parent directory before unpacking anything. tar -tzf /tmp/sonar-input/workspace.tar.gz > /tmp/archive-members.txt diff --git a/McZapkie/hamulce.cpp b/McZapkie/hamulce.cpp index ad75bac4..64a7fb85 100644 --- a/McZapkie/hamulce.cpp +++ b/McZapkie/hamulce.cpp @@ -3543,10 +3543,9 @@ double TMHZ_K5P::GetPF(double i_bcp, double PP, double HP, double dt, double ep) else dpMainValve = PFVd(PP, 0, ActFlowSpeed / LBDelay, dpPipe, 0.4); - if ((EQ(i_bcp, -1) && AutoOvrld) || (i_bcp < 0.5 && UniversalFlag & TUniversalBrake::ub_Overload)) + if (((EQ(i_bcp, -1) && AutoOvrld) || (i_bcp < 0.5 && UniversalFlag & TUniversalBrake::ub_Overload)) && TP < OverloadMaxPressure) { - if (TP < OverloadMaxPressure) - TP = TP + 0.03 * dt; + TP = TP + 0.03 * dt; } if (EQ(i_bcp, 3)) @@ -3723,10 +3722,9 @@ double TMHZ_6P::GetPF(double i_bcp, double PP, double HP, double dt, double ep) else dpMainValve = PFVd(PP, 0, ActFlowSpeed / LBDelay, dpPipe, 0.4); - if ((EQ(i_bcp, -1) && AutoOvrld) || (i_bcp < 0.5 && UniversalFlag & TUniversalBrake::ub_Overload)) + if (((EQ(i_bcp, -1) && AutoOvrld) || (i_bcp < 0.5 && UniversalFlag & TUniversalBrake::ub_Overload)) && TP < OverloadMaxPressure) { - if (TP < OverloadMaxPressure) - TP = TP + 0.03 * dt; + TP = TP + 0.03 * dt; } if (EQ(i_bcp, 4)) diff --git a/launcher/deferred_image.h b/launcher/deferred_image.h index 2ad6ec9f..29a43075 100644 --- a/launcher/deferred_image.h +++ b/launcher/deferred_image.h @@ -14,12 +14,8 @@ public: deferred_image() = default; deferred_image(const std::string &p) : path(p) { } deferred_image(const deferred_image&) = delete; - deferred_image(deferred_image &&other) noexcept - { - std::lock_guard lock(other.mutex); - path = std::move(other.path); - image = other.image; - } + 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) { @@ -31,13 +27,13 @@ public: } operator bool() const { - std::lock_guard lock(mutex); + std::scoped_lock lock(mutex); return image != null_handle || !path.empty(); } GLuint get() const { - std::lock_guard lock(mutex); + std::scoped_lock lock(mutex); if (!path.empty()) { image = GfxRenderer->Fetch_Texture(path, true); path.clear(); @@ -56,7 +52,7 @@ public: glm::ivec2 size() const { - std::lock_guard lock(mutex); + std::scoped_lock lock(mutex); if (image != null_handle) { auto &tex = GfxRenderer->Texture(image); return glm::ivec2(tex.get_width(), tex.get_height()); @@ -65,6 +61,9 @@ 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; diff --git a/network/message.h b/network/message.h index 979c69ce..5aa07d78 100644 --- a/network/message.h +++ b/network/message.h @@ -25,22 +25,22 @@ struct message struct client_hello : public message { - client_hello() : message(CLIENT_HELLO), version(0), start_packet(0) {} + client_hello() : message(CLIENT_HELLO) {} virtual void serialize(std::ostream &stream) const override; virtual void deserialize(std::istream &stream) override; - int32_t version; - uint32_t start_packet; + int32_t version{0}; + uint32_t start_packet{0}; }; struct server_hello : public message { - server_hello() : message(SERVER_HELLO), seed(0), timestamp(0), config(0) {} + server_hello() : message(SERVER_HELLO) {} - uint32_t seed; - int64_t timestamp; - int64_t config; + uint32_t seed{0}; + int64_t timestamp{0}; + int64_t config{0}; std::string scenario; virtual void serialize(std::ostream &stream) const override; @@ -60,11 +60,11 @@ struct request_command : public message struct frame_info : public request_command { - frame_info() : request_command(FRAME_INFO), render_dt(0.0), dt(0.0), sync(0.0) {} + frame_info() : request_command(FRAME_INFO) {} - double render_dt; - double dt; - double sync; + double render_dt{0.0}; + double dt{0.0}; + double sync{0.0}; virtual void serialize(std::ostream &stream) const override; virtual void deserialize(std::istream &stream) override; diff --git a/utilities/Logs.cpp b/utilities/Logs.cpp index 3a0a3d08..5b4d1d9a 100644 --- a/utilities/Logs.cpp +++ b/utilities/Logs.cpp @@ -89,7 +89,7 @@ void LogService() std::string msg; bool isError; { - std::lock_guard lock(logMutex); + std::scoped_lock lock(logMutex); msg = InfoStack.front().first; isError = InfoStack.front().second; InfoStack.pop_front(); @@ -127,7 +127,7 @@ void LogService() { std::string msg; { - std::lock_guard lock(logMutex); + std::scoped_lock lock(logMutex); msg = ErrorStack.front(); ErrorStack.pop_front(); } diff --git a/vehicle/Driver.cpp b/vehicle/Driver.cpp index 48bf0993..b3a69ad2 100644 --- a/vehicle/Driver.cpp +++ b/vehicle/Driver.cpp @@ -2320,7 +2320,8 @@ bool TController::CheckVehicles(TOrders user) p = pVehicle->FirstFind(d); // pojazd na czele składu if (!p) p = pVehicle; - pVehicles[end::front] = p; + auto *const frontvehicle { p }; // stays the consist head for the rest of this check + pVehicles[end::front] = frontvehicle; // liczenie pojazdów w składzie i ustalenie parametrów auto dir = d = 1 - d; // a dalej będziemy zliczać od czoła do tyłu fLength = 0.0; // długość składu do badania wyjechania za ograniczenie @@ -2369,7 +2370,7 @@ bool TController::CheckVehicles(TOrders user) if (iDrivigFlags & movePrimary) { // jeśli jest aktywnie prowadzącym pojazd, może zrobić własny porządek auto pantmask = 1; - p = pVehicles[end::front]; + p = frontvehicle; // establish ownership and vehicle order while (p) { @@ -2384,8 +2385,8 @@ bool TController::CheckVehicles(TOrders user) p = p->Next(); // pojazd podłączony od tyłu (licząc od czoła) } // with the order established the virtual train manager can do their work - p = pVehicles[ end::front ]; - ControlledEnginesCount = ( p != nullptr && p->MoverParameters->Power > 1.0 ) ? 1 : 0; + p = frontvehicle; + ControlledEnginesCount = p->MoverParameters->Power > 1.0 ? 1 : 0; auto hasheaters { false }; while (p) { @@ -2457,7 +2458,7 @@ bool TController::CheckVehicles(TOrders user) } // detect push-pull train configurations and mark them accordingly - if( pVehicles[ end::front ]->is_connected( pVehicles[ end::rear ], coupling::control ) ) { + if( frontvehicle->is_connected( pVehicles[ end::rear ], coupling::control ) ) { // zmiana czoła przez zmianę kabiny iDrivigFlags |= movePushPull; } @@ -2482,7 +2483,7 @@ bool TController::CheckVehicles(TOrders user) } else { // jak dociska - pVehicles[end::front]->RaLightsSet(0, -1); + frontvehicle->RaLightsSet(0, -1); } } // enable door locks @@ -2493,7 +2494,7 @@ bool TController::CheckVehicles(TOrders user) // TODO: replace connection test with connection check between last engine and first car, specifically auto const isheatingcouplingactive { ( ControlledEnginesCount == 1 ? - pVehicles[ end::front ]->is_connected( pVehicles[ end::rear ], coupling::heating ) : + frontvehicle->is_connected( pVehicles[ end::rear ], coupling::heating ) : true ) }; auto const isheatingneeded { (is_emu() || is_dmu() ? true : @@ -4544,7 +4545,7 @@ bool TController::PutCommand( std::string NewCommand, double NewValue1, double N if (NewValue1 > 0.0 ? NewValue1 > fStopTime : false) fStopTime = NewValue1; // Ra: włączenie czekania bez zmiany komendy else - OrderList[OrderPos] = Wait_for_orders; // czekanie na komendę (albo dać OrderPos=0) + OrderCurrentSet( Wait_for_orders ); // czekanie na komendę (albo dać OrderPos=0) return true; } @@ -4986,13 +4987,14 @@ TController::PrepareDirection() { void TController::JumpToNextOrder( bool const Ignoremergedchangedirection ) { // wykonanie kolejnej komendy z tablicy rozkazów - if (OrderList[OrderPos] != Wait_for_orders) + auto const currentorder { OrderCurrentGet() }; + if (currentorder != Wait_for_orders) { - if( (OrderList[OrderPos] & Change_direction) != 0 // jeśli zmiana kierunku - && OrderList[OrderPos] != Change_direction && false == Ignoremergedchangedirection ) { // ale nałożona na coś + if( (currentorder & Change_direction) != 0 // jeśli zmiana kierunku + && currentorder != Change_direction && false == Ignoremergedchangedirection ) { // ale nałożona na coś // usunięcie zmiany kierunku z innej komendy - OrderList[OrderPos] = TOrders(OrderList[OrderPos] & ~Change_direction); + OrderCurrentSet( TOrders(currentorder & ~Change_direction) ); OrderCheck(); return; } @@ -5032,21 +5034,22 @@ void TController::OrderCheck() // HACK: ensure consist doors will be closed on departure iDrivigFlags |= moveDoorOpened; } - if (OrderList[OrderPos] & Change_direction) // może być nałożona na inną i wtedy ma priorytet + auto const currentorder { OrderCurrentGet() }; + if (currentorder & Change_direction) // może być nałożona na inną i wtedy ma priorytet iDirectionOrder = -iDirection; // trzeba zmienić jawnie, bo się nie domyśli - else if (OrderList[OrderPos] == Obey_train) + else if (currentorder == Obey_train) iDrivigFlags |= moveStopPoint; // W4 są widziane - else if (OrderList[OrderPos] == Disconnect) + else if (currentorder == Disconnect) iVehicleCount = iVehicleCount < 0 ? 0 : iVehicleCount; // odczepianie lokomotywy - else if (OrderList[OrderPos] == Connect) + else if (currentorder == Connect) iDrivigFlags &= ~moveStopPoint; // podczas jazdy na połączenie nie zwracać uwagi na W4 - else if (OrderList[OrderPos] == Wait_for_orders) + else if (currentorder == Wait_for_orders) OrdersClear(); // czyszczenie rozkazów i przeskok do zerowej pozycji } void TController::OrderNext(TOrders NewOrder) { // ustawienie rozkazu do wykonania jako następny - if (OrderList[OrderPos] == NewOrder) + if (OrderCurrentGet() == NewOrder) return; // jeśli robi to, co trzeba, to koniec if (!OrderPos) OrderPos = 1; // na pozycji zerowej pozostaje czekanie @@ -5071,7 +5074,7 @@ void TController::OrderNext(TOrders NewOrder) void TController::OrderPush(TOrders NewOrder) { // zapisanie na stosie kolejnego rozkazu do wykonania - if (OrderPos == OrderTop && OrderList[OrderPos] < Shunt) // jeśli miałby być zapis na aktalnej pozycji + if (OrderPos == OrderTop && OrderCurrentGet() < Shunt) // jeśli miałby być zapis na aktalnej pozycji // ale nie jedzie ++OrderTop; // niektóre operacje muszą zostać najpierw dokończone => zapis na kolejnej @@ -5406,13 +5409,10 @@ TCommandType TController::BackwardScan( double const Range ) return TCommandType::cm_Unknown; // nic } 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 = glm::length(sem) - 2; // 2m luzu przy manewrach wystarczy - if (scandist < 0) - { - // ujemnych nie ma po co wysyłać - scandist = 0; - } + scandist = std::max(glm::length(sem) - 2.0, 0.0); // 2m luzu przy manewrach wystarczy, ujemnych nie ma po co wysyłać +#endif } auto move{false}; // czy AI w trybie manewerowym ma dociągnąć pod S1 @@ -8109,7 +8109,7 @@ TController::check_route_behind( double const Range ) { } iDirectionOrder = -iDirection; // zmiana kierunku jazdy // zmiana kierunku bez psucia kolejnych komend - OrderList[ OrderPos ] = TOrders( OrderCurrentGet() | Change_direction ); + OrderCurrentSet( TOrders( OrderCurrentGet() | Change_direction ) ); } } } diff --git a/vehicle/Driver.h b/vehicle/Driver.h index c6eaa366..644b8122 100644 --- a/vehicle/Driver.h +++ b/vehicle/Driver.h @@ -482,6 +482,7 @@ private: void OrderPush( TOrders NewOrder ); void OrderNext( TOrders NewOrder ); inline TOrders OrderCurrentGet() const; + inline void OrderCurrentSet( TOrders const Order ); inline TOrders OrderNextGet() const; void OrderCheck(); void OrdersInit( double fVel ); @@ -651,6 +652,13 @@ inline TOrders TController::OrderCurrentGet() const { return OrderList[ OrderPos ]; } +inline void TController::OrderCurrentSet( TOrders const Order ) { + if( OrderPos < 0 || OrderPos >= maxorders ) { + return; + } + OrderList[ OrderPos ] = Order; +} + inline TOrders TController::OrderNextGet() const { if( OrderPos < 0 || OrderPos >= maxorders ) { return Wait_for_orders; diff --git a/vehicle/Train.cpp b/vehicle/Train.cpp index 6f97769b..fe153cae 100644 --- a/vehicle/Train.cpp +++ b/vehicle/Train.cpp @@ -30,7 +30,8 @@ http://mozilla.org/MPL/2.0/. #include "Console.h" #include "application/application.h" #include "rendering/renderer.h" -#include +#include +#include #include #include /* @@ -12398,9 +12399,25 @@ uint16_t TTrain::id() return vid; } -#include -#include -#include +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) +{ + try + { + for (std::size_t j = Start; j < End; ++j) + { + if (Trains[j]) + Trains[j]->Update(Dt); + } + } + catch (std::exception const &) + { + Error = std::current_exception(); + } +} +} // namespace void train_table::updateAsync(double dt) { @@ -12408,7 +12425,7 @@ void train_table::updateAsync(double dt) const size_t total = m_items.size(); const size_t chunkSize = (total + threads - 1) / threads; - std::vector workers; + std::vector workers; std::vector errors(threads); workers.reserve(threads); @@ -12420,24 +12437,8 @@ void train_table::updateAsync(double dt) if (start >= end) break; // brak więcej danych - workers.emplace_back( - [this, start, end, dt, &error = errors[i]]() - { - try - { - for (size_t j = start; j < end; ++j) - { - TTrain *train = m_items[j]; - if (train) - train->Update(dt); - } - } - catch (...) - { - // przekaż wyjątek do głównego wątku - error = std::current_exception(); - } - }); + // 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])); } // Poczekaj aż wszystkie wątki skończą