fix: crash when theme is changed if mnemonic dialog has been shown by knst · Pull Request #7126 · dashpay/dash

knst · 2026-02-03T11:07:47Z

Issue being fixed or feature implemented

First found by thepez while testing #7040

It happens every time when create new wallet after dialog to validate mnemonic has been shown.

Steps to reproduce:

Create new wallet
Show mnemonic
Confirm the mnemonic is saved
Close dialog by Canceling validation or by Confirming validation
Change theme crashes app

    2025-12-22T15:16:31Z Posix Signal: Segmentation fault
    0#: (0x608F5BE3FDB5) stl_vector.h:115         - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_copy_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data const&)
     1#: (0x608F5BE3FDB5) stl_vector.h:127         - std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data::_M_swap_data(std::_Vector_base<unsigned long, std::allocator<unsigned long> >::_Vector_impl_data&)
     2#: (0x608F5BE3FDB5) stl_vector.h:1962        - std::vector<unsigned long, std::allocator<unsigned long> >::_M_move_assign(std::vector<unsigned long, std::allocator<unsigned long> >&&, std::integral_constant<bool, true>)
     3#: (0x608F5BE3FDB5) stl_vector.h:771         - std::vector<unsigned long, std::allocator<unsigned long> >::operator=(std::vector<unsigned long, std::allocator<unsigned long> >&&)
     4#: (0x608F5BE3FDB5) stacktraces.cpp:784      - HandlePosixSignal
     5#: (0x7D6B37A45330) libc_sigaction.c         - ???
     6#: (0x608F5CC0951B) <unknown-file>           - ???
     7#: (0x608F5CC09901) <unknown-file>           - ???
     8#: (0x608F5B62369D) unique_lock.h:105        - std::unique_lock<std::recursive_mutex>::~unique_lock()
     9#: (0x608F5B62369D) sync.h:226               - UniqueLock<AnnotatedMixin<std::recursive_mutex> >::~UniqueLock()
    10#: (0x608F5B62369D) guiutil.cpp:1031         - GUIUtil::loadStyleSheet(bool)
    11#: (0x608F5B6243C4) guiutil.cpp:1616         - GUIUtil::loadTheme(bool)
    12#: (0x608F5B6C27B7) atomic_base.h:505        - std::__atomic_base<int>::load(std::memory_order) const
    13#: (0x608F5B6C27B7) qatomic_cxx11.h:239      - int QAtomicOps<int>::loadRelaxed<int>(std::atomic<int> const&)
    14#: (0x608F5B6C27B7) qbasicatomic.h:107       - QBasicAtomicInteger<int>::loadRelaxed() const
    15#: (0x608F5B6C27B7) qrefcount.h:66           - QtPrivate::RefCount::deref()
    16#: (0x608F5B6C27B7) qstring.h:1308           - QString::~QString()
    17#: (0x608F5B6C27B7) appearancewidget.cpp:110 - AppearanceWidget::updateTheme(QString const&)
    18#: (0x608F5C90A0CD) <unknown-file>           - ???
    19#: (0x608F5CC5B1E1) <unknown-file>           - ???
    20#: (0x608F5CC5D00B) <unknown-file>           - ???
    21#: (0x608F5CC5D29B) <unknown-file>           - ???
    22#: (0x608F5C90A2EB) <unknown-file>           - ???
    23#: (0x608F5CC57376) <unknown-file>           - ???
    24#: (0x608F5CC586F6) <unknown-file>           - ???
    25#: (0x608F5C8DD693) <unknown-file>           - ???

Current call disconnect(cancel, nullptr, nullptr, nullptr) is too violent for qt objects because it disconnects not only user-added slots, but internal qt slots too; it makes the object Cancel Button to be in an invalid state.

What was done?

Easiest fix is specify some arguments for disconnect, for example, simple disconnect(cancel, &QDialogButtonBox::rejected, this, nullptr); fixes crash but it breaks functionality of Cancel button.

For the final solution look to the commits in PR

How Has This Been Tested?

Tested numerous user cases (list is not full):

create wallet, validate mnemonic, click "BACK"
create wallet, close dialog without validation mnemonic
create wallet, validate mnemonic, click BACK, re-validate mnemonic
show mnemonic for already existing wallet

Also I tried to do the exactly same things, but instead mouse's click use Escape or Ctrl+W or close window by mouse.

I am not sure that I provided 100% coverage by manual testing, but at this point it looks solid for me. Any extra testing is welcome, please help with it!

Breaking Changes

N/A

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation
I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)

This call disconnect(cancel, nullptr, nullptr, nullptr) for cancel makes an internal slots disconnected and it could cause objects to get invalid internal state at some point if theme is changed.

…rification dialog It is supposed to go backwards only if it's validation screen, otherwise it should close dialog

github-actions · 2026-02-03T11:08:20Z

✅ No Merge Conflicts Detected

