Fix bad memory references in NewPatchDialog

This code was storing references to patch entries which could move around in memory if a patch was erased from the middle of a vector or if the vector itself was reallocated. Instead, NewPatchDialog maintains a separate copy of the patch entries which are committed back to the patch if the user accepts the changes.

Source/Core/DolphinQt/Config/NewPatchDialog.cpp

@@ -18,6 +18,14 @@
 #include "Core/PatchEngine.h"
 #include "DolphinQt/QtUtils/ModalMessageBox.h"
 
+struct NewPatchEntry
+{
+  QLineEdit* address = nullptr;
+  QLineEdit* value = nullptr;
+  QLineEdit* comparand = nullptr;
+  PatchEngine::PatchEntry entry;
+};
+
 NewPatchDialog::NewPatchDialog(QWidget* parent, PatchEngine::Patch& patch)
     : QDialog(parent), m_patch(patch)
 {
@@ -39,6 +47,10 @@ NewPatchDialog::NewPatchDialog(QWidget* parent, PatchEngine::Patch& patch)
   }
 }
 
+NewPatchDialog::~NewPatchDialog()
+{
+}
+
 void NewPatchDialog::CreateWidgets()
 {
   m_name_edit = new QLineEdit;
@@ -80,9 +92,7 @@ void NewPatchDialog::ConnectWidgets()
 
 void NewPatchDialog::AddEntry()
 {
-  m_patch.entries.emplace_back();
-
-  m_entry_layout->addWidget(CreateEntry(m_patch.entries[m_patch.entries.size() - 1]));
+  m_entry_layout->addWidget(CreateEntry({}));
 }
 
 static u32 OnTextEdited(QLineEdit* edit, const QString& text)
@@ -104,27 +114,7 @@ static u32 OnTextEdited(QLineEdit* edit, const QString& text)
   return value;
 }
 
