From f4c579e720013065ebe5257aa40d713e354bcfce Mon Sep 17 00:00:00 2001 From: smurf3tte <75271109+smurf3tte@users.noreply.github.com> Date: Tue, 29 Dec 2020 14:28:27 -0800 Subject: [PATCH] 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. --- .../Core/DolphinQt/Config/NewPatchDialog.cpp | 126 ++++++++++-------- Source/Core/DolphinQt/Config/NewPatchDialog.h | 8 +- 2 files changed, 76 insertions(+), 58 deletions(-) diff --git a/Source/Core/DolphinQt/Config/NewPatchDialog.cpp b/Source/Core/DolphinQt/Config/NewPatchDialog.cpp index 95d0eb7621..6f06899639 100644 --- a/Source/Core/DolphinQt/Config/NewPatchDialog.cpp +++ b/Source/Core/DolphinQt/Config/NewPatchDialog.cpp @@ -18,6 +18,23 @@ #include "Core/PatchEngine.h" #include "DolphinQt/QtUtils/ModalMessageBox.h" +struct NewPatchEntry +{ + NewPatchEntry() = default; + + // These entries share the lifetime of their associated text widgets, and because they are + // captured by pointer by various edit handlers, they should not copied or moved. + NewPatchEntry(const NewPatchEntry&) = delete; + NewPatchEntry& operator=(const NewPatchEntry&) = delete; + NewPatchEntry(NewPatchEntry&&) = delete; + NewPatchEntry& operator=(NewPatchEntry&&) = delete; + + 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 +56,8 @@ NewPatchDialog::NewPatchDialog(QWidget* parent, PatchEngine::Patch& patch) } } +NewPatchDialog::~NewPatchDialog() = default; + void NewPatchDialog::CreateWidgets() { m_name_edit = new QLineEdit; @@ -80,9 +99,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 +121,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 +142,11 @@ 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 = m_entries.emplace_back(std::make_unique()).get(); + new_entry->address = address; + new_entry->value = value; + new_entry->comparand = comparand; + new_entry->entry = entry; auto* conditional = new QCheckBox(tr("Conditional")); auto* comparand_label = new QLabel(tr("Comparand:")); @@ -165,56 +164,53 @@ QGroupBox* NewPatchDialog::CreateEntry(PatchEngine::PatchEntry& entry) box->setLayout(layout); connect(address, qOverload(&QLineEdit::textEdited), - [&entry, address](const QString& text) { entry.address = OnTextEdited(address, text); }); - - connect(value, qOverload(&QLineEdit::textEdited), - [&entry, value](const QString& text) { entry.value = OnTextEdited(value, text); }); - - connect(comparand, qOverload(&QLineEdit::textEdited), - [&entry, comparand](const QString& text) { - entry.comparand = OnTextEdited(comparand, text); + [new_entry](const QString& text) { + new_entry->entry.address = OnTextEdited(new_entry->address, text); }); - connect(remove, &QPushButton::clicked, [this, box, address, value, comparand, entry] { - if (m_patch.entries.size() > 1) + connect(value, qOverload(&QLineEdit::textEdited), + [new_entry](const QString& text) { + new_entry->entry.value = OnTextEdited(new_entry->value, text); + }); + + connect(comparand, qOverload(&QLineEdit::textEdited), + [new_entry](const QString& text) { + new_entry->entry.comparand = OnTextEdited(new_entry->comparand, text); + }); + + 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 +236,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 +262,12 @@ void NewPatchDialog::accept() return; } + m_patch.entries.clear(); + + for (const auto& entry : m_entries) + { + m_patch.entries.emplace_back(entry->entry); + } + QDialog::accept(); } diff --git a/Source/Core/DolphinQt/Config/NewPatchDialog.h b/Source/Core/DolphinQt/Config/NewPatchDialog.h index d77005cc1d..ff9d09feea 100644 --- a/Source/Core/DolphinQt/Config/NewPatchDialog.h +++ b/Source/Core/DolphinQt/Config/NewPatchDialog.h @@ -4,6 +4,7 @@ #pragma once +#include #include #include @@ -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,7 +37,7 @@ private: void accept() override; - QGroupBox* CreateEntry(PatchEngine::PatchEntry& entry); + QGroupBox* CreateEntry(const PatchEngine::PatchEntry& entry); QLineEdit* m_name_edit; QWidget* m_entry_widget; @@ -41,7 +45,7 @@ private: QPushButton* m_add_button; QDialogButtonBox* m_button_box; - std::vector m_edits; + std::vector> m_entries; PatchEngine::Patch& m_patch; };