From a5d33cb3f98399ccd0398b4eb747b39853887433 Mon Sep 17 00:00:00 2001 From: Lin Date: Sun, 26 Jul 2026 18:15:18 +0800 Subject: [PATCH 1/5] naive: erase zombie target on first-datagram admission drop NaiveConnectUdpDatagramBackend::Send() inserted a new TargetEntry into targets_ before the association-level admission checks. When those checks rejected the datagram that created the target, Send() returned OK and left a permanent kConnecting entry with an empty outbound queue that no timer ever reaps, leaking one of the 32 target slots for the association's lifetime. A client that bursts many distinct destinations under queue pressure could exhaust the target table and stall further targets. Track whether the target was created for this datagram and erase it on the association-level queue-capacity drop path. The cooldown/retiring branches cannot fire for a freshly created kConnecting target, so only this path needs the cleanup. Not built locally (Chromium tree); change is code-review-only. Co-Authored-By: Claude Fable 5 --- .../naive/naive_connect_udp_datagram_backend.cc | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/net/tools/naive/naive_connect_udp_datagram_backend.cc b/src/net/tools/naive/naive_connect_udp_datagram_backend.cc index 181ba9ec22..b0ad09c13b 100644 --- a/src/net/tools/naive/naive_connect_udp_datagram_backend.cc +++ b/src/net/tools/naive/naive_connect_udp_datagram_backend.cc @@ -90,7 +90,8 @@ int NaiveConnectUdpDatagramBackend::Send( } const Socks5UdpTargetKey key(datagram.destination); auto it = targets_.find(key); - if (it == targets_.end()) { + const bool created_target = it == targets_.end(); + if (created_target) { if (targets_.size() >= Socks5UdpBackendLimits::kMaxTargets) { ++stats_.capacity_drops; RecordCounterEvent("target_capacity_drop", stats_.capacity_drops); @@ -129,6 +130,15 @@ int NaiveConnectUdpDatagramBackend::Send( queued_payload_bytes_) { ++stats_.capacity_drops; RecordCounterEvent("queue_capacity_drop", stats_.capacity_drops); + // A target created for this datagram is still kConnecting with an empty + // queue, so the cooldown/retiring checks above cannot fire for it and only + // this association-level admission can reject its first datagram. Erase the + // freshly created entry instead of leaving a zombie kConnecting target that + // no timer reaps, which would otherwise leak a target slot for the whole + // association lifetime. + if (created_target) { + targets_.erase(it); + } return OK; } From d6f95e9f610d7b557a42efbb0c9635b32e27d8a9 Mon Sep 17 00:00:00 2001 From: Lin Date: Sun, 26 Jul 2026 18:15:56 +0800 Subject: [PATCH 2/5] naive: tolerate transient UDP relay errors per datagram HandleRelayRead() and OnRelayWriteComplete() called Finish() on any negative result, tearing down the control TCP connection and every active tunnel for the association. A single per-packet condition would therefore kill traffic to all unrelated destinations sharing the association. The most common case is Windows surfacing a prior ICMP "port unreachable" as ERR_CONNECTION_RESET on the next recv/send; ERR_MSG_TOO_BIG and ERR_ADDRESS_UNREACHABLE are similarly per-datagram. Classify these as recoverable: drop the affected datagram and continue pumping instead of finishing the association. Non-recoverable errors still finish. Not built locally (Chromium tree); change is code-review-only. Co-Authored-By: Claude Fable 5 --- src/net/tools/naive/socks5_udp_association.cc | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/net/tools/naive/socks5_udp_association.cc b/src/net/tools/naive/socks5_udp_association.cc index 552aeb7a10..290af1678b 100644 --- a/src/net/tools/naive/socks5_udp_association.cc +++ b/src/net/tools/naive/socks5_udp_association.cc @@ -35,6 +35,24 @@ uint16_t RequestedClientPort( return control_socket->request_endpoint().port(); } +// A local UDP relay error that concerns a single datagram rather than the +// association as a whole. On some platforms a prior ICMP "port unreachable" +// surfaces as ERR_CONNECTION_RESET on the next recv/send, and an oversize +// payload surfaces as ERR_MSG_TOO_BIG. Tearing down the control TCP connection +// and every active tunnel for such a per-packet condition is a denial of +// service against unrelated destinations sharing the association, so these are +// dropped and the pumps continue. +bool IsRecoverableRelayError(int result) { + switch (result) { + case ERR_CONNECTION_RESET: + case ERR_MSG_TOO_BIG: + case ERR_ADDRESS_UNREACHABLE: + return true; + default: + return false; + } +} + } // namespace Socks5UdpAssociation::QueuedResponse::QueuedResponse( @@ -189,6 +207,11 @@ void Socks5UdpAssociation::OnRelayReadComplete(int result) { bool Socks5UdpAssociation::HandleRelayRead(int result) { if (result < 0) { + if (IsRecoverableRelayError(result)) { + // Drop the failed read but keep the association and its tunnels alive; + // the caller continues pumping subsequent reads. + return true; + } Finish(result); return false; } @@ -314,6 +337,15 @@ void Socks5UdpAssociation::PumpRelayWrites() { void Socks5UdpAssociation::OnRelayWriteComplete(int result) { relay_write_pending_ = false; if (result < 0) { + if (IsRecoverableRelayError(result)) { + // Drop the datagram that failed to send and continue with the queue + // rather than terminating the whole association. + CHECK(!response_queue_.empty()); + response_queue_.pop_front(); + last_activity_ = base::TimeTicks::Now(); + PumpRelayWrites(); + return; + } Finish(result); return; } From cc2208ec8750c042a9939b7a66141febd7d59aa4 Mon Sep 17 00:00:00 2001 From: Lin Date: Sun, 26 Jul 2026 18:16:20 +0800 Subject: [PATCH 3/5] naive: rotate UDP associations at tunnel_timeout_ CleanUpIdleConnections() rotated TCP tunnels on both idle_timeout_ and a tunnel_timeout_ max-lifetime bound, but UDP associations were only aged by idle_timeout_. A long-lived association carrying steady traffic never rotated, unlike every other tunnel, weakening the intended lifetime cap and connection rotation behavior. Expose Socks5UdpAssociation::creation_time() and apply the same "idle > idle_timeout_ || age > tunnel_timeout_" test used for TCP tunnels. Not built locally (Chromium tree); change is code-review-only. Co-Authored-By: Claude Fable 5 --- src/net/tools/naive/naive_proxy.cc | 4 +++- src/net/tools/naive/socks5_udp_association.h | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/net/tools/naive/naive_proxy.cc b/src/net/tools/naive/naive_proxy.cc index 4481bfa4b7..177c41c102 100644 --- a/src/net/tools/naive/naive_proxy.cc +++ b/src/net/tools/naive/naive_proxy.cc @@ -605,7 +605,9 @@ void NaiveProxy::CleanUpIdleConnections() { ClosePendingSocks(id, ERR_TIMED_OUT); } for (const auto& [id, association] : udp_association_by_id_) { - if (now - association->last_activity() > idle_timeout_) { + base::TimeDelta idle = now - association->last_activity(); + base::TimeDelta age = now - association->creation_time(); + if (idle > idle_timeout_ || age > tunnel_timeout_) { idle_udp_associations.push_back(id); } } diff --git a/src/net/tools/naive/socks5_udp_association.h b/src/net/tools/naive/socks5_udp_association.h index 7e9932c5d5..aebf634559 100644 --- a/src/net/tools/naive/socks5_udp_association.h +++ b/src/net/tools/naive/socks5_udp_association.h @@ -42,6 +42,7 @@ class Socks5UdpAssociation { unsigned int id() const { return id_; } base::TimeTicks last_activity() const { return last_activity_; } + base::TimeTicks creation_time() const { return created_at_; } // Returns ERR_IO_PENDING while the association is active. The callback is // invoked once when the TCP control connection or UDP relay terminates. @@ -101,6 +102,7 @@ class Socks5UdpAssociation { CompletionOnceCallback completion_callback_; bool finished_ = false; + const base::TimeTicks created_at_ = base::TimeTicks::Now(); base::TimeTicks last_activity_ = base::TimeTicks::Now(); base::WeakPtrFactory weak_ptr_factory_{this}; From 2ec711012e97b98f5291615597e055dfae88be2d Mon Sep 17 00:00:00 2001 From: Lin Date: Sun, 26 Jul 2026 18:16:48 +0800 Subject: [PATCH 4/5] quic: unregister H3 datagram visitor symmetrically on stream close Close() unregisters the Http3DatagramVisitor when datagram_visitor_registered_ is set, but OnStreamClosed() cleared the flag without calling UnregisterHttp3DatagramVisitor(). The stream is already closed on this path so the unregister is effectively a no-op today, but the asymmetry is fragile: a future change that lets the stream handle outlive the close notification would leave a dangling visitor registration. Make OnStreamClosed() perform the same guarded unregister as Close(). Not built locally (Chromium tree); change is code-review-only. Co-Authored-By: Claude Fable 5 --- src/net/quic/quic_proxy_datagram_client_socket.cc | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/net/quic/quic_proxy_datagram_client_socket.cc b/src/net/quic/quic_proxy_datagram_client_socket.cc index e9fa07f684..a5e3b71a0b 100644 --- a/src/net/quic/quic_proxy_datagram_client_socket.cc +++ b/src/net/quic/quic_proxy_datagram_client_socket.cc @@ -401,7 +401,14 @@ void QuicProxyDatagramClientSocket::OnStreamClosed(int result) { // They may otherwise re-enter OnIOComplete after this close notification. weak_factory_.InvalidateWeakPtrs(); next_state_ = STATE_DISCONNECTED; - datagram_visitor_registered_ = false; + // Unregister symmetrically with Close(). The stream is already closed here so + // the call is effectively a no-op, but leaving the visitor registered while + // clearing the flag desynchronizes the two paths and is fragile if the + // stream handle outlives this notification in a future refactor. + if (datagram_visitor_registered_) { + stream_handle_->UnregisterHttp3DatagramVisitor(); + datagram_visitor_registered_ = false; + } connect_request_sent_ = false; awaiting_connect_response_ = false; read_buf_ = nullptr; From 474a1e4b0aeb9c64e6d0083eaddd205c887bf608 Mon Sep 17 00:00:00 2001 From: Lin Date: Fri, 21 Aug 2026 11:54:10 +0800 Subject: [PATCH 5/5] naive: test and tighten native UDP hardening Reject new CONNECT-UDP targets before factory/emplace when the association queue is full, so admission pressure cannot churn throwaway tunnels. Recoverable relay SendTo failures drop the datagram without refreshing last_activity_. Add ShouldExpire() for the idle-or-age rotation already used by CleanUpIdleConnections. Cover the four hardening behaviors in naive_connect_udp_backend_test and naive_socks5_udp_association_test. Built and ran the M2/M3 owner scripts on the existing macOS arm64 Release tree: M2_SOCKS5_UDP_INGRESS_OK M3_NATIVE_UDP_CLIENT_OK --- .../quic/quic_proxy_datagram_client_socket.cc | 5 +- .../naive_connect_udp_backend_test_bin.cc | 16 ++ .../naive_connect_udp_datagram_backend.cc | 31 ++-- src/net/tools/naive/naive_proxy.cc | 4 +- .../naive_socks5_udp_association_test_bin.cc | 171 +++++++++++++++++- src/net/tools/naive/socks5_udp_association.cc | 21 +-- src/net/tools/naive/socks5_udp_association.h | 3 + 7 files changed, 206 insertions(+), 45 deletions(-) diff --git a/src/net/quic/quic_proxy_datagram_client_socket.cc b/src/net/quic/quic_proxy_datagram_client_socket.cc index a5e3b71a0b..da2f131344 100644 --- a/src/net/quic/quic_proxy_datagram_client_socket.cc +++ b/src/net/quic/quic_proxy_datagram_client_socket.cc @@ -401,10 +401,7 @@ void QuicProxyDatagramClientSocket::OnStreamClosed(int result) { // They may otherwise re-enter OnIOComplete after this close notification. weak_factory_.InvalidateWeakPtrs(); next_state_ = STATE_DISCONNECTED; - // Unregister symmetrically with Close(). The stream is already closed here so - // the call is effectively a no-op, but leaving the visitor registered while - // clearing the flag desynchronizes the two paths and is fragile if the - // stream handle outlives this notification in a future refactor. + // Match Close(): do not clear the flag while leaving the visitor registered. if (datagram_visitor_registered_) { stream_handle_->UnregisterHttp3DatagramVisitor(); datagram_visitor_registered_ = false; diff --git a/src/net/tools/naive/naive_connect_udp_backend_test_bin.cc b/src/net/tools/naive/naive_connect_udp_backend_test_bin.cc index a50283bdcd..c59288a96e 100644 --- a/src/net/tools/naive/naive_connect_udp_backend_test_bin.cc +++ b/src/net/tools/naive/naive_connect_udp_backend_test_bin.cc @@ -852,6 +852,22 @@ void TestTargetAndQueueLimits() { 128 && association_cap_backend.stats_for_testing().capacity_drops == 1, "association packet queue admits 128 and drops the 129th"); + const size_t targets_before_new_drop = + association_cap_backend.target_count_for_testing(); + const size_t factory_calls_before_new_drop = association_states.size(); + association_cap_backend.Send( + DatagramFor(net::Socks5UdpEndpoint{ + .type = net::Socks5UdpAddressType::kDomain, + .host = "new-target-at-capacity.test", + .port = 443, + }, + {0x03}), + net::CompletionOnceCallback()); + Expect(association_cap_backend.target_count_for_testing() == + targets_before_new_drop && + association_states.size() == factory_calls_before_new_drop && + association_cap_backend.stats_for_testing().capacity_drops == 2, + "first datagram admission drop does not construct or leak a target"); auto byte_state = std::make_shared(); net::NaiveConnectUdpDatagramBackend byte_cap_backend( diff --git a/src/net/tools/naive/naive_connect_udp_datagram_backend.cc b/src/net/tools/naive/naive_connect_udp_datagram_backend.cc index b0ad09c13b..20210fce4a 100644 --- a/src/net/tools/naive/naive_connect_udp_datagram_backend.cc +++ b/src/net/tools/naive/naive_connect_udp_datagram_backend.cc @@ -90,13 +90,25 @@ int NaiveConnectUdpDatagramBackend::Send( } const Socks5UdpTargetKey key(datagram.destination); auto it = targets_.find(key); - const bool created_target = it == targets_.end(); - if (created_target) { + const bool association_queue_full = + queued_datagram_count_ >= + Socks5UdpBackendLimits::kMaxQueuedDatagramsPerAssociation || + datagram.payload.size() > + Socks5UdpBackendLimits::kMaxQueuedPayloadBytesPerAssociation - + queued_payload_bytes_; + if (it == targets_.end()) { if (targets_.size() >= Socks5UdpBackendLimits::kMaxTargets) { ++stats_.capacity_drops; RecordCounterEvent("target_capacity_drop", stats_.capacity_drops); return OK; } + // Reject before factory/emplace so a full association queue cannot churn + // unique destinations into throwaway tunnels. + if (association_queue_full) { + ++stats_.capacity_drops; + RecordCounterEvent("queue_capacity_drop", stats_.capacity_drops); + return OK; + } std::unique_ptr tunnel = tunnel_factory_.Run(context_, datagram.destination); if (!tunnel) { @@ -123,22 +135,9 @@ int NaiveConnectUdpDatagramBackend::Send( } if (entry->outbound_queue.size() >= Socks5UdpBackendLimits::kMaxQueuedDatagramsPerTarget || - queued_datagram_count_ >= - Socks5UdpBackendLimits::kMaxQueuedDatagramsPerAssociation || - datagram.payload.size() > - Socks5UdpBackendLimits::kMaxQueuedPayloadBytesPerAssociation - - queued_payload_bytes_) { + association_queue_full) { ++stats_.capacity_drops; RecordCounterEvent("queue_capacity_drop", stats_.capacity_drops); - // A target created for this datagram is still kConnecting with an empty - // queue, so the cooldown/retiring checks above cannot fire for it and only - // this association-level admission can reject its first datagram. Erase the - // freshly created entry instead of leaving a zombie kConnecting target that - // no timer reaps, which would otherwise leak a target slot for the whole - // association lifetime. - if (created_target) { - targets_.erase(it); - } return OK; } diff --git a/src/net/tools/naive/naive_proxy.cc b/src/net/tools/naive/naive_proxy.cc index 177c41c102..600adeaddd 100644 --- a/src/net/tools/naive/naive_proxy.cc +++ b/src/net/tools/naive/naive_proxy.cc @@ -605,9 +605,7 @@ void NaiveProxy::CleanUpIdleConnections() { ClosePendingSocks(id, ERR_TIMED_OUT); } for (const auto& [id, association] : udp_association_by_id_) { - base::TimeDelta idle = now - association->last_activity(); - base::TimeDelta age = now - association->creation_time(); - if (idle > idle_timeout_ || age > tunnel_timeout_) { + if (association->ShouldExpire(now, idle_timeout_, tunnel_timeout_)) { idle_udp_associations.push_back(id); } } diff --git a/src/net/tools/naive/naive_socks5_udp_association_test_bin.cc b/src/net/tools/naive/naive_socks5_udp_association_test_bin.cc index ff36a3067e..c17c02d34e 100644 --- a/src/net/tools/naive/naive_socks5_udp_association_test_bin.cc +++ b/src/net/tools/naive/naive_socks5_udp_association_test_bin.cc @@ -20,6 +20,7 @@ #include "base/run_loop.h" #include "base/task/single_thread_task_executor.h" #include "base/task/single_thread_task_runner.h" +#include "base/time/time.h" #include "net/base/io_buffer.h" #include "net/base/ip_address.h" #include "net/base/ip_endpoint.h" @@ -224,6 +225,9 @@ struct RelayState { bool recv_pending = false; bool send_pending = false; bool closed = false; + scoped_refptr pending_recv_buffer; + net::IPEndPoint* pending_recv_address = nullptr; + net::CompletionOnceCallback pending_recv_callback; scoped_refptr pending_send_buffer; net::CompletionOnceCallback pending_send_callback; }; @@ -236,6 +240,12 @@ void QueueRelayData(const std::shared_ptr& state, .data = std::move(data), .source = source, .result = net::OK}); } +void QueueRelayResult(const std::shared_ptr& state, int result) { + CHECK_LT(result, 0); + state->reads.push_back( + RelayReadEvent{.data = {}, .source = {}, .result = result}); +} + class ScriptedDatagramSocket final : public net::DatagramServerSocket { public: ScriptedDatagramSocket(std::shared_ptr state, @@ -258,9 +268,9 @@ class ScriptedDatagramSocket final : public net::DatagramServerSocket { CHECK(!state_->recv_pending); if (state_->reads.empty()) { state_->recv_pending = true; - pending_recv_buffer_ = base::WrapRefCounted(buffer); - pending_recv_address_ = address; - pending_recv_callback_ = std::move(callback); + state_->pending_recv_buffer = base::WrapRefCounted(buffer); + state_->pending_recv_address = address; + state_->pending_recv_callback = std::move(callback); return net::ERR_IO_PENDING; } @@ -308,11 +318,11 @@ class ScriptedDatagramSocket final : public net::DatagramServerSocket { } state_->closed = true; ++state_->close_calls; - if (pending_recv_callback_) { + if (state_->pending_recv_callback) { ++state_->pending_recv_cancellations; - pending_recv_callback_.Reset(); - pending_recv_buffer_.reset(); - pending_recv_address_ = nullptr; + state_->pending_recv_callback.Reset(); + state_->pending_recv_buffer.reset(); + state_->pending_recv_address = nullptr; state_->recv_pending = false; } if (state_->pending_send_callback) { @@ -367,12 +377,35 @@ class ScriptedDatagramSocket final : public net::DatagramServerSocket { private: const std::shared_ptr state_; const net::IPEndPoint local_endpoint_; - scoped_refptr pending_recv_buffer_; - net::IPEndPoint* pending_recv_address_ = nullptr; - net::CompletionOnceCallback pending_recv_callback_; net::NetLogWithSource net_log_; }; +void CompleteRelayRead(const std::shared_ptr& state, + std::vector data, + const net::IPEndPoint& source) { + CHECK(state->recv_pending); + CHECK(state->pending_recv_callback); + CHECK_LE(data.size(), + static_cast(state->pending_recv_buffer->size())); + state->pending_recv_buffer->first(data.size()).copy_from(data); + *state->pending_recv_address = source; + state->recv_pending = false; + state->pending_recv_buffer.reset(); + state->pending_recv_address = nullptr; + std::move(state->pending_recv_callback).Run(static_cast(data.size())); +} + +void CompleteRelayReadError(const std::shared_ptr& state, + int result) { + CHECK_LT(result, 0); + CHECK(state->recv_pending); + CHECK(state->pending_recv_callback); + state->recv_pending = false; + state->pending_recv_buffer.reset(); + state->pending_recv_address = nullptr; + std::move(state->pending_recv_callback).Run(result); +} + void CompleteRelaySend(const std::shared_ptr& state, int result) { CHECK(state->send_pending); CHECK(state->pending_send_callback); @@ -643,6 +676,93 @@ void TestSynchronousBackendEchoAndSendErrorStopsReads() { "synchronous SendTo error never reads closed relay"); } +void TestRecoverableRelayReadErrorsContinue() { + AssociationHarness harness; + BuildAssociation(harness); + for (int result : {net::ERR_CONNECTION_RESET, net::ERR_MSG_TOO_BIG, + net::ERR_ADDRESS_UNREACHABLE}) { + QueueRelayResult(harness.relay, result); + } + QueueRelayData(harness.relay, ValidPacket(), harness.control_peer); + + Expect(StartAssociation(harness) == net::ERR_IO_PENDING, + "recoverable synchronous read association starts"); + RunUntilIdle(); + + Expect(harness.completion.calls == 0, + "recoverable synchronous read errors keep association active"); + Expect(harness.backend->send_calls == 1, + "valid datagram after synchronous read errors reaches backend"); + Expect(harness.relay->recv_pending, + "relay read rearms after synchronous recoverable errors"); + + if (harness.relay->recv_pending) { + CompleteRelayReadError(harness.relay, net::ERR_CONNECTION_RESET); + Expect(harness.completion.calls == 0 && harness.relay->recv_pending, + "asynchronous recoverable read error rearms receive"); + } + if (harness.relay->recv_pending) { + CompleteRelayRead(harness.relay, ValidPacket(), harness.control_peer); + Expect(harness.backend->send_calls == 2 && harness.relay->recv_pending, + "valid datagram after asynchronous read error reaches backend"); + } +} + +void TestRecoverableRelayWriteErrorsContinue() { + AssociationHarness synchronous; + BuildAssociation(synchronous); + synchronous.backend->mode = BackendMode::kEchoSynchronously; + for (int result : {net::ERR_CONNECTION_RESET, net::ERR_MSG_TOO_BIG, + net::ERR_ADDRESS_UNREACHABLE}) { + synchronous.relay->send_results.push_back(result); + QueueRelayData(synchronous.relay, ValidPacket(), synchronous.control_peer); + } + QueueRelayData(synchronous.relay, ValidPacket(), synchronous.control_peer); + + Expect(StartAssociation(synchronous) == net::ERR_IO_PENDING, + "recoverable synchronous write association starts"); + RunUntilIdle(); + Expect(synchronous.completion.calls == 0, + "recoverable synchronous write errors keep association active"); + Expect(synchronous.relay->send_calls == 4 && + synchronous.relay->recv_pending, + "relay sends the datagram after synchronous write errors"); + + AssociationHarness asynchronous; + BuildAssociation(asynchronous); + asynchronous.backend->mode = BackendMode::kEchoSynchronously; + asynchronous.relay->send_results.push_back(net::ERR_IO_PENDING); + asynchronous.relay->send_results.push_back(net::ERR_IO_PENDING); + QueueRelayData(asynchronous.relay, ValidPacket(), asynchronous.control_peer); + QueueRelayData(asynchronous.relay, ValidPacket(), asynchronous.control_peer); + Expect(StartAssociation(asynchronous) == net::ERR_IO_PENDING, + "recoverable asynchronous write association starts"); + RunUntilIdle(); + Expect(asynchronous.relay->send_pending && + asynchronous.relay->send_calls == 1, + "first asynchronous relay write is pending"); + const base::TimeTicks activity_before = + asynchronous.association->last_activity(); + CompleteRelaySend(asynchronous.relay, net::ERR_CONNECTION_RESET); + Expect(asynchronous.completion.calls == 0 && + asynchronous.relay->send_calls == 2 && + asynchronous.relay->send_pending && + asynchronous.association->last_activity() == activity_before, + "asynchronous write error drops one response without idle refresh"); +} + +void TestFatalRelayReadErrorFinishesAssociation() { + AssociationHarness harness; + BuildAssociation(harness); + QueueRelayResult(harness.relay, net::ERR_FAILED); + Expect(StartAssociation(harness) == net::ERR_IO_PENDING, + "fatal read association starts"); + RunUntilIdle(); + Expect(harness.completion.calls == 1 && + harness.completion.result == net::ERR_FAILED, + "non-recoverable relay read still finishes the association"); +} + void TestSynchronousControlDataYieldsThenEof() { AssociationHarness harness; BuildAssociation(harness); @@ -759,6 +879,33 @@ void TestResponseQueuePressure() { "response pressure does not terminate the SOCKS association"); } +void TestExpirationPolicy() { + AssociationHarness harness; + BuildAssociation(harness); + const base::TimeTicks created = harness.association->creation_time(); + const base::TimeTicks last_activity = harness.association->last_activity(); + const base::TimeDelta idle_timeout = base::Seconds(5); + const base::TimeDelta tunnel_timeout = base::Seconds(2); + + Expect(!harness.association->ShouldExpire( + created + tunnel_timeout, base::Seconds(30), tunnel_timeout), + "association remains active at the exact maximum-age boundary"); + Expect(harness.association->ShouldExpire( + created + tunnel_timeout + base::Milliseconds(1), + base::Seconds(30), tunnel_timeout), + "active association expires after its maximum lifetime"); + Expect(!harness.association->ShouldExpire( + last_activity + idle_timeout, idle_timeout, base::Hours(1)), + "association remains active at the exact idle boundary"); + Expect(harness.association->ShouldExpire( + last_activity + idle_timeout + base::Milliseconds(1), idle_timeout, + base::Hours(1)), + "idle association expires after the idle timeout"); + Expect(!harness.association->ShouldExpire( + created + base::Seconds(1), idle_timeout, tunnel_timeout), + "fresh association does not expire"); +} + } // namespace int main() { @@ -768,10 +915,14 @@ int main() { TestFirstRecvFromCompletesSynchronously(); TestSynchronousInvalidBurstYieldsAndRecovers(); TestSynchronousBackendEchoAndSendErrorStopsReads(); + TestRecoverableRelayReadErrorsContinue(); + TestRecoverableRelayWriteErrorsContinue(); + TestFatalRelayReadErrorFinishesAssociation(); TestSynchronousControlDataYieldsThenEof(); TestPendingReadDestructionCancelsCallbacks(); TestPendingBackendSendDestructionCancelsCallback(); TestResponseQueuePressure(); + TestExpirationPolicy(); if (failures != 0) { std::cerr << "M2 G4/G5 deterministic association failures=" << failures diff --git a/src/net/tools/naive/socks5_udp_association.cc b/src/net/tools/naive/socks5_udp_association.cc index 290af1678b..33c7247491 100644 --- a/src/net/tools/naive/socks5_udp_association.cc +++ b/src/net/tools/naive/socks5_udp_association.cc @@ -35,13 +35,8 @@ uint16_t RequestedClientPort( return control_socket->request_endpoint().port(); } -// A local UDP relay error that concerns a single datagram rather than the -// association as a whole. On some platforms a prior ICMP "port unreachable" -// surfaces as ERR_CONNECTION_RESET on the next recv/send, and an oversize -// payload surfaces as ERR_MSG_TOO_BIG. Tearing down the control TCP connection -// and every active tunnel for such a per-packet condition is a denial of -// service against unrelated destinations sharing the association, so these are -// dropped and the pumps continue. +// Per-datagram relay failures (ICMP port-unreachable, oversize). Do not +// Finish() the whole association. bool IsRecoverableRelayError(int result) { switch (result) { case ERR_CONNECTION_RESET: @@ -84,6 +79,13 @@ Socks5UdpAssociation::Socks5UdpAssociation( Socks5UdpAssociation::~Socks5UdpAssociation() = default; +bool Socks5UdpAssociation::ShouldExpire(base::TimeTicks now, + base::TimeDelta idle_timeout, + base::TimeDelta tunnel_timeout) const { + return now - last_activity_ > idle_timeout || + now - created_at_ > tunnel_timeout; +} + int Socks5UdpAssociation::Start(CompletionOnceCallback callback) { CHECK(!completion_callback_); completion_callback_ = std::move(callback); @@ -208,8 +210,6 @@ void Socks5UdpAssociation::OnRelayReadComplete(int result) { bool Socks5UdpAssociation::HandleRelayRead(int result) { if (result < 0) { if (IsRecoverableRelayError(result)) { - // Drop the failed read but keep the association and its tunnels alive; - // the caller continues pumping subsequent reads. return true; } Finish(result); @@ -338,11 +338,8 @@ void Socks5UdpAssociation::OnRelayWriteComplete(int result) { relay_write_pending_ = false; if (result < 0) { if (IsRecoverableRelayError(result)) { - // Drop the datagram that failed to send and continue with the queue - // rather than terminating the whole association. CHECK(!response_queue_.empty()); response_queue_.pop_front(); - last_activity_ = base::TimeTicks::Now(); PumpRelayWrites(); return; } diff --git a/src/net/tools/naive/socks5_udp_association.h b/src/net/tools/naive/socks5_udp_association.h index aebf634559..2eac0365e5 100644 --- a/src/net/tools/naive/socks5_udp_association.h +++ b/src/net/tools/naive/socks5_udp_association.h @@ -43,6 +43,9 @@ class Socks5UdpAssociation { unsigned int id() const { return id_; } base::TimeTicks last_activity() const { return last_activity_; } base::TimeTicks creation_time() const { return created_at_; } + bool ShouldExpire(base::TimeTicks now, + base::TimeDelta idle_timeout, + base::TimeDelta tunnel_timeout) const; // Returns ERR_IO_PENDING while the association is active. The callback is // invoked once when the TCP control connection or UDP relay terminates.