Skip to content

kad: cap CKeyEntry::m_filenames so the list can't grow unbounded - #652

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/kad-keyentry-filenames-unbounded-leak
May 18, 2026
Merged

kad: cap CKeyEntry::m_filenames so the list can't grow unbounded#652
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/kad-keyentry-filenames-unbounded-leak

Conversation

@got3nks

@got3nks got3nks commented May 18, 2026

Copy link
Copy Markdown
Contributor

Bug investigation in #314.

@ngosang's 20-min heaptrack of amuled (after 6 h warm-up on a 7 TB shareset) showed leaks heavily concentrated in the Kademlia keyword-index code path:

289.57K leaked over 87497 calls from
  Kademlia::CEntry::sFileNameEntry(sFileNameEntry const&)
    at Entry.h:59
  std::__cxx11::list<sFileNameEntry>::push_front
  Kademlia::CEntry::SetFileName
    at Entry.cpp:118
  Kademlia::CKademliaUDPListener::Process2PublishKeyRequest
    at KademliaUDPListener.cpp:1036

Plus ~10 more entries in the top-30 leak list traced to the same handful of call sites in KademliaUDPListener.cpp and Indexed.cpp. Total leaked in 20 min: ~2.36 MB, which extrapolates to ~170 MB/day — consistent with the multi-day creep the issue originally reported.

Root cause

CKeyEntry::MergeIPsAndFilenames (src/kademlia/kademlia/Entry.cpp:350) merges the incoming entry's m_filenames list into the stored entry's, one sFileNameEntry per distinct publisher-chosen name. The publishing-IP list right next to it caps at 100 (:429-432); the filename list never got the matching cap. A popular file accumulates one entry per distinct filename variant — language renames, mirror prefixes, transliterations, case changes, trailing-paren copies — and the list grows monotonically for the lifetime of the entry.

GetCommonFileName already walks the list and returns the highest-m_popularityIndex entry as the displayed name, so the long-tail filename variants past a few dozen are read-only ballast.

Fix

Cap m_filenames at 100 entries (matching the m_publishingIPs cap), popularity-ordered: when the list grows past the cap, sort by m_popularityIndex descending and resize. GetCommonFileName's "most popular wins" semantics are preserved since the survivors are exactly the entries with the highest popularity counts.

What this doesn't change

CIndexed (the actual keyword → file index) is untouched. Search queries land there via keyword hash, not via per-file filename lists, and the KADEMLIAMAXINDEX = 60000 cap that protects it is unrelated. The filename list inside each CKeyEntry only ever feeds GetCommonFileName() for the displayed name in search-result rendering — capping it doesn't change which files appear or whether amuled answers a search, only which name shows next to a file when multiple publishers used multiple names.

eMule's reference code caps the equivalent list on the same shape; we just never ported that half when the 100-IP cap below was added.

…le-project#314)

CKeyEntry::MergeIPsAndFilenames merges the incoming entry's filename
list into the stored entry's, one sFileNameEntry per distinct
publisher-chosen name. The publishing-IP list right next to it caps
at 100 (Entry.cpp:429-432); the filename list never got the matching
cap. A popular file accumulates one entry per distinct filename
variant -- language renames, mirror prefixes, transliterations,
case changes, trailing-paren copies -- and the list grows
monotonically for the lifetime of the entry.

ngosang's 20-min heaptrack of amuled (after 6 h warm-up, 7 TB
shareset) showed ~2.36 MB leaked, with the leak headers
overwhelmingly traced to:

  Kademlia::CEntry::sFileNameEntry(sFileNameEntry const&)
    std::list<sFileNameEntry>::push_front
    CEntry::SetFileName (Entry.cpp:118)
    CKademliaUDPListener::Process2PublishKeyRequest

Extrapolated rate ~170 MB/day, matching the multi-day RSS creep the
issue originally reported.

Cap m_filenames at 100 entries (matching the m_publishingIPs cap
below), popularity-ordered: when the list grows past the cap, sort
by m_popularityIndex descending and resize. GetCommonFileName's
"most popular wins" semantics are preserved since the survivors are
exactly the entries with the highest popularity counts.

The CIndexed keyword-to-file map is untouched. Search queries land
there via keyword hash, not via per-file filename lists, and the
KADEMLIAMAXINDEX = 60000 cap that protects it is unrelated.
m_filenames inside each CKeyEntry only ever feeds GetCommonFileName
for the displayed name in search-result rendering -- capping it
doesn't change which files appear in results, only which name shows
next to a file when multiple publishers used multiple names.

eMule's reference code caps the equivalent list on the same shape;
amule never ported that half when the 100-IP cap was added.
@mrjimenez
mrjimenez merged commit c95e002 into amule-project:master May 18, 2026
12 checks passed
@got3nks
got3nks deleted the fix/kad-keyentry-filenames-unbounded-leak branch May 22, 2026 13:51
ngosang added a commit to ngosang/amule that referenced this pull request Jul 27, 2026
…mule-project#652)

The Preferences view rendered every field emitted by the amuleapi
/preferences endpoint except two, so the Web UI could neither show nor
edit them even though both round-trip fine over EC:

- files.create_normal (EC_TAG_FILES_CREATE_NORMAL, the inverse of
  /eMule/CreateSparseFiles). Left out by amule-project#599 as "INI-only", but it is
  a functional pref the daemon accepts on PATCH. Added to the Files tab
  next to alloc_full_size, its sibling disk-allocation policy.
- core_tweaks.verbose (EC_TAG_CORETW_VERBOSE). It lives on the desktop
  Debugging page, which preferences.js deliberately skips; its real API
  category is core_tweaks, so it goes on the Advanced tab -- the same
  treatment files.mmap_enabled already gets there.

Both are plain type: "bool" entries in TABS[], so buildField/collect
handle them with no engine change, and the guest read-only path comes
for free from the existing isGuest gate. en/es i18n keys added in
lockstep.

Auditing the Spanish side of amule-project#599 turned up two stale entries, fixed
here with the wording the desktop catalog (po/es.po) already uses:

- prefs_field_files_endgame was never translated and still read in
  English -> "Modo endgame: cambiar a fuentes mas rapidas para los
  bloques finales".
- prefs_field_security_can_see_shares kept its old checkbox phrasing
  ("Permitir que otros vean mis archivos compartidos") after the field
  became a 3-state select, so the UI read "Permitir que otros vean mis
  archivos compartidos: [Todos]" -> "Quien puede ver mis archivos
  compartidos".

check-i18n.mjs only compares keys and placeholders, so neither slipped
past it; it stays green at 736 keys per locale and no prefs_field_* is
left untranslated.

Verified against a running daemon: GET returns both new fields, PATCH
echoes and persists them, and re-parsing WritePreferencesBody() against
TABS[] now reports no missing field in any of the 12 categories.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants