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) ------------------------------------------------------------------------ diff --git a/src/openrct2/scripting/ScriptEngine.cpp b/src/openrct2/scripting/ScriptEngine.cpp index 445f92330d..3d05f5f162 100644 --- a/src/openrct2/scripting/ScriptEngine.cpp +++ b/src/openrct2/scripting/ScriptEngine.cpp @@ -1597,44 +1597,44 @@ void ScriptEngine::SetParkStorageFromJSON(std::string_view value) IntervalHandle ScriptEngine::AllocateHandle() { - for (size_t i = 0; i < _intervals.size(); i++) - { - if (!_intervals[i].IsValid()) - { - return static_cast(i + 1); - } - } - _intervals.emplace_back(); - return static_cast(_intervals.size()); + const auto nextHandle = _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) { 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 +1645,28 @@ 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) + 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,11 +1674,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); + } + else + { + 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;