From 8bcd409fb0a3c9532b0796db1c47707e42b7522e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Sat, 6 Jan 2024 09:38:25 +0200 Subject: [PATCH 1/4] Fix #21145, #21158: Keep handles for intervals stable and resolve crash --- src/openrct2/scripting/ScriptEngine.cpp | 95 ++++++++++++++----------- src/openrct2/scripting/ScriptEngine.h | 11 +-- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/openrct2/scripting/ScriptEngine.cpp b/src/openrct2/scripting/ScriptEngine.cpp index 445f92330d..aa57a2c176 100644 --- a/src/openrct2/scripting/ScriptEngine.cpp +++ b/src/openrct2/scripting/ScriptEngine.cpp @@ -1597,44 +1597,48 @@ void ScriptEngine::SetParkStorageFromJSON(std::string_view value) IntervalHandle ScriptEngine::AllocateHandle() { - for (size_t i = 0; i < _intervals.size(); i++) + auto res = _nextIntervalHandle; + _nextIntervalHandle++; + + // In case of overflow start from 1 again + if (_nextIntervalHandle == 0) { - if (!_intervals[i].IsValid()) - { - return static_cast(i + 1); - } + _nextIntervalHandle = 1; } - _intervals.emplace_back(); - return static_cast(_intervals.size()); + + return res; } IntervalHandle ScriptEngine::AddInterval(const std::shared_ptr& plugin, int32_t delay, bool repeat, DukValue&& callback) { auto handle = AllocateHandle(); - if (handle != 0) - { - auto& interval = _intervals[static_cast(handle) - 1]; - interval.Owner = plugin; - interval.Handle = handle; - interval.Delay = delay; - interval.LastTimestamp = _lastIntervalTimestamp; - interval.Callback = std::move(callback); - interval.Repeat = repeat; - } + assert(handle != 0); + + auto& interval = _intervals[handle]; + interval.Owner = plugin; + interval.Delay = delay; + interval.LastTimestamp = _lastIntervalTimestamp; + interval.Callback = std::move(callback); + interval.Repeat = repeat; + return handle; } void ScriptEngine::RemoveInterval(const std::shared_ptr& plugin, IntervalHandle handle) { - if (handle > 0 && static_cast(handle) <= _intervals.size()) - { - auto& interval = _intervals[static_cast(handle) - 1]; + if (handle == 0) + return; - // Only allow owner or REPL (nullptr) to remove intervals - if (plugin == nullptr || interval.Owner == plugin) - { - interval = {}; - } + auto it = _intervals.find(handle); + if (it == _intervals.end()) + return; + + auto& interval = it->second; + + // Only allow owner or REPL (nullptr) to remove intervals + if (plugin == nullptr || interval.Owner == plugin) + { + _intervals.erase(it); } } @@ -1645,29 +1649,29 @@ void ScriptEngine::UpdateIntervals() { // timestamp has wrapped, subtract all intervals by the remaining amount before wrap auto delta = static_cast(std::numeric_limits::max() - _lastIntervalTimestamp); - for (auto& interval : _intervals) + for (auto& entry : _intervals) { - if (interval.IsValid()) - { - interval.LastTimestamp = -delta; - } + auto& interval = entry.second; + interval.LastTimestamp = -delta; } } _lastIntervalTimestamp = timestamp; - for (auto& interval : _intervals) + // This loop needs to account for removal and insertions during iteration. + for (auto it = _intervals.begin(), itNext = it; it != _intervals.end(); it = itNext) { - if (interval.IsValid()) - { - if (timestamp >= interval.LastTimestamp + interval.Delay) - { - ExecutePluginCall(interval.Owner, interval.Callback, {}, false); + itNext++; - interval.LastTimestamp = timestamp; - if (!interval.Repeat) - { - RemoveInterval(nullptr, interval.Handle); - } + auto& interval = it->second; + + if (timestamp >= interval.LastTimestamp + interval.Delay) + { + ExecutePluginCall(interval.Owner, interval.Callback, {}, false); + + interval.LastTimestamp = timestamp; + if (!interval.Repeat) + { + _intervals.erase(it); } } } @@ -1675,12 +1679,17 @@ void ScriptEngine::UpdateIntervals() void ScriptEngine::RemoveIntervals(const std::shared_ptr& plugin) { - for (auto& interval : _intervals) + for (auto it = _intervals.begin(); it != _intervals.end();) { + auto& interval = it->second; + if (interval.Owner == plugin) { - interval = {}; + it = _intervals.erase(it); + continue; } + + it++; } } diff --git a/src/openrct2/scripting/ScriptEngine.h b/src/openrct2/scripting/ScriptEngine.h index 4c5542e0de..36db2f2ba2 100644 --- a/src/openrct2/scripting/ScriptEngine.h +++ b/src/openrct2/scripting/ScriptEngine.h @@ -126,20 +126,14 @@ namespace OpenRCT2::Scripting } }; - using IntervalHandle = int32_t; + using IntervalHandle = uint32_t; struct ScriptInterval { std::shared_ptr Owner; - IntervalHandle Handle{}; uint32_t Delay{}; int64_t LastTimestamp{}; DukValue Callback; bool Repeat{}; - - bool IsValid() const - { - return Handle != 0; - } }; class ScriptEngine @@ -162,7 +156,8 @@ namespace OpenRCT2::Scripting DukValue _parkStorage; uint32_t _lastIntervalTimestamp{}; - std::vector _intervals; + std::map _intervals; + IntervalHandle _nextIntervalHandle = 1; std::unique_ptr _pluginFileWatcher; std::unordered_set _changedPluginFiles; From c7a716e0501a5d7a81183b9caee9788942b93181 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Sat, 6 Jan 2024 09:44:32 +0200 Subject: [PATCH 2/4] Update changelog.txt --- distribution/changelog.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/distribution/changelog.txt b/distribution/changelog.txt index 43ff57dce5..4973792786 100644 --- a/distribution/changelog.txt +++ b/distribution/changelog.txt @@ -1,5 +1,7 @@ 0.4.8 (in development) ------------------------------------------------------------------------ +- Fix: [#21145] [Plugin] setInterval/setTimeout handle conflict. +- Fix: [#21158] [Plugin] Potential crash using setInterval/setTimeout within the callback. 0.4.7 (2023-12-31) ------------------------------------------------------------------------ From 88a0e4b8d409c21ee704cbb22301f56ea46f1648 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Thu, 11 Jan 2024 17:34:32 +0200 Subject: [PATCH 3/4] Apply review suggestions, remove redundant comment --- src/openrct2/scripting/ScriptEngine.cpp | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/src/openrct2/scripting/ScriptEngine.cpp b/src/openrct2/scripting/ScriptEngine.cpp index aa57a2c176..10e8a7bd97 100644 --- a/src/openrct2/scripting/ScriptEngine.cpp +++ b/src/openrct2/scripting/ScriptEngine.cpp @@ -1597,16 +1597,10 @@ void ScriptEngine::SetParkStorageFromJSON(std::string_view value) IntervalHandle ScriptEngine::AllocateHandle() { - auto res = _nextIntervalHandle; - _nextIntervalHandle++; - // In case of overflow start from 1 again - if (_nextIntervalHandle == 0) - { - _nextIntervalHandle = 1; - } + _nextIntervalHandle = std::max(_nextIntervalHandle++, 1U); - return res; + return _nextIntervalHandle; } IntervalHandle ScriptEngine::AddInterval(const std::shared_ptr& plugin, int32_t delay, bool repeat, DukValue&& callback) @@ -1657,7 +1651,6 @@ void ScriptEngine::UpdateIntervals() } _lastIntervalTimestamp = timestamp; - // This loop needs to account for removal and insertions during iteration. for (auto it = _intervals.begin(), itNext = it; it != _intervals.end(); it = itNext) { itNext++; @@ -1686,10 +1679,11 @@ void ScriptEngine::RemoveIntervals(const std::shared_ptr& plugin) if (interval.Owner == plugin) { it = _intervals.erase(it); - continue; } - - it++; + else + { + it++; + } } } From 6eea65fac7318ab7763c79ab97f92288f98fdb9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=CE=B6eh=20Matt?= <5415177+ZehMatt@users.noreply.github.com> Date: Thu, 11 Jan 2024 19:45:07 +0200 Subject: [PATCH 4/4] Make the compiler happy --- src/openrct2/scripting/ScriptEngine.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/openrct2/scripting/ScriptEngine.cpp b/src/openrct2/scripting/ScriptEngine.cpp index 10e8a7bd97..3d05f5f162 100644 --- a/src/openrct2/scripting/ScriptEngine.cpp +++ b/src/openrct2/scripting/ScriptEngine.cpp @@ -1597,10 +1597,12 @@ void ScriptEngine::SetParkStorageFromJSON(std::string_view value) IntervalHandle ScriptEngine::AllocateHandle() { - // In case of overflow start from 1 again - _nextIntervalHandle = std::max(_nextIntervalHandle++, 1U); + const auto nextHandle = _nextIntervalHandle; - return _nextIntervalHandle; + // In case of overflow start from 1 again + _nextIntervalHandle = std::max(_nextIntervalHandle + 1U, 1U); + + return nextHandle; } IntervalHandle ScriptEngine::AddInterval(const std::shared_ptr& plugin, int32_t delay, bool repeat, DukValue&& callback)