This PR currently has no conflicts with other open PRs.

coderabbitai · 2026-02-03T11:11:03Z

Walkthrough

The changes refactor the mnemonic verification dialog's button handling mechanism. The "Back" button UI element is removed from the step 2 page in the form definition. In the C++ implementation, back-navigation logic is centralized into a new reject() method override that conditionally clears mnemonic words and navigates between dialog steps or closes the dialog. The accept() method visibility is changed from public to protected, and a dedicated onShowMnemonicAgainClicked() slot is removed in favor of the standard reject() override for handling cancellation and back actions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: preventing a crash when the theme is changed after the mnemonic dialog has been shown. It matches the core issue and solution.
Description check	✅ Passed	The PR description clearly explains the issue (crash when theme changes), provides reproduction steps, identifies the root cause, and describes the solution implemented.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

PastaPastaPasta

utACK

UdjinM6

ACK 009104c

…nd reducing exposure of sensitive data b847bae fix: restore eager clearing of parsed words and improve secure memory handling (UdjinM6) b8b0c1c fmt: fix formatting for mnemonicverificatinodialog (Konstantin Akimov) 1edec05 fix: add mnemonicverificationdialog to non-backported list (Konstantin Akimov) f5d8b74 fix: avoid extra allocation of secured data in temporary std::string for mnemonic (Konstantin Akimov) c7e3b46 refactor: SecureString already does zeroeing, remove duplicated code (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Firstly, current implementation of mnemonic dialog has zeroing of SecureString's objects with std::fill which is useless, and most likely even removed by optimizing compiler. For reference, `SecureString`'s implementation of it, see [src/support/cleanse.cpp](https://github.com/dashpay/dash/blob/develop/src/support/cleanse.cpp) for details: void deallocate(T* p, std::size_t n) { if (p != nullptr) { memory_cleanse(p, sizeof(T) * n); // <- safe memory cleaning } LockedPoolManager::Instance().free(p); } Secondly, current implementation causes creating extra temporary object with sensitive data: QString mnemonicStr = QString::fromStdString(std::string(m_mnemonic.begin(), m_mnemonic.end())); This std::string object maybe omitted, see PR ## What was done? This PR tidy-up a bit mnemonic dialog by fixing these issues and some minor improvements for formatting. Though, using `memory_cleanse` should be considered to use for QStrings. This PR conflicts to #7126 because the same function is changed; I will prefer #7126 to be merged first because 7126 is meant to be backported. ## How Has This Been Tested? Tested as an extra changes to #7126 locally in the same branch, splitted to 2 PR after that. ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only) ACKs for top commit: UdjinM6: utACK b847bae Tree-SHA512: c843a39b49f76c6647d004e1044fd4d24ecc4a82ccc311c50d4a88989bfcd62ac1cec09079c3ac40e5efd9e3790115625e65eedb10fc03bfd6475272cb0df72e

knst added 2 commits February 3, 2026 17:53

fix: crash in mnemonicverificationdialog by proper using reject() event

c24473b

This call disconnect(cancel, nullptr, nullptr, nullptr) for cancel makes an internal slots disconnected and it could cause objects to get invalid internal state at some point if theme is changed.

fix: return full functionality of Back / Cancel button on mnemonic ve…

009104c

…rification dialog It is supposed to go backwards only if it's validation screen, otherwise it should close dialog

knst added this to the 23.1 milestone Feb 3, 2026

knst added the backport-candidate-23.0.x label Feb 3, 2026

knst requested review from PastaPastaPasta, UdjinM6 and thephez February 3, 2026 11:23

knst mentioned this pull request Feb 3, 2026

feat: tidy-up mnemonic dialog by removing useless code and reducing exposure of sensitive data #7127

Merged

5 tasks

PastaPastaPasta approved these changes Feb 5, 2026

View reviewed changes

UdjinM6 approved these changes Feb 5, 2026

View reviewed changes

PastaPastaPasta merged commit 7d0d5a7 into dashpay:develop Feb 6, 2026
87 of 90 checks passed

knst mentioned this pull request Feb 13, 2026

chore: v23.1.0 release #7139

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: crash when theme is changed if mnemonic dialog has been shown#7126

fix: crash when theme is changed if mnemonic dialog has been shown#7126
PastaPastaPasta merged 2 commits intodashpay:developfrom
knst:fix-mnemonic-crash-theme

knst commented Feb 3, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Feb 3, 2026

Uh oh!

coderabbitai bot commented Feb 3, 2026 •

edited

Loading

Uh oh!

PastaPastaPasta left a comment

Uh oh!

UdjinM6 left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

knst commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

github-actions bot commented Feb 3, 2026

✅ No Merge Conflicts Detected

Uh oh!

coderabbitai bot commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Uh oh!

PastaPastaPasta left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

knst commented Feb 3, 2026 •

edited

Loading

coderabbitai bot commented Feb 3, 2026 •

edited

Loading