Fix/proxyfor timeout regression - #251
varunagarwal-pro wants to merge 3 commits into
Conversation
proxyFor()'s internal timeout for its RLock'd map-read goroutine (intended purely as a deadlock-recovery guard, per the ProxyFor doc-comment: "if it takes longer than getproxytimeout, it returns an error") was inadvertently changed from getproxytimeout (5s) to minWaitPeriodSec/2 (1s), with no accompanying doc-comment update and no explanation in the commit message. Impact: ProxyFor() only retries/waits for a missing proxy when isWellknown(id) is true (WG/Orbot/pip/internal/global-h1 ids). For any other, app-registered custom proxy id, proxyFor() is the only lookup attempt - there is no fallback wait. On a loaded or low-RAM device, the paired px.Lock() in AddProxy/RemoveProxy can legitimately hold the mutex for longer than 1s while a proxy is being registered/torn down (especially proxies whose constructor performs real I/O). Previously this had up to 5s of slack before proxyFor() gave up; now it has only 1s, turning a previously-recoverable, momentary lock stall into a permanent, unretried "proxy not found" for that connection - observed downstream (celzero/rethink-app-derived fork) as an intermittently failing custom local HTTP proxy route on Android TV / Fire TV Stick hardware, causing affected app connections to fail and the calling app to retry indefinitely. Fix: restore timeout := getproxytimeout, matching the function's existing doc-comment and preserving the deadlock-recovery intent this guard was designed for, without touching the minWaitPeriodSec change (3s->2s) which only affects the separate wellknown-id retry/backoff path and is not implicated in this regression. No behavior change for the intended deadlock-recovery case (an actual hang still errors out, just with the originally-documented 5s grace period instead of 1s).
This reverts commit f8fdaaf. StreamShield investigation into a Zee5 playback hang (video CDN TCP connections dying with "endpoint is closed for send" on the direct/ Exit proxy path) is bisecting this commit as one of two remaining suspects in the celzero/firestack 61894b7..8677a52 range, after ruling out and fixing the proxyFor lock-read timeout regression (315d233, see StreamShield PR celzero#248). This commit added a periodic core.Gx("proxy.health.TxRx."+pid, ...) health-ping trigger fired whenever a proxy's last-good rx or tx is older than tzzTimeout (2m). Reverting to test whether this newly introduced background Ping() call against long-idle-but-otherwise- healthy proxies (including the Exit passthrough proxy carrying real CDN traffic) is contributing to, or masking symptoms of, the observed TCP write failures. Not confirmed as root cause; reverted as part of a bisection A/B test.
This reverts commit b33dbb8. StreamShield investigation into a Zee5 playback hang (video CDN TCP connections dying with "endpoint is closed for send" on the direct/ Exit proxy path) is bisecting this commit as the second of two remaining suspects in the celzero/firestack 61894b7..8677a52 range, after ruling out and fixing the proxyFor lock-read timeout regression (315d233, see StreamShield PR celzero#248). This commit added a stack-wide ICMP rate limiter (10/s, burst 7) plus a per-source ICMP echo tarpit/stall (sleeps up to 5s per ping once a source exceeds 10 pings in a 10s window) in the netstack ICMP path. Reverting to test whether this new rate limiting/stalling logic is interacting badly with concurrent TCP dialing/health-probing on the same netstack instance, or otherwise contributing to the observed TCP write failures on unrelated connections. Not confirmed as root cause; reverted as part of a bisection A/B test.
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Approved |
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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() }) |
There was a problem hiding this comment.
[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:
| core.Gx("proxy.health.TOK."+pid, func() { p.Ping() }) | |
| core.Gx("proxy.health.TNOK."+pid, func() { p.Ping() }) |
| return | ||
| } | ||
|
|
||
| pinged := h.h.Ping(icmpMsg, src, dst) |
There was a problem hiding this comment.
[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.
| 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() }) |
There was a problem hiding this comment.
[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:
| core.Gx("proxy.health.TOK."+pid, func() { p.Ping() }) | |
| core.Gx("proxy.health.TNOK."+pid, func() { p.Ping() }) |
|
Closing this PR: it duplicated #248's fix and additionally carried two speculative, unconfirmed bisection-experiment revert commits (their own messages say 'Not confirmed as root cause'). Those experiments are being dropped entirely, not carried forward, since the actual confirmed root cause has since been identified and fixed in #250. Consolidating everything into #250, which contains only the two verified, confirmed fixes: the proxyFor lock-read timeout restore, and the rwext read/write deadline fix. |
1 participant
No description provided.