Fix crash on selecting "Mask values" in transaction view #774

theStack · 2023-10-30T21:20:16Z

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 4492de1), 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 · 2023-10-30T21:20:19Z

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 · 2023-10-30T21:21:56Z

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

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.

pablomartin4btc

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 · 2023-10-30T23:33:01Z

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): 757e58d

pablomartin4btc

tACK e26e665

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

tack e26e665

furszy

Could also:

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

furszy

utACK e26e665

theStack · 2023-10-31T13:02:59Z

Could also:

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

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 · 2023-10-31T17:51:43Z

Should this be backported?

theStack · 2023-10-31T18:21:16Z

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 · 2023-10-31T18:29:14Z

ACK e26e665

pablomartin4btc · 2023-10-31T21:18:07Z

I do agree with @theStack backporting (checked it with the git tag command) since the issue technically is the "conflict" between these 2 lines:

gui/src/qt/transactionview.cpp

Lines 532 to 533 in 3c0b66c

    
           dlg->setAttribute(Qt::WA_DeleteOnClose); 
        
           m_opened_dialogs.append(dlg);

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

ACK e26e665, tested on Ubuntu 22.04.

fanquake · 2023-11-01T09:51:48Z

Will backport this once merged.

hebasto · 2023-11-01T09:52:05Z

gui/src/qt/transactionview.cpp

Lines 532 to 533 in 3c0b66c

dlg->setAttribute(Qt::WA_DeleteOnClose);

m_opened_dialogs.append(dlg);

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.

fanquake · 2023-11-01T10:02:17Z

Added to bitcoin/bitcoin#28754 for 26.x.

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. Github-Pull: bitcoin-core/gui#774 Rebased-From: e26e665

e4e8479 doc: update manual pages for v26.0rc2 (fanquake) 0b189a9 build: bump version to v26.0rc2 (fanquake) e097d4c gui: fix crash on selecting "Mask values" in transaction view (Sebastian Falbesoner) 05e8874 guix: update signapple (fanquake) deccc50 guix: Zip needs to include all files with time as SOURCE_DATE_EPOCH (Andrew Chow) fe57abd test: add coverage for snapshot chainstate not matching AssumeUTXO parameters (pablomartin4btc) b761a58 assumeutxo, blockstorage: prevent core dump on invalid hash (pablomartin4btc) d3ebf6e [test] Test i2p private key constraints (Vasil Dimov) 1f11784 [net] Check i2p private key constraints (dergoegge) 6544ffa bugfix: Mark CNoDestination and PubKeyDestination constructor explicit (MarcoFalke) Pull request description: Backports for v26.0rc2: * #28695 * #28698 * #28728 * #28757 * #28759 * bitcoin-core/gui#774 ACKs for top commit: josibake: ACK e4e8479 hebasto: re-ACK e4e8479, only a backport of bitcoin-core/gui#774 added since my [recent](#28754 (review)) review. TheCharlatan: Re-ACK e4e8479 Tree-SHA512: 4b95afd26b8bf91250cb883423de8b274cefa48dc474734f5900aeb756eee3a6c656116efcfa2caff3c250678c16b70cc6b7a5d840018dc7e2c1e8161622cd61

fanquake · 2023-11-01T13:10:28Z

Pulled this into a branch for 25.x backporting. See bitcoin/bitcoin#28768.

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. Github-Pull: bitcoin-core/gui#774 Rebased-From: e26e665

53bbda5 doc: update release notes for 25.x (fanquake) 31e1e03 test: add regression test for the getrawtransaction segfault (Martin Zumsande) 041228d rpc: fix getrawtransaction segfault (Martin Zumsande) b86285d gui: fix crash on selecting "Mask values" in transaction view (Sebastian Falbesoner) c21024f doc: add historical release notes for 25.1 (fanquake) Pull request description: Collecting backports for the 25.x branch. Currently: * bitcoin-core/gui#774 * #29003 ACKs for top commit: stickies-v: ACK 53bbda5 Tree-SHA512: 9b1ba17cce9de70d20329372ba71225dd930718a1f7db84a7be764dcfbba01c5e466255e7b95433ab6d7559ee8aaa04cc99ee5d1512d91fcc0a8015f1aa4150a

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. Github-Pull: bitcoin-core/gui#774 Rebased-From: e26e665f9f64a962dd56053be817cc953e714847

