From a8a945b53401f0f4fbc6407a5c8f05718c29af71 Mon Sep 17 00:00:00 2001 From: cristian64 Date: Fri, 17 May 2024 22:10:10 +0100 Subject: [PATCH] Prevent segmentation fault and construct `DlgAddWatchEntry` dialogs in the stack. Previously, these dialogs were constructed dynamically in the heap, and were never deleted explicitly; only when their parent QObject was destroyed at a much later time would they be deleted. This deferred destruction was in fact masking a segmentation fault caused by a double-free: the entry in the dialog was not extracted via `stealEntry()`; instead the reference was taking from the `entryCopy` variable (same pointer), resulting in the entry getting deleted twice, eventually. Reproduction steps: - Double click on an watch entry to bring up the **Edit Watch** dialog. - Make any edit and press **OK**. - Quit the application gracefully to force the destruction of the dialog. - A segmentation fault would be produced: ``` (gdb) bt #0 QArrayDataPointer::deref() (this=, this=0x558b5d1a35c0) at /usr/include/c++/11/bits/atomic_base.h:392 #1 QArrayDataPointer::~QArrayDataPointer() (this=0x558b5d1a35c0, __in_chrg=) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qarraydatapointer.h:129 #2 QString::~QString() (this=0x558b5d1a35c0, __in_chrg=) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qstring.h:1302 #3 MemWatchEntry::~MemWatchEntry() (this=0x558b5d1a35c0, __in_chrg=) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:48 #4 0x0000558b5c517787 in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:31 #5 0x0000558b5c51782d in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:32 #6 0x00007fde5556599b in QObjectPrivate::deleteChildren() () at /lib/x86_64-linux-gnu/libQt6Core.so.6 #7 0x00007fde5624cab8 in QWidget::~QWidget() () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6 #8 0x0000558b5c51d0cd in MemWatchWidget::~MemWatchWidget() (this=0x558b5cd068e0, __in_chrg=) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:41 #9 0x0000558b5c5367cd in MainWindow::~MainWindow() (this=0x7ffc70ef10f0, __in_chrg=) at /w/dolphin-memory-engine/Source/GUI/MainWindow.cpp:56 #10 0x0000558b5c4f1ad7 in main(int, char**) (argc=, argv=) at /w/dolphin-memory-engine/Source/main.cpp:56 ``` # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Date: Fri May 17 22:10:10 2024 +0100 # # On branch watch_entry_dialog_segfault # Changes to be committed: # modified: Source/GUI/MemViewer/MemViewer.cpp # modified: Source/GUI/MemWatcher/MemWatchWidget.cpp # --- Source/GUI/MemViewer/MemViewer.cpp | 8 +++++--- Source/GUI/MemWatcher/MemWatchWidget.cpp | 14 ++++++++------ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Source/GUI/MemViewer/MemViewer.cpp b/Source/GUI/MemViewer/MemViewer.cpp index 16e836ad..8b661ad3 100644 --- a/Source/GUI/MemViewer/MemViewer.cpp +++ b/Source/GUI/MemViewer/MemViewer.cpp @@ -497,9 +497,11 @@ void MemViewer::addByteIndexAsWatch(int index) { MemWatchEntry* entry = new MemWatchEntry(); entry->setConsoleAddress(m_currentFirstAddress + index); - DlgAddWatchEntry* dlg = new DlgAddWatchEntry(true, entry, this); - if (dlg->exec() == QDialog::Accepted) - emit addWatch(dlg->stealEntry()); + DlgAddWatchEntry dlg(true, entry, this); + if (dlg.exec() == QDialog::Accepted) + { + emit addWatch(dlg.stealEntry()); + } } bool MemViewer::handleNaviguationKey(const int key, bool shiftIsHeld) diff --git a/Source/GUI/MemWatcher/MemWatchWidget.cpp b/Source/GUI/MemWatcher/MemWatchWidget.cpp index 7a071d18..9c7d2afa 100644 --- a/Source/GUI/MemWatcher/MemWatchWidget.cpp +++ b/Source/GUI/MemWatcher/MemWatchWidget.cpp @@ -381,10 +381,10 @@ void MemWatchWidget::onWatchDoubleClicked(const QModelIndex& index) else if (index.column() == MemWatchModel::WATCH_COL_ADDRESS && !node->isGroup()) { MemWatchEntry* entryCopy = new MemWatchEntry(node->getEntry()); - DlgAddWatchEntry* dlg = new DlgAddWatchEntry(false, entryCopy, this); - if (dlg->exec() == QDialog::Accepted) + DlgAddWatchEntry dlg(false, entryCopy, this); + if (dlg.exec() == QDialog::Accepted) { - m_watchModel->editEntry(entryCopy, index); + m_watchModel->editEntry(dlg.stealEntry(), index); m_hasUnsavedChanges = true; } } @@ -499,9 +499,11 @@ void MemWatchWidget::onAddGroup() void MemWatchWidget::onAddWatchEntry() { - DlgAddWatchEntry* dlg = new DlgAddWatchEntry(true, nullptr, this); - if (dlg->exec() == QDialog::Accepted) - addWatchEntry(dlg->stealEntry()); + DlgAddWatchEntry dlg(true, nullptr, this); + if (dlg.exec() == QDialog::Accepted) + { + addWatchEntry(dlg.stealEntry()); + } } void MemWatchWidget::addWatchEntry(MemWatchEntry* entry)