-static bool PatchEq(const PatchEngine::PatchEntry& a, const PatchEngine::PatchEntry& b)
-{
-  if (a.address != b.address)
-    return false;
-
-  if (a.type != b.type)
-    return false;
-
-  if (a.value != b.value)
-    return false;
-
-  if (a.comparand != b.comparand)
-    return false;
-
-  if (a.conditional != b.conditional)
-    return false;
-
-  return true;
-}
-
-QGroupBox* NewPatchDialog::CreateEntry(PatchEngine::PatchEntry& entry)
+QGroupBox* NewPatchDialog::CreateEntry(const PatchEngine::PatchEntry& entry)
 {
   QGroupBox* box = new QGroupBox();
 
@@ -145,9 +135,12 @@ QGroupBox* NewPatchDialog::CreateEntry(PatchEngine::PatchEntry& entry)
   auto* value = new QLineEdit;
   auto* comparand = new QLineEdit;
 
-  m_edits.push_back(address);
-  m_edits.push_back(value);
-  m_edits.push_back(comparand);
+  auto new_entry = new NewPatchEntry;
+  new_entry->address = address;
+  new_entry->value = value;
+  new_entry->comparand = comparand;
+  new_entry->entry = entry;
+  m_entries.emplace_back(new_entry);
 
   auto* conditional = new QCheckBox(tr("Conditional"));
   auto* comparand_label = new QLabel(tr("Comparand:"));
@@ -165,56 +158,53 @@ QGroupBox* NewPatchDialog::CreateEntry(PatchEngine::PatchEntry& entry)
   box->setLayout(layout);
 
   connect(address, qOverload<const QString&>(&QLineEdit::textEdited),
-          [&entry, address](const QString& text) { entry.address = OnTextEdited(address, text); });
+          [new_entry](const QString& text) {
+            new_entry->entry.address = OnTextEdited(new_entry->address, text);
+          });
 
   connect(value, qOverload<const QString&>(&QLineEdit::textEdited),
-          [&entry, value](const QString& text) { entry.value = OnTextEdited(value, text); });
+          [new_entry](const QString& text) {
+            new_entry->entry.value = OnTextEdited(new_entry->value, text);
+          });
 
   connect(comparand, qOverload<const QString&>(&QLineEdit::textEdited),
-          [&entry, comparand](const QString& text) {
-            entry.comparand = OnTextEdited(comparand, text);
+          [new_entry](const QString& text) {
+            new_entry->entry.comparand = OnTextEdited(new_entry->comparand, text);
           });
 
-  connect(remove, &QPushButton::clicked, [this, box, address, value, comparand, entry] {
-    if (m_patch.entries.size() > 1)
+  connect(remove, &QPushButton::clicked, [this, box, new_entry] {
+    if (m_entries.size() > 1)
     {
       box->setVisible(false);
       m_entry_layout->removeWidget(box);
       box->deleteLater();
 
-      m_patch.entries.erase(
-          std::find_if(m_patch.entries.begin(), m_patch.entries.end(),
-                       [entry](const PatchEngine::PatchEntry& e) { return PatchEq(e, entry); }));
-
-      const auto it = std::remove_if(
-          m_edits.begin(), m_edits.end(), [address, value, comparand](QLineEdit* line_edit) {
-            return line_edit == address || line_edit == value || line_edit == comparand;
-          });
-      m_edits.erase(it, m_edits.end());
+      m_entries.erase(std::find_if(m_entries.begin(), m_entries.end(),
+                                   [new_entry](const auto& e) { return e.get() == new_entry; }));
     }
   });
 
-  connect(byte, &QRadioButton::toggled, [&entry](bool checked) {
+  connect(byte, &QRadioButton::toggled, [new_entry](bool checked) {
     if (checked)
-      entry.type = PatchEngine::PatchType::Patch8Bit;
+      new_entry->entry.type = PatchEngine::PatchType::Patch8Bit;
   });
 
-  connect(word, &QRadioButton::toggled, [&entry](bool checked) {
+  connect(word, &QRadioButton::toggled, [new_entry](bool checked) {
     if (checked)
-      entry.type = PatchEngine::PatchType::Patch16Bit;
+      new_entry->entry.type = PatchEngine::PatchType::Patch16Bit;
   });
 
-  connect(dword, &QRadioButton::toggled, [&entry](bool checked) {
+  connect(dword, &QRadioButton::toggled, [new_entry](bool checked) {
     if (checked)
-      entry.type = PatchEngine::PatchType::Patch32Bit;
+      new_entry->entry.type = PatchEngine::PatchType::Patch32Bit;
   });
 
   byte->setChecked(entry.type == PatchEngine::PatchType::Patch8Bit);
   word->setChecked(entry.type == PatchEngine::PatchType::Patch16Bit);
   dword->setChecked(entry.type == PatchEngine::PatchType::Patch32Bit);
 
-  connect(conditional, &QCheckBox::toggled, [&entry, comparand_label, comparand](bool checked) {
-    entry.conditional = checked;
+  connect(conditional, &QCheckBox::toggled, [new_entry, comparand_label, comparand](bool checked) {
+    new_entry->entry.conditional = checked;
     comparand_label->setVisible(checked);
     comparand->setVisible(checked);
   });
@@ -240,11 +230,22 @@ void NewPatchDialog::accept()
 
   bool valid = true;
 
-  for (const auto* edit : m_edits)
+  for (const auto& entry : m_entries)
   {
-    edit->text().toUInt(&valid, 16);
+    entry->address->text().toUInt(&valid, 16);
+    if (!valid)
+      break;
+
+    entry->value->text().toUInt(&valid, 16);
     if (!valid)
       break;
+
+    if (entry->entry.conditional)
+    {
+      entry->comparand->text().toUInt(&valid, 16);
+      if (!valid)
+        break;
+    }
   }
 
   if (!valid)
@@ -255,5 +256,12 @@ void NewPatchDialog::accept()
     return;
   }
 
+  m_patch.entries.clear();
+
+  for (const auto& entry : m_entries)
+  {
+    m_patch.entries.emplace_back(entry->entry);
+  }
+
   QDialog::accept();
 }

Source/Core/DolphinQt/Config/NewPatchDialog.h

@@ -4,6 +4,7 @@
 
 #pragma once
 
+#include <memory>
 #include <vector>
 
 #include <QDialog>
@@ -21,10 +22,13 @@ class QLineEdit;
 class QVBoxLayout;
 class QPushButton;
 
+struct NewPatchEntry;
+
 class NewPatchDialog : public QDialog
 {
 public:
   explicit NewPatchDialog(QWidget* parent, PatchEngine::Patch& patch);
+  ~NewPatchDialog() override;
 
 private:
   void CreateWidgets();
@@ -33,15 +37,15 @@ class NewPatchDialog : public QDialog
 
   void accept() override;
 
-  QGroupBox* CreateEntry(PatchEngine::PatchEntry& entry);
+  QGroupBox* CreateEntry(const PatchEngine::PatchEntry& entry);
 
   QLineEdit* m_name_edit;
   QWidget* m_entry_widget;
   QVBoxLayout* m_entry_layout;
   QPushButton* m_add_button;
   QDialogButtonBox* m_button_box;
 
-  std::vector<QLineEdit*> m_edits;
+  std::vector<std::unique_ptr<NewPatchEntry>> m_entries;
 
   PatchEngine::Patch& m_patch;
 };