Fix crash on selecting "Mask values" in transaction view

theStack commented at 9:20 pm on October 30, 2023: contributor

This PR fixes a crash bug that can be caused with the following steps:

change to the “Transactions” view
right-click on an arbitrary transaction -> “Show transaction details”
close the transaction detail window again
select menu item “Settings” -> “Mask values”

The problem is that the list of opened dialogs, tracked in the member variable m_opened_dialogs (introduced in #708, commit 4492de1be11f4131811f504a037342c78f480e20), is only ever appended with newly opened transaction detail dialog pointers, but never removed. This leads to dangling pointers in the list, and if the “Mask values” menu item is selected, a crash is caused in the course of trying to close the opened transaction detail dialogs (see closeOpenedDialogs() method). Fix this by removing a pointer of the list if the corresponding widget is destroyed.

DrahtBot commented at 9:20 pm on October 30, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, furszy, achow101, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

theStack commented at 9:21 pm on October 30, 2023: contributor

Tagging @furszy who signaled interest in this crash bug while talking earlier.

theStack force-pushed on Oct 30, 2023

DrahtBot added the label CI failed on Oct 30, 2023

DrahtBot removed the label CI failed on Oct 30, 2023

gui: fix crash on selecting "Mask values" in transaction view

This commits fixes a crash bug that can be caused with the following steps:
- change to the "Transactions" view
- right-click on an arbitrary transaction -> "Show transaction details"
- close the transaction detail window again
- select "Settings" -> "Mask values"

The problem is that the list of opened dialogs, tracked in the member
variable `m_opened_dialogs`, is only ever appended with newly opened
transaction detail dialog pointers, but never removed. This leads to
dangling pointers in the list, and if the "Mask values" menu item is
selected, a crash is caused in the course of trying to close the opened
transaction detail dialogs (see `closeOpenedDialogs()` method). Fix this
by removing the pointer from the list if the corresponding widget is
destroyed.

e26e665f9f

theStack force-pushed on Oct 30, 2023

pablomartin4btc commented at 11:29 pm on October 30, 2023: contributor

Concept ACK

Good catch! I’m the culprit perhaps, I’ll take a deeper look at it ASAP!

I took a quick look at the code and LGTM, the original issue with the multiple transaction details dialogs was caught by another reviewer and solved on the original PR #708 but this is a different one.

theStack commented at 11:33 pm on October 30, 2023: contributor

Just force-pushed a much simpler version (only three lines) which avoids the circular dependency and the introduction of new methods. The old version is still available here for reference (or in case the current solution has problems that I’m not aware of): https://github.com/bitcoin-core/gui/commit/757e58d6f4cc9592de3d960df0eee44f1cbf6c7d

pablomartin4btc approved

pablomartin4btc commented at 4:49 am on October 31, 2023: contributor

tACK e26e665f9f64a962dd56053be817cc953e714847

I easily managed to reproduce the issue in masterfollowing the instructions in the description of the PR (bitcoin-qtcrashes with:Segmentation fault (core dumped)output).

I couldn’t reproduce the error anymore while using the new bitcoin-qt version compiled with this branch.

I’d prefer this second approach better. Thank for fixing it!

MarnixCroes approved

MarnixCroes commented at 8:41 am on October 31, 2023: contributor

tack e26e665f9f64a962dd56053be817cc953e714847

furszy commented at 12:29 pm on October 31, 2023: member

Could also:

0diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
1@@ -656,7 +656,7 @@
2 {
3     // close all dialogs opened from this view
4     for (QDialog* dlg : m_opened_dialogs) {
5-        dlg->close();
6+        if (dlg) dlg->close();
7     }
8     m_opened_dialogs.clear();
9 }

furszy commented at 12:29 pm on October 31, 2023: member

utACK e26e665f9

theStack commented at 1:02 pm on October 31, 2023: contributor

Could also:

0diff --git a/src/qt/transactionview.cpp b/src/qt/transactionview.cpp
1@@ -656,7 +656,7 @@
2 {
3     // close all dialogs opened from this view
4     for (QDialog* dlg : m_opened_dialogs) {
5-        dlg->close();
6+        if (dlg) dlg->close();
7     }
8     m_opened_dialogs.clear();
9 }

This wouldn’t change the behaviour, as a pointer in the m_opened_dialogs list is not set to NULL after the dialog it points to is destroyed. I think it could work if we used guarded pointers like QPointer in the list instead, IIUC (but we don’t seem to use them anywhere in our codebase).

achow101 commented at 5:51 pm on October 31, 2023: member

Should this be backported?

theStack commented at 6:21 pm on October 31, 2023: contributor

Should this be backported?

Yes, I think so. The bug first occured with release 25.0 (according to git tag --contains 4492de1be11 in the main repo), so backporting to 25.x and 26.x would make sense.

achow101 commented at 6:29 pm on October 31, 2023: member

ACK e26e665f9f64a962dd56053be817cc953e714847

pablomartin4btc commented at 9:18 pm on October 31, 2023: contributor

I do agree with @theStack backporting (checked it with the git tag command) since the issue technically is the “conflict” between these 2 lines: https://github.com/bitcoin-core/gui/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/qt/transactionview.cpp#L532-L533

The last one (#L533) was added by the #708 (merged in v25 - I wanted to say I’m innocent but no haha), so another way to solve this issue is just by removing #L532, which I’ve tested and works fine.

hebasto renamed this:
~~gui: fix crash on selecting "Mask values" in transaction view~~
Fix crash on selecting "Mask values" in transaction view
on Nov 1, 2023

hebasto added the label Needs backport (25.x) on Nov 1, 2023

hebasto added the label Needs backport (26.x) on Nov 1, 2023

hebasto approved

hebasto commented at 9:44 am on November 1, 2023: member

ACK e26e665f9f64a962dd56053be817cc953e714847, tested on Ubuntu 22.04.

fanquake commented at 9:51 am on November 1, 2023: member

Will backport this once merged.

hebasto commented at 9:52 am on November 1, 2023: member

https://github.com/bitcoin-core/gui/blob/3c0b66c2ece7695ab59f466d7b4d81a4c18ffd76/src/qt/transactionview.cpp#L532-L533

The last one (#L533) was added by the #708 (merged in v25 - I wanted to say I’m innocent but no haha), so another way to solve this issue is just by removing #L532, which I’ve tested and works fine.

It will still allow to grow the size of m_opened_dialogs unbounded.

The current PR branch tackles with m_opened_dialogs size, which is the optimum solution.

hebasto merged this on Nov 1, 2023

hebasto closed this on Nov 1, 2023

fanquake cross-referenced this on Nov 1, 2023 from issue [26.x] Backports for rc2 by fanquake

fanquake removed the label Needs backport (26.x) on Nov 1, 2023

fanquake commented at 10:02 am on November 1, 2023: member

Added to https://github.com/bitcoin/bitcoin/pull/28754 for 26.x.

fanquake referenced this in commit e097d4cb53 on Nov 1, 2023

theStack deleted the branch on Nov 1, 2023

fanquake referenced this in commit 67b2512560 on Nov 1, 2023

fanquake commented at 1:10 pm on November 1, 2023: member

Pulled this into a branch for 25.x backporting. See https://github.com/bitcoin/bitcoin/pull/28768.

fanquake removed the label Needs backport (25.x) on Nov 1, 2023

fanquake referenced this in commit 647bdfcf51 on Nov 1, 2023

fanquake cross-referenced this on Nov 1, 2023 from issue [25.x] Backports by fanquake

Fix crash on selecting “Mask values” in transaction view #774

Reviews