Skip to content

Commit

Permalink
Prevent segmentation fault and construct DlgAddWatchEntry dialogs i…
Browse files Browse the repository at this point in the history
…n 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 taken 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<char16_t>::deref() (this=<optimised out>, this=0x558b5d1a35c0) at /usr/include/c++/11/bits/atomic_base.h:392
aldelaro5#1  QArrayDataPointer<char16_t>::~QArrayDataPointer() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qarraydatapointer.h:129
aldelaro5#2  QString::~QString() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /usr/include/x86_64-linux-gnu/qt6/QtCore/qstring.h:1302
aldelaro5#3  MemWatchEntry::~MemWatchEntry() (this=0x558b5d1a35c0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/MemoryWatch/MemWatchEntry.cpp:48
aldelaro5#4  0x0000558b5c517787 in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:31
aldelaro5#5  0x0000558b5c51782d in DlgAddWatchEntry::~DlgAddWatchEntry() (this=0x558b5d1be8f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/Dialogs/DlgAddWatchEntry.cpp:32
aldelaro5#6  0x00007fde5556599b in QObjectPrivate::deleteChildren() () at /lib/x86_64-linux-gnu/libQt6Core.so.6
aldelaro5#7  0x00007fde5624cab8 in QWidget::~QWidget() () at /lib/x86_64-linux-gnu/libQt6Widgets.so.6
aldelaro5#8  0x0000558b5c51d0cd in MemWatchWidget::~MemWatchWidget() (this=0x558b5cd068e0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MemWatcher/MemWatchWidget.cpp:41
aldelaro5#9  0x0000558b5c5367cd in MainWindow::~MainWindow() (this=0x7ffc70ef10f0, __in_chrg=<optimised out>) at /w/dolphin-memory-engine/Source/GUI/MainWindow.cpp:56
aldelaro5#10 0x0000558b5c4f1ad7 in main(int, char**) (argc=<optimised out>, argv=<optimised out>) at /w/dolphin-memory-engine/Source/main.cpp:56
```
  • Loading branch information
cristian64 committed May 17, 2024
1 parent ae7c268 commit c3666c2
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 9 deletions.
8 changes: 5 additions & 3 deletions Source/GUI/MemViewer/MemViewer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
14 changes: 8 additions & 6 deletions Source/GUI/MemWatcher/MemWatchWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit c3666c2

Please sign in to comment.