Skip to content

Fix/proxyfor timeout regression #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
varunagarwal-pro wants to merge 3 commits into from
Closed

Fix/proxyfor timeout regression #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fb0ef4c
fix(ipn/proxies): restore getproxytimeout for proxyFor lock-read guard
varunagarwal-pro Sep 9, 2026
a251584
Revert "ipn/proxies: ping if last good rx/tx is more than 2m"
varunagarwal-pro Sep 9, 2026
a419e04
Revert "netstack/icmp: prevent icmp floods"
varunagarwal-pro Sep 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions intra/core/expiringmap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,6 @@ func (m *ExpMap[P, Q]) Get(key P) uint32 {
return v.hits
}

func (m *ExpMap[P, Q]) SetMin(key P) uint32 {
if done(m.ctx) || m.minlife <= 0 {
return 0
}
return m.Set(key, m.minlife)
}

// Set sets the expiry for the given key and returns the number of hits.
// expiry is clamped to minlife. If the key was expired, its hit window
// is reset to 0 before returning. Value is set to Q's zero value.
Expand Down
24 changes: 0 additions & 24 deletions intra/icmp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,23 +24,13 @@ import (

type icmpHandler struct {
*baseHandler
staller *core.ExpMap[netip.AddrPort, string] // src(addr:port) -> stallSecs
}

var _ netstack.GICMPHandler = (*icmpHandler)(nil)

// pings allowed per source within icmpFloodTrackTTL, after
// which each ping is stalled for up to icmptarpitMaxSecs.
const (
icmpFloodHits = 10 // pings allowed per source per window
icmptarpitMaxSecs = 5 // max stall per ping
icmpFloodTrackTTL = 10 * time.Second
)

func NewICMPHandler(pctx context.Context, resolver dnsx.Resolver, prox ipn.ProxyProvider, listener Listener) netstack.GICMPHandler {
h := &icmpHandler{
baseHandler: newBaseHandler(pctx, "icmp", resolver, prox, listener),
staller: core.NewExpiringMapLifetime[netip.AddrPort, string](pctx, "icmp.staller", icmpFloodTrackTTL),
}

core.Gx("icmp.ps", h.processSummaries)
Expand All @@ -49,14 +39,6 @@ func NewICMPHandler(pctx context.Context, resolver dnsx.Resolver, prox ipn.Proxy
return h
}

func (h *icmpHandler) maybeStall(src netip.AddrPort) (secs uint32) {
if n := h.staller.Get(src); n > icmpFloodHits {
secs = icmptarpitMaxSecs
}
h.staller.SetMin(src) // track for icmpFloodTrackTTL
return
}

// Ping implements netstack.GICMPHandler. Takes ownership of msg.
// Nb: to send icmp pings, root access is required; and so,
// send "unprivileged" icmp pings via udp reqs; which do
Expand Down Expand Up @@ -112,12 +94,6 @@ func (h *icmpHandler) Ping(msg []byte, source, target netip.AddrPort) (echoed bo
return false // denied
}

// delay flooders; this fn is async, so stalling doesn't block dispatchers
if secs := h.maybeStall(source); secs > 0 {
log.I("t.icmp: flood: stalled %s => %s for %ds", source, target, secs)
time.Sleep(time.Duration(secs) * time.Second)
}

if px, err = h.prox.ProxyTo(cid, dst, "icmp", uid, pids); err != nil || px == nil {
err = log.EE("t.icmp: egress: no proxy(%s); err %v", pids, err)
return false // denied
Expand Down
21 changes: 20 additions & 1 deletion intra/ipn/proxies.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -929,7 +929,26 @@ func (px *proxifier) proxyFor(id string) (Proxy, error) {
// Ingress (dummy): no fast path, fall through to general lookup
}

timeout := time.Duration(minWaitPeriodSec/2) * time.Second
// Regression fix: this used to be getproxytimeout (5s) and was
// inadvertently shortened to minWaitPeriodSec/2 (1s) in 8677a52c
// ("core/volatile: cr by muse spark" era commit chain). proxyFor is
// called for every proxy id, including non-wellknown, app-registered
// ids (see isWellknown/ProxyFor above) for which there is NO retry/
// wait fallback -- ProxyFor returns immediately with errProxyNotFound
// for those ids, so this is the *only* window a caller gets to find
// a just-registered proxy. The lookup itself is a cheap RLock'd map
// read (see below), but on loaded/low-RAM devices the paired Lock()
// in AddProxy/RemoveProxy can legitimately hold the mutex for longer
// than 1s during proxy setup/teardown, especially for proxies that do
// real I/O in their constructor. Shortening this guard to 1s turns a
// rare, recoverable stall into a hard, unretried lookup failure for
// any non-wellknown proxy id registered right around this window --
// observed in production as a permanently-failing custom local proxy
// route until the next reconnect. Restoring getproxytimeout (5s)
// keeps this a deadlock-recovery guard (its original documented
// purpose, see the ProxyFor doc-comment above) rather than a
// register-race timeout.
timeout := getproxytimeout
// go.dev/play/p/xCug1W3OcMH
p, completed := core.Grx("pxr.ProxyFor: "+id, func(_ context.Context) (Proxy, error) {
px.RLock()
Expand Down
7 changes: 1 addition & 6 deletions intra/ipn/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,9 +766,6 @@ func healthy(p Proxy) error {
age := now - stat.LastOpen

oldEnough := age > ageThreshold.Milliseconds()

lastGoodRx := now - stat.LastGoodRx
lastGoodTx := now - stat.LastGoodTx
lastOK := stat.LastOK
lastOKNeverOK := lastOK <= 0
lastOKBeyondThres := lastOK > 0 && now-lastOK > lastOKThreshold.Milliseconds()
Expand All @@ -778,10 +775,8 @@ func healthy(p Proxy) error {
pid, core.FmtMillis(age), pxstatus(status), lastOKNeverOK, lastOKBeyondThres)
} else if now-lastOK > tzzTimeout.Milliseconds() {
core.Gx("proxy.health.TZZ."+pid, func() { p.Ping() })
} else if lastGoodTx > tzzTimeout.Milliseconds() || lastGoodRx > tzzTimeout.Milliseconds() {
core.Gx("proxy.health.TxRx."+pid, func() { p.Ping() })
} else if status != TOK {
core.Gx("proxy.health.TNOK."+pid, func() { p.Ping() })
core.Gx("proxy.health.TOK."+pid, func() { p.Ping() })

@github-actions github-actions Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
The diagnostic key now contradicts the branch condition. This branch only runs when status != TOK (status is not OK), but the label was changed from proxy.health.TNOK. (not-ok) to proxy.health.TOK.. TOK is a real status constant meaning "ok" (see pxstatus in proxies.go), so the emitted log key now implies success for a not-ok case, which will mislead anyone filtering these health logs. Suggest keeping a name consistent with the condition.

Suggestion:

Suggested change
core.Gx("proxy.health.TOK."+pid, func() { p.Ping() })
core.Gx("proxy.health.TNOK."+pid, func() { p.Ping() })

@github-actions github-actions Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
The guard for this branch is still status != TOK (i.e. the proxy status is not ok), but the log/tracking label was renamed from proxy.health.TNOK. to proxy.health.TOK.. Since TOK is the proxy status meaning "ok" (pxstatus.String() maps TOK -> "ok"), the new label contradicts the condition and will mislead anyone reading health logs/traces. Restore the TNOK label (or use a clearer name).

Suggestion:

Suggested change
core.Gx("proxy.health.TOK."+pid, func() { p.Ping() })
core.Gx("proxy.health.TNOK."+pid, func() { p.Ping() })

}

return nil // ok
Expand Down
14 changes: 0 additions & 14 deletions intra/netstack/icmp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,6 @@ func (f *icmpForwarder) reply4(id stack.TransportEndpointID, pkt *stack.PacketBu
log.D("icmp: v4: %s: type %v passthrough", f.o, hdr.Type())
return // not handled
}
// consult the stack-wide ICMP rate limiter; see: stackopts.go:SetNetstackOpts
// github.com/google/gvisor/blob/738e1d995f/pkg/tcpip/network/ipv4/icmp.go
if !f.s.AllowICMPMessage() {
log.V("icmp: v4: %s: rate limited; dropping echo %s => %s", f.o, src, dst)
return true // handled (silently dropped)
}
ipHdr := header.IPv4(l3hdr)
replyData := stack.PayloadSince(pkt.TransportHeader())
localAddressBroadcast := pkt.NetworkPacketInfo.LocalAddressBroadcast
Expand Down Expand Up @@ -203,14 +197,6 @@ func (f *icmpForwarder) reply6(id stack.TransportEndpointID, pkt *stack.PacketBu
}

l3 := pkt.Network() // l3.Dst == id.LocalAddr and l3.Src == id.RemoteAddr

// consult the stack-wide ICMP rate limiter before; see: stackopts.go:SetNetstackOpts
// github.com/google/gvisor/blob/738e1d995f/pkg/tcpip/network/ipv6/icmp.go
if !f.s.AllowICMPMessage() {
log.V("icmp: v6: %s: rate limited; dropping echo %s => %s", f.o, l3.DestinationAddress(), l3.SourceAddress())
return true // handled (silently dropped)
}

route, err := f.s.FindRoute(pkt.NICID, l3.DestinationAddress(), l3.SourceAddress(), pkt.NetworkProtocolNumber, false)
if err != nil {
log.W("icmp: v6: %s: no route on %v to %s <= %s", f.o, pkt.NICID, l3.DestinationAddress(), l3.SourceAddress())
Expand Down
6 changes: 0 additions & 6 deletions intra/netstack/icmpecho.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,12 +217,6 @@ func (r *icmpResponder) process(h *icmpForwarder, nic tcpip.NICID, pkt *wire.Par
return
}

// consult the stack-wide ICMP rate limiter before; see: stackopts.go:SetNetstackOpts
if h.s != nil && !h.s.AllowICMPMessage() {
logwv(true)("icmp: responder: rate limited; dropping ping %s => %s", src, dst)
return
}

pinged := h.h.Ping(icmpMsg, src, dst)

@github-actions github-actions Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[security · medium]
This removal leaves the ICMP echo path with no throttling at all. AllowICMPMessage() is no longer consulted anywhere in the tree and SetICMPLimit/SetICMPBurst were dropped from SetNetstackOpts, while the separate maybeStall/tar-pit mitigation in intra/icmp.go was also deleted. Because process is dispatched from an unbounded core.Gx("icmp.responder", …) goroutine per request (see handle above) and now unconditionally runs h.h.Ping(...), a flood of ICMP echo requests will spawn unbounded goroutines and proxy/egress work with no rate cap — a resource-exhaustion/DoS vector. If dropping the limiter is intentional, please confirm and document it; otherwise the stack-wide rate-limit consultation should be retained here.


resp, proto, l4proto, tag, err := r.echoReply(pkt, payload, pinged)
Expand Down
14 changes: 0 additions & 14 deletions intra/netstack/stackopts.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,14 @@
package netstack

import (
"golang.org/x/time/rate"
"gvisor.dev/gvisor/pkg/tcpip"
"gvisor.dev/gvisor/pkg/tcpip/network/ipv4"
"gvisor.dev/gvisor/pkg/tcpip/network/ipv6"
"gvisor.dev/gvisor/pkg/tcpip/stack"
"gvisor.dev/gvisor/pkg/tcpip/transport/tcp"
)

const (
// icmpPingLimit caps generated ICMP messages per second:
// Firestack client code must consult Stack.AllowICMPMessage()
// but it auto-applies to gVisor generated ICMP errors (ipv4.go:allowICMPReply)
icmpPingLimit = rate.Limit(10)
// icmpPingBurst caps the initial burst:
// Firestack client code must consult Stack.AllowICMPMessage()
icmpPingBurst = 7
)

func SetNetstackOpts(s *stack.Stack) {
s.SetICMPLimit(icmpPingLimit)
s.SetICMPBurst(icmpPingBurst)

// TODO: other stack otps?
// github.com/xjasonlyu/tun2socks/blob/31468620e/core/option/option.go#L69

Expand Down
Loading