From 99a3bbc0552cfea13cd2928c67e53c7638b289ac Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Mon, 30 Oct 2023 19:16:59 +0100 Subject: [PATCH 1/2] Core/State: Return an empty string on invalid input to SystemTimeAsDoubleToString(). --- Source/Core/Core/State.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/Source/Core/Core/State.cpp b/Source/Core/Core/State.cpp index 61b47a1198..eeb9b67924 100644 --- a/Source/Core/Core/State.cpp +++ b/Source/Core/Core/State.cpp @@ -265,16 +265,19 @@ static double GetSystemTimeAsDouble() static std::string SystemTimeAsDoubleToString(double time) { // revert adjustments from GetSystemTimeAsDouble() to get a normal Unix timestamp again - time_t seconds = (time_t)time + DOUBLE_TIME_OFFSET; - tm* localTime = localtime(&seconds); + time_t seconds = static_cast(time) + DOUBLE_TIME_OFFSET; + errno = 0; + tm* local_time = localtime(&seconds); + if (errno != 0 || !local_time) + return ""; #ifdef _WIN32 wchar_t tmp[32] = {}; - wcsftime(tmp, std::size(tmp), L"%x %X", localTime); + wcsftime(tmp, std::size(tmp), L"%x %X", local_time); return WStringToUTF8(tmp); #else char tmp[32] = {}; - strftime(tmp, sizeof(tmp), "%x %X", localTime); + strftime(tmp, sizeof(tmp), "%x %X", local_time); return tmp; #endif } From 437946fb1a9c4614c92b0135f28bf29a2bc23f75 Mon Sep 17 00:00:00 2001 From: "Admiral H. Curtiss" Date: Mon, 30 Oct 2023 18:49:58 +0100 Subject: [PATCH 2/2] Core/State: Refactor logic for determining the relative age of existing savestates. The code previously did this indirectly via `std::map`, the key being the timestamp, which required a questionable workaround for the case where multiple states have the same timestamp. By having a particular combination of timestamps in the on-disk savestates, you could cause this workaround to infinitely loop, locking up Dolphin. This avoids this completely by refactoring the logic and just using `std::vector` instead. --- Source/Core/Core/State.cpp | 87 ++++++++++++++++++++------------------ 1 file changed, 46 insertions(+), 41 deletions(-) diff --git a/Source/Core/Core/State.cpp b/Source/Core/Core/State.cpp index eeb9b67924..db855923e6 100644 --- a/Source/Core/Core/State.cpp +++ b/Source/Core/Core/State.cpp @@ -3,6 +3,7 @@ #include "Core/State.h" +#include #include #include #include @@ -230,21 +231,23 @@ void SaveToBuffer(std::vector& buffer) true); } -// return state number not in map -static int GetEmptySlot(std::map m) +namespace +{ +struct SlotWithTimestamp +{ + int slot; + double timestamp; +}; +} // namespace + +// returns first slot number not in the vector, or -1 if all are in the vector +static int GetEmptySlot(const std::vector& used_slots) { for (int i = 1; i <= (int)NUM_STATES; i++) { - bool found = false; - for (auto& p : m) - { - if (p.second == i) - { - found = true; - break; - } - } - if (!found) + const auto it = std::find_if(used_slots.begin(), used_slots.end(), + [i](const SlotWithTimestamp& slot) { return slot.slot == i; }); + if (it == used_slots.end()) return i; } return -1; @@ -284,11 +287,10 @@ static std::string SystemTimeAsDoubleToString(double time) static std::string MakeStateFilename(int number); -// read state timestamps -static std::map GetSavedStates() +static std::vector GetUsedSlotsWithTimestamp() { + std::vector result; StateHeader header; - std::map m; for (int i = 1; i <= (int)NUM_STATES; i++) { std::string filename = MakeStateFilename(i); @@ -296,17 +298,16 @@ static std::map GetSavedStates() { if (ReadHeader(filename, header)) { - double d = GetSystemTimeAsDouble() - header.legacy_header.time; - - // increase time until unique value is obtained - while (m.find(d) != m.end()) - d += .001; - - m.emplace(d, i); + result.emplace_back(SlotWithTimestamp{.slot = i, .timestamp = header.legacy_header.time}); } } } - return m; + return result; +} + +static bool CompareTimestamp(const SlotWithTimestamp& lhs, const SlotWithTimestamp& rhs) +{ + return lhs.timestamp < rhs.timestamp; } static void CompressBufferToFile(const u8* raw_buffer, u64 size, File::IOFile& f) @@ -957,33 +958,37 @@ void Load(int slot) void LoadLastSaved(int i) { - std::map savedStates = GetSavedStates(); - - if (i > (int)savedStates.size()) - Core::DisplayMessage("State doesn't exist", 2000); - else + if (i <= 0) { - std::map::iterator it = savedStates.begin(); - std::advance(it, i - 1); - Load(it->second); + Core::DisplayMessage("State doesn't exist", 2000); + return; } + + std::vector used_slots = GetUsedSlotsWithTimestamp(); + if (static_cast(i) > used_slots.size()) + { + Core::DisplayMessage("State doesn't exist", 2000); + return; + } + + std::stable_sort(used_slots.begin(), used_slots.end(), CompareTimestamp); + Load((used_slots.end() - i)->slot); } // must wait for state to be written because it must know if all slots are taken void SaveFirstSaved() { - std::map savedStates = GetSavedStates(); - - // save to an empty slot - if (savedStates.size() < NUM_STATES) - Save(GetEmptySlot(savedStates), true); - // overwrite the oldest state - else + std::vector used_slots = GetUsedSlotsWithTimestamp(); + if (used_slots.size() < NUM_STATES) { - std::map::iterator it = savedStates.begin(); - std::advance(it, savedStates.size() - 1); - Save(it->second, true); + // save to an empty slot + Save(GetEmptySlot(used_slots), true); + return; } + + // overwrite the oldest state + std::stable_sort(used_slots.begin(), used_slots.end(), CompareTimestamp); + Save(used_slots.front().slot, true); } // Load the last state before loading the state