Merge pull request #21162 from ZehMatt/fix-intervals

Fix #21145, #21158: Keep handles for intervals stable and resolve crash

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)
 ------------------------------------------------------------------------

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<IntervalHandle>(i + 1);
-        }
-    }
-    _intervals.emplace_back();
-    return static_cast<IntervalHandle>(_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>& plugin, int32_t delay, bool repeat, DukValue&& callback)
 {
     auto handle = AllocateHandle();
-    if (handle != 0)
-    {
-        auto& interval = _intervals[static_cast<size_t>(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>& plugin, IntervalHandle handle)
 {
-    if (handle > 0 && static_cast<size_t>(handle) <= _intervals.size())
-    {
-        auto& interval = _intervals[static_cast<size_t>(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<int64_t>(std::numeric_limits<uint32_t>::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>& 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++;
         }
     }
 }

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<Plugin> 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<ScriptInterval> _intervals;
+        std::map<IntervalHandle, ScriptInterval> _intervals;
+        IntervalHandle _nextIntervalHandle = 1;
 
         std::unique_ptr<FileWatcher> _pluginFileWatcher;
         std::unordered_set<std::string> _changedPluginFiles;