fix: drop LowID-0 sources in CanAddSource - #790
Merged
Conversation
A LowID of 0 has no callback semantics — server LowID assignments start at 1, so an OP_CALLBACKREQUEST aimed at LowID 0 can't be routed. The source is unreachable, and accepting it spins a CClientTCPSocket through the listensocket pool until CheckTimeOut sweeps it ~60 s later with ED2k Client: --- Deleted client D:4 U:8 "Client unknown on IP:Port 0.0.0.0:<port> using unknown unknown"; Reason was Timeout in the debug log (mifritscher2's report in amule-project#786). The two CanAddSource callers that can supply userid==0 — server source list (PartFile.cpp:1809) and source exchange (PartFile.cpp:3021) — both skip IsGoodIP for LowID ids, so a LowID 0 currently passes through. SearchFile.cpp:178 already pre-gates on a non-zero ClientID. Kad sources either run through IsGoodIP for HighID (DownloadQueue.cpp:1614) or hardcode userid==1 for Kad-LowID synthesised entries (DownloadQueue.cpp:1638/1664), so none of them can reach this gate with id==0. The Kad-side choice of 1 (not 0) as the synthetic LowID is itself confirmation that 0 isn't treated as a usable id elsewhere in the codebase. HighID 0 (IP 0.0.0.0) is intentionally left to the upstream IsGoodIP filter that already gates both callers; this guard is scoped narrowly to the LowID case so the inbound-connection recovery path at BaseClient.cpp:672-677 — which intentionally takes m_nUserIDHybrid == 0 from older HighID clients and patches it from the socket peer IP — is untouched. That path goes through the incoming-socket constructor and never reaches CanAddSource.
got3nks
force-pushed
the
fix/drop-zero-id-sources
branch
from
May 31, 2026 15:38
e006117 to
60a5e9a
Compare
mrjimenez
pushed a commit
to mrjimenez/amule
that referenced
this pull request
Aug 4, 2026
… defaults (amule-project#790) * refactor(gui): build the tray speed presets in one place The ladder of limit presets was written out three times -- once in the appindicator backend's submenu builder and once each for upload and download in the wx backend -- and the two backends had drifted. The appindicator one passes its labels straight to GTK, so "Unlimited" and "kB/s" were never translated there, while the wx one ran both through _(). One of them also formatted an unsigned with %d. One helper produces the speeds and one produces the labels, so both backends show the same strings in the user's language. The clamp moves into the helper with them. The appindicator backend was clamping in its two callers and then handing the result to a builder that has to cope with any value anyway; there is now a single place that decides what an unusable capacity turns into. No change to the numbers offered. Raising the stale defaults they are derived from is a separate question. * fix(gui): spread the tray limit presets, and raise the stale capacity The tray offered fifths of the configured line capacity, whose defaults were 300 and 100 kB/s. On any current connection the top entry was already a heavy throttle, which is what amule-project#788 reports. Two changes, and the first helps everyone without touching a config. The ladder is no longer even. Fifths span a single factor of five, so on a fast line every entry lands high and there is no way to throttle hard from the tray, while on a slow one they bunch near the top. Dividing by 1, 2, 4, 10 and 50 instead covers full speed down to a heavy throttle from the same capacity -- at the old default of 300 that is 300/150/75/30/6 rather than 300/240/180/120/60, better reach downward with the same ceiling. The capacity defaults move to 100/20 Mbit, converted at 1024. They are not limits: capacity is what the line can do, and the traffic graph scales from it too. Only new configurations see this -- aMule writes every key on save, so an existing amule.conf already carries the old value and wxConfig applies a default only when the key is absent. That is also why the ladder change matters on its own. Deriving the presets from the configured limit instead was considered and rejected: the menu could then only ever lower it, since every entry would be at or below the cap in force, and the shipped default of unlimited leaves nothing to scale from at all. * chore(i18n): regenerate catalogs after the tray menu refactor
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
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.
Drops sources with
LowID && id == 0at theCanAddSourcegate. Fixes the spurious"0.0.0.0:port ... Timeout"log lines reported in #786.Why this is safe
A LowID of 0 has no callback semantics — server LowID assignments start at 1, so an
OP_CALLBACKREQUESTaimed at LowID 0 can't be routed to anything. The peer is unreachable in any code path. The Kad code's own choice of1(not0) as the synthetic LowID for Kad-firewalled sources at DownloadQueue.cpp:1638 / :1664 is itself confirmation that0isn't treated as a usable id elsewhere in the codebase.Accepting a LowID-0 source currently constructs a
CUpDownClient, which creates aCClientTCPSocketinTryToConnect(BaseClient.cpp:1490). The socket is neverconnect()ed — it sits idle inlistensocket's pool untilCheckTimeOut(ClientTCPSocket.cpp:147) firesDisconnect("Timeout")~60 s later and the client gets cleaned up. That's the noisy log line in #786.What this PR deliberately does NOT touch
0.0.0.0) is already filtered upstream byIsGoodIPin both server-source-list (PartFile.cpp:1799) and source-exchange (PartFile.cpp:3005) callers. The new gate is scoped narrowly to LowID 0 —IsLowID(hybridID) && hybridID == 0— so the HighID path is unchanged.m_nUserIDHybrid == 0. BaseClient.cpp:672-677 intentionally tolerates older HighID clients that connect to us without sending their id in HELLO and patches the id fromsocket->GetPeerInt(). That path uses the single-arg incoming-socket constructor at BaseClient.cpp:98 and never reachesCanAddSource.Path coverage
CanAddSourceCanAddSourceSearchFile::MergeClientID != 0(SearchFile.cpp:178)IsGoodIP(ED2KID)filter at DownloadQueue.cpp:16141, never0(DownloadQueue.cpp:1638 / :1664)OP_CALLBACKREQUESTEDfrom serverCanAddSource(usesdwIPdirectly)CanAddSource(single-arg constructor, id patched at BaseClient.cpp:676)FriendListCanAddSourceNet effect
One extra rejection in
CanAddSourcefor the LowID-0 case. Eliminates the per-source ~60 s oflistensocketpool burn and the corresponding"Timeout"log spam.Refs #786.