UAF prevention: Notify_KnownFileBeingDestroyed broadcast on every CKnownFile destruction - #756
Merged
Conversation
This was referenced May 27, 2026
aMule has eight latent use-after-free sites where raw CKnownFile* / CPartFile* pointers outlive the pointee — surfaced by the ASan crash in issue amule-project#755 (CGenericClientListCtrl::m_knownfiles) and the production SEGV chain in amule-project#748 (CUpDownClient::m_uploadingfile / m_reqfile, fixed in amule-project#749). amule-project#749 is also superseded by this PR. Audit + fix as one coherent change: The new MuleNotify::KnownFileBeingDestroyed(file) broadcast fires from every CKnownFile destruction site BEFORE delete. Each subscriber strips references by pointer-value comparison only — never dereferences the pointer (by the time a main-thread queued subscriber runs, the bytes have typically been recycled). Documented contract in GuiEvents.h. Fire sites: - CPartFile::Delete() (user cancel) - CKnownFileList::Clear() (shutdown) - CKnownFileList::PruneDuplicates() × 3 (TTL/cap eviction) - CKnownFilesRem::DeleteItem (amulegui EC_TAG_FILE_REMOVED) Subscribers (GUI): - CGenericClientListCtrl::RemoveKnownFile (amule-project#755 crash site — m_knownfiles vector + ClientCtrlItem_Struct::m_owner per-row) - CCommentDialog::DropReferencesTo (open-instance registry + self-dismiss on file destruction) - CCommentDialogLst::DropReferencesTo (same) - CFileDetailDialog::DropReferencesTo (same + stops the 5-second update timer + strips m_files vector entries) Subscribers (amulegui only): - CUpDownClientListRem::DropReferencesTo (amule-project#749 content, now triggered through the broadcast instead of a direct call) Subscribers (amule daemon only): - CAICHHashSet::DropReferencesTo (strip-by-ptr on the static m_liRequestedData recovery-request list; pre-existing IsPartFile() guard in ClientAICHRequestFailed can be spoofed by allocator reuse so the strip is needed too) - CPartFileWriteThread::DropReferencesTo (strip pending writes whose pFile matches, deleting the buffered data; protects the write loop from dereffing a freed CPartFile next tick) Validate-before-deref defences added at three event-handler sites where the broadcast pattern doesn't fit (events already in flight on the wx queue, the dangling pointer is in the event payload): - OnFinishedAICHHashing — check IsKnownFile(owner) || IsPartFile(owner) before swapping hashsets; AICH hashing of a huge file can run for minutes during which the CKnownFile can be evicted - OnFinishedAllocation — wxASSERT was no-op in Release; convert to real check on IsPartFile() so the cancel-during-preallocation window (~10 s on slow disks) stops UAF'ing - OnFinishedCompletion — same fix OnFinishedHashing already had the validate-before-deref pattern; left in place. New helper: - CKnownFileList::IsKnownFile(const CKnownFile*) — pointer-value scan over m_knownFileMap, safe to call with a possibly-freed pointer (no deref). Symbol audit recap (see PR description for full table): Site Status CGenericClientListCtrl::m_knownfiles FIXED (amule-project#755) ClientCtrlItem_Struct::m_owner FIXED (rows pruned) CCommentDialog::m_file FIXED CCommentDialogLst::m_file FIXED CFileDetailDialog::m_file + m_files FIXED CUpDownClient::m_uploadingfile / m_reqfile FIXED (refactored amule-project#749) CAICHHashSet::m_liRequestedData FIXED CHashingEvent::m_owner / m_result FIXED (validate) CAllocFinishedEvent::m_file FIXED (validate) CCompletionEvent::m_owner FIXED (validate) CPartFileWriteThread::m_flushList FIXED CClientRef holders (Kad sources, upload list, friend list, …) SAFE (refcounted) Owner containers (m_Files_map, m_filelist, …) SAFE (owner pairing) CAICHHashSet::m_pOwner, CFileStatistic::fileParent SAFE (1:1 composition) Tested: - amule (full client, Debug, Ubuntu 25.10 aarch64) builds clean - amulegui (remote GUI, Debug) builds clean - amuled (headless daemon, Debug) builds clean - Smoke-test: amuled startup + shutdown clean (no crashes; broadcast fires for every known file during CKnownFileList::Clear without subscriber misbehaving) Supersedes amule-project#749. Closes amule-project#755. Refs amule-project#748.
got3nks
force-pushed
the
pr-uaf-broadcast-hook
branch
from
May 27, 2026 23:11
741c631 to
5952485
Compare
1 task
got3nks
added a commit
to got3nks/amule
that referenced
this pull request
Jun 4, 2026
…ndex Adds 55+ merged PRs to the 3.0.0 changelog since the last update (amule-project#747, 2026-05-27). Narrative additions cover: - Packaging: expanded the top list to include the macOS per-arch .app bundles and the Windows NSIS installer alongside the existing AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785 (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796 (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths). - Bug Fixes & Stability: post-amule-project#744 fixes including EC notification leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728), amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760), PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788, amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756), Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833, amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774). - Translations: late-cycle wave covering French/Turkish manpages (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for date+version drift (amule-project#802). - Contributors: added ngosang for UX feedback on the late-3.0 cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the user-facing manual at amule-org.github.io. - Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.
got3nks
added a commit
to got3nks/amule
that referenced
this pull request
Jun 4, 2026
…ndex Adds 55+ merged PRs to the 3.0.0 changelog since the last update (amule-project#747, 2026-05-27). Narrative additions cover: - Packaging: expanded the top list to include the macOS per-arch .app bundles and the Windows NSIS installer alongside the existing AppImage / Flatpak / .dmg / .zip entries. New bullets for amule-project#785 (alc/alcc/cas/wxcas everywhere + Windows amuleweb), amule-project#794 (.dmg amuleweb path), amule-project#789 (<OS>-<arch> artifact naming), amule-project#780 / amule-project#796 (Windows DPI + comctl32 manifest), amule-project#784 (FHS share/amule paths). - Bug Fixes & Stability: post-amule-project#744 fixes including EC notification leak (amule-project#797), big-library scaling (amule-project#736, amule-project#840 superseding amule-project#728), amulegui ghost entries (amule-project#810, amule-project#819, amule-project#841, amule-project#824, amule-project#830, amule-project#760), PartFile early hash (amule-project#762), server protocol fixes (amule-project#835, amule-project#788, amule-project#721, amule-project#787), crypto stream UB (amule-project#779), UAF prevention (amule-project#756), Kad rotation (amule-project#795, amule-project#799/amule-project#805), GTK warning silencing (amule-project#833, amule-project#826/amule-project#836), and the clang-tidy worklist (amule-project#770, amule-project#772-amule-project#774). - Translations: late-cycle wave covering French/Turkish manpages (amule-project#753/amule-project#754/amule-project#776), Galician (amule-project#763), Slovenian (amule-project#771), pt-BR (amule-project#768/amule-project#775/amule-project#812), French (amule-project#811), plus man-page tooling for date+version drift (amule-project#802). - Contributors: added ngosang for UX feedback on the late-3.0 cycle (amule-project#817/amule-project#818/amule-project#821/amule-project#828/amule-project#844) and ongoing work on the user-facing manual at amule-org.github.io. - Merged PRs flat index: extended with amule-project#746-amule-project#845 + amule-project#841.
mrjimenez
pushed a commit
that referenced
this pull request
Jun 4, 2026
Adds 55+ merged PRs to the 3.0.0 changelog since the last update (#747, 2026-05-27). Narrative additions cover: - Packaging: expanded the top list to include the macOS per-arch .app bundles and the Windows NSIS installer alongside the existing AppImage / Flatpak / .dmg / .zip entries. New bullets for #785 (alc/alcc/cas/wxcas everywhere + Windows amuleweb), #794 (.dmg amuleweb path), #789 (<OS>-<arch> artifact naming), #780 / #796 (Windows DPI + comctl32 manifest), #784 (FHS share/amule paths). - Bug Fixes & Stability: post-#744 fixes including EC notification leak (#797), big-library scaling (#736, #840 superseding #728), amulegui ghost entries (#810, #819, #841, #824, #830, #760), PartFile early hash (#762), server protocol fixes (#835, #788, #721, #787), crypto stream UB (#779), UAF prevention (#756), Kad rotation (#795, #799/#805), GTK warning silencing (#833, #826/#836), and the clang-tidy worklist (#770, #772-#774). - Translations: late-cycle wave covering French/Turkish manpages (#753/#754/#776), Galician (#763), Slovenian (#771), pt-BR (#768/#775/#812), French (#811), plus man-page tooling for date+version drift (#802). - Contributors: added ngosang for UX feedback on the late-3.0 cycle (#817/#818/#821/#828/#844) and ongoing work on the user-facing manual at amule-org.github.io. - Merged PRs flat index: extended with #746-#845 + #841.
mrjimenez
pushed a commit
to mrjimenez/amule
that referenced
this pull request
Aug 8, 2026
…mule-project#845) Queueing a search result makes it known, and since the wxDataViewCtrl port every result update marks the model dirty and flushes a full Cleared(), so the filter predicate re-runs straight away. With "Hide Known Files" on, the row the user just double-clicked is gone by the next idle -- before the colour that confirms the download was added has ever been on screen, and with nothing left to say which results were taken. Users who keep the option on permanently lose any feedback that adding worked (amule-project#756). Before the port UpdateResult() patched the row's cells in place and had no path that could remove it, so a result that became known simply recoloured and stayed. Restoring that by making updates incremental again is not available: ItemChanged() and delete+re-add were both tried during the port and neither made GTK/MSW re-derive container-ness on group formation, which is why every change became a coalesced reset. So exempt the results the user queued from this list instead. They are recorded by hash in DownloadSelected() -- grouping only ever pairs results whose hashes match, so one insert covers a group and its variants -- and IsFiltered() skips the known test for them. SetFilter() and ShowResults() clear the set, so re-filtering or a new search collapses the kept rows away. Results that were already known are still hidden by the same live status test as before. Keyed on the user's action rather than on when a result became known, because those differ between the two GUIs: amulegui constructs every result NEW and only learns the real status from a later poll (CSearchListRem::ProcessItemUpdate), so "already known on arrival" is not a question it can answer, while the monolithic build computes it up front in CSearchFile::SetDownloadStatus(). Hooking the GUI action keeps one code path correct for both.
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.
Summary
aMule has 8 latent use-after-free sites where raw
CKnownFile*/CPartFile*pointers can outlive the pointee. ASan caught one of them in #755 (CGenericClientListCtrl::m_knownfiles), and the production SEGV in #748 hit another (CUpDownClient::m_uploadingfile/m_reqfile, fixed in #749). The other 6 are reachable through ordinary user actions but produce silent 1-byte heap corruption rather than crashes on non-ASan builds.This PR fixes all 8 with a single discipline: a new
Notify_KnownFileBeingDestroyed(file)broadcast that fires from everyCKnownFile-destruction site before thedelete. Subscribers strip references using pointer-value comparison only — never dereference, since main-thread queued subscribers may run after the file is freed. Contract documented inGuiEvents.h.Audit table
CGenericClientListCtrl::m_knownfiles+ClientCtrlItem_Struct::m_ownerCCommentDialog::m_fileCCommentDialogLst::m_fileCFileDetailDialog::m_file+m_filesCAICHHashSet::m_liRequestedDataCHashingEvent::m_owner/m_resultOnFinishedHashing); addedIsKnownFile()check inOnFinishedAICHHashing(previously deref'd unchecked)CAllocFinishedEvent::m_file/CCompletionEvent::m_ownerwxASSERT_MSGwhich is no-op in Release)CPartFileWriteThread::m_flushListCUpDownClient::m_uploadingfile/m_reqfileDropReferencesTonow called by the broadcast handlerCleared as safe in the audit (do not need the broadcast):
CClientRef-based holders (refcounted), owner containers (m_Files_map,m_filelist,m_knownFileMap, EC mirrors),CAICHHashSet::m_pOwner+CFileStatistic::fileParent(1:1 composition).Fire sites
CPartFile::Delete()— user cancel.CKnownFileList::Clear()— daemon shutdown.CKnownFileList::PruneDuplicates()× 3 — TTL eviction + per-hash cap.CKnownFilesRem::DeleteItem— amuleguiEC_TAG_FILE_REMOVEDpath.Subscribers per build
amule+amulegui: 2×CGenericClientListCtrl(transfer-window'sclientlistctrl, shared-window'speerslistctrl), the 3 dialog classes.amuleguionly:CUpDownClientListRem::DropReferencesTo(was amule-remote-gui: null dangling client→file refs before deleting a CKnownFile #749).amuledaemon only:CAICHHashSet,CPartFileWriteThread.Validate-before-deref additions
Three event handlers receive a
CKnownFile*/CPartFile*in the event payload; the broadcast can't reach into the wx event queue to strip them. Solution: validate the pointer is still in its canonical container before dereferencing. This pattern was already inOnFinishedHashing(it commented "Check if the partfile still exists, as it might have been deleted in the mean time"); the PR extends it to:OnFinishedAICHHashing—IsKnownFile(owner) || IsPartFile(owner)before hashset swap. Multi-GB AICH hashing can run for minutes during which the owner can be TTL-evicted.OnFinishedAllocation— replacewxASSERT_MSG(no-op in Release) with a realIsPartFilecheck. Preallocation can take 10+ seconds on slow disks; cancel-during-preallocation was reachable.OnFinishedCompletion— same fix.New helper:
CKnownFileList::IsKnownFile(const CKnownFile*)— pointer-value scan, safe with a possibly-freed pointer.Test plan
amule(full GUI),amulegui(remote GUI),amuled(daemon), Debug mode, Ubuntu 25.10 aarch64.unittests/tests/still pass (the affected classes are out of scope of the existing test surface).amuledstartup + clean SIGTERM shutdown.CKnownFileList::Clearfires the broadcast for every known file during shutdown; no crashes, no aborts, "aMule, arresto completato" reached.Diff stats
20 files changed, 530 insertions(+), 3 deletions(-)
Note
Supersedes #749. The narrower #749 fix is correct for its scope; this PR's
CUpDownClientListRem::DropReferencesTois the same code routed through the broadcast for consistency with the other 7 subscribers.