From a0cfcc50a2c3c8f484a46c7227037fd84997de5a Mon Sep 17 00:00:00 2001
From: elsid <elsid.mail@gmail.com>
Date: Thu, 15 Sep 2022 22:41:17 +0200
Subject: [PATCH] Fix dangling pointer access on clicking save in the main menu

getSignature() returns an object which means expression like:
className = it->getSignature().mPlayerClassName;
assigns a temporary object to className that does not outlive the statement.
Having className a string view such code leads to a dangling pointer.

Return a reference from getSignature to save on redundant copying.

Change getSignature implementation to make it visible that it finds a maximum
element.

Do not call getSignature multiple times when possible to avoid seaching for the
same max element multiple times.
---
 apps/openmw/mwgui/savegamedialog.cpp | 14 ++++++++------
 apps/openmw/mwstate/character.cpp    | 23 ++++++++++++-----------
 apps/openmw/mwstate/character.hpp    |  2 +-
 3 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/apps/openmw/mwgui/savegamedialog.cpp b/apps/openmw/mwgui/savegamedialog.cpp
index 5149552ea9..8da5047efd 100644
--- a/apps/openmw/mwgui/savegamedialog.cpp
+++ b/apps/openmw/mwgui/savegamedialog.cpp
@@ -176,26 +176,28 @@ namespace MWGui
         {
             if (it->begin()!=it->end())
             {
+                const ESM::SavedGame& signature = it->getSignature();
+
                 std::stringstream title;
-                title << it->getSignature().mPlayerName;
+                title << signature.mPlayerName;
 
                 // For a custom class, we will not find it in the store (unless we loaded the savegame first).
                 // Fall back to name stored in savegame header in that case.
                 std::string_view className;
-                if (it->getSignature().mPlayerClassId.empty())
-                    className = it->getSignature().mPlayerClassName;
+                if (signature.mPlayerClassId.empty())
+                    className = signature.mPlayerClassName;
                 else
                 {
                     // Find the localised name for this class from the store
-                    const ESM::Class* class_ = MWBase::Environment::get().getWorld()->getStore().get<ESM::Class>().search(
-                                it->getSignature().mPlayerClassId);
+                    const ESM::Class* class_ = MWBase::Environment::get().getWorld()->getStore().get<ESM::Class>()
+                            .search(signature.mPlayerClassId);
                     if (class_)
                         className = class_->mName;
                     else
                         className = "?"; // From an older savegame format that did not support custom classes properly.
                 }
 
-                title << " (#{sLevel} " << it->getSignature().mPlayerLevel << " " << MyGUI::TextIterator::toTagsString(toUString(className)) << ")";
+                title << " (#{sLevel} " << signature.mPlayerLevel << " " << MyGUI::TextIterator::toTagsString(toUString(className)) << ")";
 
                 mCharacterSelection->addItem (MyGUI::LanguageManager::getInstance().replaceTags(title.str()));
 
diff --git a/apps/openmw/mwstate/character.cpp b/apps/openmw/mwstate/character.cpp
index 8e21325c47..842a6f967d 100644
--- a/apps/openmw/mwstate/character.cpp
+++ b/apps/openmw/mwstate/character.cpp
@@ -4,6 +4,8 @@
 #include <filesystem>
 #include <sstream>
 #include <utility>
+#include <algorithm>
+#include <tuple>
 
 #include <components/esm/defs.hpp>
 #include <components/esm3/esmreader.hpp>
@@ -172,23 +174,22 @@ MWState::Character::SlotIterator MWState::Character::end() const
     return mSlots.rend();
 }
 
-ESM::SavedGame MWState::Character::getSignature() const
+const ESM::SavedGame& MWState::Character::getSignature() const
 {
     if (mSlots.empty())
         throw std::logic_error ("character signature not available");
 
-    std::vector<Slot>::const_iterator iter (mSlots.begin());
-
-    Slot slot = *iter;
+    const auto tiePlayerLevelAndTimeStamp = [] (const Slot& v)
+    {
+        return std::tie(v.mProfile.mPlayerLevel, v.mTimeStamp);
+    };
 
-    for (++iter; iter!=mSlots.end(); ++iter)
-        if (iter->mProfile.mPlayerLevel>slot.mProfile.mPlayerLevel)
-            slot = *iter;
-        else if (iter->mProfile.mPlayerLevel==slot.mProfile.mPlayerLevel &&
-            iter->mTimeStamp>slot.mTimeStamp)
-            slot = *iter;
+    const auto lessByPlayerLevelAndTimeStamp = [&] (const Slot& l, const Slot& r)
+    {
+        return tiePlayerLevelAndTimeStamp(l) < tiePlayerLevelAndTimeStamp(r);
+    };
 
-    return slot.mProfile;
+    return std::max_element(mSlots.begin(), mSlots.end(), lessByPlayerLevelAndTimeStamp)->mProfile;
 }
 
 const std::filesystem::path& MWState::Character::getPath() const
diff --git a/apps/openmw/mwstate/character.hpp b/apps/openmw/mwstate/character.hpp
index 9883eb253c..1f3f6debf5 100644
--- a/apps/openmw/mwstate/character.hpp
+++ b/apps/openmw/mwstate/character.hpp
@@ -62,7 +62,7 @@ namespace MWState
 
             const std::filesystem::path& getPath() const;
 
-            ESM::SavedGame getSignature() const;
+            const ESM::SavedGame& getSignature() const;
             ///< Return signature information for this character.
             ///
             /// \attention This function must not be called if there are no slots.