GUI/NetWatch: Fix heap corruption from off-thread model mutation#330
Open
kwsantiago wants to merge 3 commits into
Open
GUI/NetWatch: Fix heap corruption from off-thread model mutation#330kwsantiago wants to merge 3 commits into
kwsantiago wants to merge 3 commits into
Conversation
BlockConnected and TransactionAddedToMempool run on the scheduler thread and mutated the Qt model directly (beginInsertRows/beginRemoveRows/ dataChanged) while the GUI thread painted it, corrupting Qt-internal selection state and causing heap corruption / crashes. Marshal the model mutations to the GUI thread via a queued invocation, as GuiBlockView does. Fixes bitcoinknots#306
setClientModel(nullptr) deleted m_validation_interface without unregistering it from ValidationSignals, leaving a dangling pointer that was dereferenced during shutdown callback flushing (use-after-free). Unregister first, mirroring the destructor.
m_data is allocated with ::operator new but freed with a delete-expression on a uint8_t*, which is undefined behaviour (mismatched deallocation size under sized delete). Free it with the matching ::operator delete.
|
tACK 191d0fd I reviewed the change with Code Rabbit and it found no issues. I built PR330 on Ubuntu 24.04 running on ARMv8-A (64-bit) with gcc for Windows using guix. I tested for memory leaks, and found none. I then built using guix. And tested with: Everything worked as expected in the "Watch network activity" window. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #306.
The Net Watch model was mutated from validation-interface callbacks running on the scheduler thread (
BlockConnected,TransactionAddedToMempool), while the GUI thread read and painted the same model. Qt models are single-threaded; the concurrentbeginInsertRows/beginRemoveRows/dataChangedcalls corrupted Qt-internal selection state, producing the heap corruption in #306. The callbacks now marshal the model mutations onto the GUI thread with a queued invocation, asGuiBlockViewalready does.Reproduced on release and ASan builds by flooding the mempool while the Net Watch view painted; deterministic crash before, clean after (ASan clean through churn and shutdown).
Two adjacent memory-safety bugs found in the same file while verifying, fixed as separate commits:
setClientModel(nullptr)deleted the validation interface without unregistering it, leaving a dangling pointer dereferenced during shutdown flush (use-after-free).LogEntry::clear()freed::operator newmemory with adelete-expression onuint8_t*(mismatched deallocation, UB under sized delete).