theStack force-pushed the 202310-gui-fix_mask_values_crash_in_transaction_view branch from 02863e3 to 757e58d Compare October 30, 2023 21:53

DrahtBot added CI failed and removed CI failed labels Oct 30, 2023

theStack force-pushed the 202310-gui-fix_mask_values_crash_in_transaction_view branch from 757e58d to e26e665 Compare October 30, 2023 23:29

pablomartin4btc reviewed Oct 30, 2023

View reviewed changes

pablomartin4btc approved these changes Oct 31, 2023

View reviewed changes

MarnixCroes approved these changes Oct 31, 2023

View reviewed changes

furszy reviewed Oct 31, 2023

View reviewed changes

hebasto changed the title ~~gui: fix crash on selecting "Mask values" in transaction view~~ Fix crash on selecting "Mask values" in transaction view Nov 1, 2023

hebasto added Needs backport (25.x) Needs backport (26.x) labels Nov 1, 2023

hebasto approved these changes Nov 1, 2023

View reviewed changes

hebasto merged commit 04bfe8c into bitcoin-core:master Nov 1, 2023
16 checks passed

fanquake mentioned this pull request Nov 1, 2023

[26.x] Backports for rc2 bitcoin/bitcoin#28754

Merged

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

theStack deleted the 202310-gui-fix_mask_values_crash_in_transaction_view branch November 1, 2023 10:03

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

fanquake mentioned this pull request Nov 1, 2023

[25.x] Backports bitcoin/bitcoin#28768

Merged

bitcoin-core locked and limited conversation to collaborators Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash on selecting "Mask values" in transaction view #774

Fix crash on selecting "Mask values" in transaction view #774

theStack commented Oct 30, 2023 •

edited by hebasto

Loading

DrahtBot commented Oct 30, 2023 •

edited

Loading

theStack commented Oct 30, 2023

pablomartin4btc left a comment

theStack commented Oct 30, 2023

pablomartin4btc left a comment

MarnixCroes left a comment

furszy left a comment

furszy left a comment

theStack commented Oct 31, 2023

achow101 commented Oct 31, 2023

theStack commented Oct 31, 2023

achow101 commented Oct 31, 2023

pablomartin4btc commented Oct 31, 2023

hebasto left a comment

fanquake commented Nov 1, 2023

hebasto commented Nov 1, 2023 •

edited

Loading

fanquake commented Nov 1, 2023

fanquake commented Nov 1, 2023 •

edited

Loading

Fix crash on selecting "Mask values" in transaction view #774

Fix crash on selecting "Mask values" in transaction view #774

Conversation

theStack commented Oct 30, 2023 • edited by hebasto Loading

DrahtBot commented Oct 30, 2023 • edited Loading

Reviews

theStack commented Oct 30, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

theStack commented Oct 30, 2023

pablomartin4btc left a comment

Choose a reason for hiding this comment

MarnixCroes left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

theStack commented Oct 31, 2023

achow101 commented Oct 31, 2023

theStack commented Oct 31, 2023

achow101 commented Oct 31, 2023

pablomartin4btc commented Oct 31, 2023

hebasto left a comment

Choose a reason for hiding this comment

fanquake commented Nov 1, 2023

hebasto commented Nov 1, 2023 • edited Loading

fanquake commented Nov 1, 2023

fanquake commented Nov 1, 2023 • edited Loading

theStack commented Oct 30, 2023 •

edited by hebasto

Loading

DrahtBot commented Oct 30, 2023 •

edited

Loading

hebasto commented Nov 1, 2023 •

edited

Loading

fanquake commented Nov 1, 2023 •

edited

Loading