SearchManager: partition uint32 search-ID space by top bit - #533
Closed
got3nks wants to merge 1 commit into
Closed
Conversation
CSearchManager::m_nextID (Kad-allocated search IDs) and CSearchDlg::m_nSearchID (ed2k Local/Global search IDs) both start at 0 and increment freely on every search. CSearchList keys results by search ID across both networks, so a long enough session lets the two counters collide and route Kad results into the ed2k tab (or vice versa). Reported as amule-project#530, where the proposed fix offsets the Kad counter by 1024 — an empirical buffer that delays the collision but does not prevent it (a session with >=1024 user-initiated Local / Global searches re-introduces the bug). Partition the 32-bit ID space by the top bit so the two allocators can never produce equal values regardless of session length: ed2k IDs 0x00000000 .. 0x7FFFFFFF (top bit = 0) Kad IDs 0x80000000 .. 0xFFFFFEFF (top bit = 1) The two sets are disjoint by construction. This fits the existing convention of high-bit tagging in this same file: SearchManager.cpp already reserves the top byte (0xFFFFFF00 .. 0xFFFFFFFF) for the related-search "use supplied ID as-is" path, so high-bit reservations are already a pattern; this just splits the remaining space cleanly between the two allocators. Two surgical edits, both call-site preserving: - src/kademlia/kademlia/SearchManager.cpp:59 m_nextID starts at 0x80000000 instead of 0. The Kad counter only ever increments, so it stays in the top half. Comment block on the decl spells out the partition contract. - src/SearchDlg.cpp:509 m_nSearchID++ -> m_nSearchID = (m_nSearchID + 1) & 0x7fffffff The mask clears the top bit on every increment, keeping ed2k IDs in the bottom half even after 2^31 increments (where unmasked ++ would wrap into the Kad range). Both counters stay where they are; no cross-module refactor, so the amule-remote-gui link doesn't break the way amule-project#530's single-shared- counter attempt did.
Contributor
Author
|
Superseded by #530, which now incorporates the top-bit partition with the overflow-safe mask-on-emit + dead-code cleanup. |
got3nks
added a commit
to got3nks/amule
that referenced
this pull request
Jul 20, 2026
…ect#533) The Flatpak job regenerates ECCodes.h from ECCodes.abstract rather than using the committed header, and amule-project#530's comments in that file made the generated header unparseable ("invalid preprocessing directive #=" plus a truncated ECOpCodes enum), failing every Flatpak build on master. The generator only treats a line as a comment when it matches "^#" with no leading whitespace, so the comments indented to line up with the tag entries fell through to the data path and had every word turned into a field. It also flattens the file into a CMake list by replacing newlines with ";", so a semicolon inside a comment splits the line and the remainder loses its leading "#". Documentation for these codes lives in the hand-maintained header, so the abstract keeps to entries. Generating before and after: master produces 8 mangled lines and 11 compile errors, this produces none, with all seven new codes intact.
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
1 participant
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.
Alternative to #530 — closing the same bug at the structural level rather than buffering past it. Bug + dual-counter root-cause diagnosis are both @danim7's — see #530 for the full repro with screenshots and the analysis pointing at the two-counter collision.
Bug
CSearchManager::m_nextID(Kad-allocated search IDs) andCSearchDlg::m_nSearchID(ed2k Local/Global search IDs) are two independent counters that both start at0and increment freely on every search.CSearchListkeys results by search ID across both networks, so a long-enough session lets the two counters collide and route Kad results into the ed2k tab (or vice versa).Why this approach
#530 offsets the Kad counter by
1024— a buffer that delays the collision but does not prevent it: a session with ≥1024 user-initiated Local/Global searches re-introduces the bug.This patch partitions the 32-bit ID space by the top bit so the two allocators can never produce equal values, regardless of session length:
0x00000000–0x7FFFFFFFCSearchDlg::m_nSearchID)0x80000000–0xFFFFFEFFCSearchManager::m_nextID)The two sets are disjoint by construction. The partition also fits the existing convention in this same file —
SearchManager.cpp:169already reserves the top byte (0xFFFFFF00..0xFFFFFFFF) for the related-search "use supplied ID as-is" path, so high-bit reservations of the search-ID space are already a pattern; this just splits the remaining space cleanly between the two ID-incrementing allocators.Diff
Two surgical edits, both call-site preserving (no cross-module refactor →
amule-remote-guilink unaffected, which was the snag in #530's single-shared-counter attempt):The mask in
SearchDlg.cppkeeps ed2k IDs in the bottom half even after2^31increments (where an unmasked++would wrap into the Kad range).