feat(udp-server): discard requests from clients with source port 0 - #2017
Merged
josecelano merged 18 commits intoJul 22, 2026
Merged
Conversation
Adds the issue spec and pre-fix manual verification evidence for issue torrust#1450: discard UDP requests from clients with source port 0. The evidence captures the WARN log produced by the original tracker when a crafted UDP datagram with source port 0 is received: WARN process_request:send_response{...}: failed to send bytes_count=16 error=Invalid argument (os error 22) The spec explains why port 0 is possible in UDP (RFC 768, connectionless protocol), the design decision (early discard + stats counter, no per- request log), and how to reproduce the bug manually using a raw Python socket script.
Fixes torrust#1450. UDP datagrams with source port 0 are undeliverable — sending a response to <ip>:0 always fails with EINVAL (os error 22). Previously the tracker processed the full request and only discovered the problem at send_to time, producing a noisy WARN log in production. Changes: - Add early guard in Processor::process_request: if client port == 0, emit a UdpRequestDiscarded stats event and return immediately before any parsing or handler invocation. - Add Event::UdpRequestDiscarded to event.rs. - Add udp_tracker_server_requests_discarded_total metric counter. - Add statistics event handler request_discarded.rs (follows the pattern of request_aborted.rs). - Register the new handler in the event handler dispatch table. - Add unit tests: - handler increments the discarded counter on UdpRequestDiscarded. - processor discards the request (no response, counter incremented) when client source port is 0.
Wires the new UdpRequestDiscarded counter into the REST API stats response so operators can observe discarded port-0 requests via the /api/v1/stats endpoint alongside the existing aborted/banned counters. Changes: - Add udp_requests_discarded field to Stats resource struct. - Map udp_requests_discarded_total() from UDP server metrics in the stats adapter. - Add plain-text line to the stats response formatter. - Update the contract test fixture with the new field (value 0). - Add after-fix manual verification evidence for issue torrust#1450. Verified end-to-end: udp_requests_discarded: 1 (after one crafted port-0 datagram) udp4_responses: 0 (no response sent) WARN log: absent (no more 'os error 22' noise)
josecelano
force-pushed
the
1450-discard-udp-requests-from-clients-with-port-0
branch
from
July 21, 2026 17:40
15acb8a to
9b647b4
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds an early guard in the UDP server to discard requests coming from clients whose source port is 0, avoiding guaranteed send_to failures (EINVAL) and replacing noisy WARN logs with a dedicated discard statistic exposed via Prometheus and the REST /api/v1/stats response.
Changes:
- Add a new UDP-server stats event (
UdpRequestDiscarded) and Prometheus counter (udp_tracker_server_requests_discarded_total), including handler + repository/metrics support. - Discard port-0 requests at the start of
Processor::process_requestand emit the new discard event. - Expose
udp_requests_discardedthrough the REST stats protocol/adapter and update contract tests; add issue spec + before/after evidence docs.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| project-words.txt | Adds new technical words for spell checking (needs sorted insertion fixes). |
| packages/udp-server/src/statistics/repository.rs | Extends repository tests to include the new discarded counter. |
| packages/udp-server/src/statistics/mod.rs | Defines + describes the new discarded counter. |
| packages/udp-server/src/statistics/metrics.rs | Adds a getter for udp_requests_discarded_total(). |
| packages/udp-server/src/statistics/event/handler/request_discarded.rs | New event handler + unit test to increment the discarded counter. |
| packages/udp-server/src/statistics/event/handler/mod.rs | Registers dispatch for UdpRequestDiscarded. |
| packages/udp-server/src/server/processor.rs | Adds the port-0 early discard guard + a unit test for discard/stat emission (test needs adjustment). |
| packages/udp-server/src/event.rs | Adds Event::UdpRequestDiscarded. |
| packages/rest-api-runtime-adapter/src/v1/adapters/stats.rs | Wires discarded requests into the REST stats adapter output. |
| packages/rest-api-protocol/src/v1/context/stats/resources/stats.rs | Adds udp_requests_discarded to the Stats REST payload. |
| packages/axum-rest-api-server/tests/server/v1/contract/context/stats.rs | Updates stats contract test to include the new field. |
| packages/axum-rest-api-server/src/v1/context/stats/responses.rs | Adds udp_requests_discarded to the metrics-style response output. |
| docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/ISSUE.md | New issue spec documenting rationale, design, and acceptance criteria. |
| docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/before-fix-manual-verification.md | Captures pre-fix WARN/EINVAL evidence and reproduction steps. |
| docs/issues/open/1450-discard-udp-requests-from-clients-with-port-0/evidence/after-fix-manual-verification.md | Captures post-fix evidence: no WARN + discarded counter increment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (5)
project-words.txt:92
project-words.txtis no longer in case-insensitive alphabetical order:dportis placed beforedatagramand should be moved to its correct sorted position (afterdpkg). This can cause noisy diffs and makes the dictionary harder to maintain.
dashmap
dport
datagram
datagrams
datetime
project-words.txt:172
project-words.txtis no longer in case-insensitive alphabetical order:HDRINCLshould sort betweenhasherandhealthcheck, not afterHydranode.
hotspots
httpclientpeerid
Hydranode
HDRINCL
hyperium
project-words.txt:236
project-words.txtis no longer in case-insensitive alphabetical order:middleboxshould come beforemiddlewares.
metainfo
microbenchmark
microbenchmarks
middlewares
middlebox
project-words.txt:364
project-words.txtis no longer in case-insensitive alphabetical order:sendtoshould sort afterSeedableand beforeserde, and it should not appear betweensarifandsavepath.
sarif
sendto
savepath
scanf
sccache
packages/udp-server/src/server/processor.rs:231
- The unit test uses a fixed
sleep(50ms)to wait for the async stats listener, which can be flaky on slow/loaded CI runners. Also, assertingudp4_requests_received_total() == 0is a bit misleading since the real server increments the "received" counter inlauncher.rsbeforeProcessor::process_requestruns (so a port-0 datagram can still count as received). Prefer a bounded poll/timeout waiting forudp_requests_discarded_total == 1, and assert that no responses were sent instead.
// Give the async event listener time to process the emitted event.
tokio::time::sleep(Duration::from_millis(50)).await;
// The discarded counter must be 1; all other counters must stay at 0.
let stats = container.udp_tracker_server_container.stats_repository.get_stats().await;
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2017 +/- ##
===========================================
+ Coverage 81.46% 81.56% +0.10%
===========================================
Files 341 341
Lines 24208 24295 +87
Branches 24208 24295 +87
===========================================
+ Hits 19721 19817 +96
+ Misses 4188 4179 -9
Partials 299 299 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- project-words.txt: fix alphabetical ordering of dport, HDRINCL, middlebox, and sendto (all were placed out of case-insensitive order) - processor.rs: replace fixed sleep(50ms) with bounded timeout+poll to avoid flaky behaviour on slow CI runners - processor.rs: clarify assertion message for udp4_requests_received_total to reflect that UdpRequestReceived is emitted by the launcher, not the processor, so the counter is always 0 in unit tests that bypass the launcher
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
project-words.txt:92
- project-words.txt is kept in case-insensitive alphabetical order; the newly added word
dportis currently out of order (it should come afterdpkg). This will cause unnecessary churn/conflicts in future dictionary edits.
dashmap
datagram
datagrams
dport
datetime
project-words.txt:327
- project-words.txt ordering:
recvfromshould be placed after the otherreco*words (e.g.,recognised,recompiles) to keep the dictionary sorted.
reannounce
recaches
recvfrom
recognised
recompiles
- project-words.txt: fix case-insensitive sort order for nmap/nping (moved after new*/nextest/nghttp/ngtcp/nocapture/nologin) and for recvfrom (moved after recognised and recompiles) - processor.rs: replace 'source port 0 is invalid' with more accurate wording noting RFC 768 does not forbid port 0; the real issue is the OS rejects send_to to port 0 with EINVAL
9 threads processed (all action): 6 in first round, 3 after re-review on push. All threads replied and resolved.
Replace the single combined test with two focused tests, each covering one responsibility of Processor::process_request for the port-0 case: - processor_does_not_send_a_response_when_client_port_is_0 Asserts that udp4_responses_sent_total and udp6_responses_sent_total remain 0 after processing a port-0 request (early return, no send_response call). - processor_emits_discard_event_when_client_port_is_0 Asserts that udp_requests_discarded_total == 1, confirming that Event::UdpRequestDiscarded was emitted to the stats bus. Also extract two shared test helpers to make each test body a clear three-section Arrange / Act / Assert: - setup_processor_with_stats_listener() — all environment boilerplate - wait_for_discarded_count() — bounded async poll The removed assertion (udp4_requests_received_total == 0) tested the launcher's responsibility, not the processor's, so it was dropped.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
project-words.txt:266
project-words.txtmust remain case-insensitively alphabetically sorted, butnmapandnpingare currently placed betweennologinandnonblocking. In strict lexicographic order,nmapshould come before theno*words, andnpingshould come after allno*words (e.g., afternotnull).
nocapture
nologin
nmap
nping
nonblocking
Thread #13 (PRRT_kwDOGp2yqc6S0_RT) suggested discarding port-0 requests in the launcher loop before spawning/pushing into active_requests. Applied in b4fb60c, replied and resolved.
The port-0 discard added in this PR increased the complexity of Launcher::run_udp_server_main, which now mixes setup, receive/error handling, a per-request policy pipeline, and four copies of the stats-event emission boilerplate. It also recreates ServiceBinding on every loop iteration (a per-request allocation with no benefit). The draft spec plans a follow-up refactor constrained to static dispatch and zero new per-request allocations, with benchmark verification, to be implemented in a separate PR.
Copilot review threads on PR torrust#2017 pointed out that the processor port-0 tests used an empty payload, so they could not detect a regression where the discard guard moved after parsing/handler work. - Build a valid UDP connect request payload in the test helper. - Assert udp4_connect_requests_accepted_total() == 0 so the tests fail if the connect handler ever runs for a port-0 request.
Member
Author
|
ACK 99858e2 |
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.
Summary
Fixes #1450 by discarding UDP tracker requests received from source port
0before they can enter the request-processing concurrency buffer. Linux rejects responses sent to<client-ip>:0withEINVAL; previously the tracker processed such requests fully and then logged a noisyWARNaftersend_tofailed.Changes
ActiveRequests.UdpRequestDiscardedand a trace-level diagnostic, then continues the receive loop.Processor::process_requestguard as defense-in-depth for direct callers.udp_requests_discardedmetric and expose it through the REST statistics endpoint.0:BoundSocket:newtobind;socket.service_binding()rather than duplicating a fragile constructor/expectinProcessor.Verification
TORRUST_GIT_HOOKS_LOG_DIR=.tmp ./contrib/dev-tools/git/hooks/pre-commit.shcargo test --package torrust-tracker-udp-server0:send_tologsInvalid argument (os error 22).WARN; REST stats reportudp_requests_discarded: 1; no UDP response is sent.Follow-up
The request-loop cleanup is deliberately deferred to the draft spec at
docs/issues/drafts/simplify-udp-server-main-loop.md. This PR preserves the current hot-path design while ensuring port-0 traffic is rejected before task creation and request-buffer admission.Review handling
All Copilot review threads raised during this PR have been addressed and resolved; the audit log is in
docs/pr-reviews/pr-2017-copilot-suggestions.md.