Skip to content

fix: drop LowID-0 sources in CanAddSource - #790

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/drop-zero-id-sources
May 31, 2026
Merged

fix: drop LowID-0 sources in CanAddSource#790
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/drop-zero-id-sources

Conversation

@got3nks

@got3nks got3nks commented May 31, 2026

Copy link
Copy Markdown
Contributor

Drops sources with LowID && id == 0 at the CanAddSource gate. 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_CALLBACKREQUEST aimed at LowID 0 can't be routed to anything. The peer is unreachable in any code path. The Kad code's own choice of 1 (not 0) as the synthetic LowID for Kad-firewalled sources at DownloadQueue.cpp:1638 / :1664 is itself confirmation that 0 isn't treated as a usable id elsewhere in the codebase.

Accepting a LowID-0 source currently constructs a CUpDownClient, which creates a CClientTCPSocket in TryToConnect (BaseClient.cpp:1490). The socket is never connect()ed — it sits idle in listensocket's pool until CheckTimeOut (ClientTCPSocket.cpp:147) fires Disconnect("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

  • HighID 0 (IP 0.0.0.0) is already filtered upstream by IsGoodIP in 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.
  • Inbound legacy HighID with 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 from socket->GetPeerInt(). That path uses the single-arg incoming-socket constructor at BaseClient.cpp:98 and never reaches CanAddSource.

Path coverage

Source path LowID 0 reachable today? After this PR
Server source list Yes Rejected at CanAddSource
Source exchange from peer Yes Rejected at CanAddSource
SearchFile::Merge No — caller already gates on ClientID != 0 (SearchFile.cpp:178) Unchanged
Kad type 1/4 (HighID) No — IsGoodIP(ED2KID) filter at DownloadQueue.cpp:1614 Unchanged
Kad type 3/5/6 (firewalled) No — userid hardcoded to 1, never 0 (DownloadQueue.cpp:1638 / :1664) Unchanged
OP_CALLBACKREQUESTED from server Not via CanAddSource (uses dwIP directly) Unchanged
Inbound legacy HighID without HELLO id Not via CanAddSource (single-arg constructor, id patched at BaseClient.cpp:676) Unchanged
FriendList Not via CanAddSource Unchanged

Net effect

One extra rejection in CanAddSource for the LowID-0 case. Eliminates the per-source ~60 s of listensocket pool burn and the corresponding "Timeout" log spam.

Refs #786.

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
got3nks force-pushed the fix/drop-zero-id-sources branch from e006117 to 60a5e9a Compare May 31, 2026 15:38
@got3nks got3nks changed the title fix: drop sources with id == 0 in CanAddSource fix: drop LowID-0 sources in CanAddSource May 31, 2026
@mrjimenez
mrjimenez merged commit e586f3f into amule-project:master May 31, 2026
7 checks passed
@got3nks
got3nks deleted the fix/drop-zero-id-sources branch June 3, 2026 14:16
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
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