Skip to content

Commit

Permalink
Fix bad memory references in NewPatchDialog
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@smurf3tte
smurf3tte committed Dec 29, 2020
1 parent f3b8a98 commit f4c579e
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 54 deletions.
118 changes: 66 additions & 52 deletions Source/Core/DolphinQt/Config/NewPatchDialog.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -39,6 +56,8 @@ NewPatchDialog::NewPatchDialog(QWidget* parent, PatchEngine::Patch& patch)
}
}

NewPatchDialog::~NewPatchDialog() = default;

void NewPatchDialog::CreateWidgets()
{
m_name_edit = new QLineEdit;
Expand Down Expand Up @@ -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)
Expand All @@ -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();

Expand All @@ -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<NewPatchEntry>()).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:"));
Expand All @@ -165,56 +164,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);
});
Expand All @@ -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)
Expand All @@ -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();
}
8 changes: 6 additions & 2 deletions Source/Core/DolphinQt/Config/NewPatchDialog.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#pragma once

#include <memory>
#include <vector>

#include <QDialog>
Expand All @@ -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();
Expand All @@ -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;
};

0 comments on commit f4c579e

Please sign in to comment.