Skip to content

Reject out-of-range ports in the TCP benchmark proxy - #16345

Open
tamird wants to merge 1 commit into
google:masterfrom
tamird:tcp-proxy-port-bounds
Open

tamird wants to merge 1 commit into
google:masterfrom
tamird:tcp-proxy-port-bounds

Conversation

@tamird

@tamird tamird commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

A netstack listen or forwarding port of 65537 wraps to port 1. A bad
native forwarding port also makes the proxy retry a connection that
cannot succeed.

Parse the listen port at its 16-bit width and the netstack destination
as a numeric address and port before opening network devices. Check the
native forwarding port at startup so permanent errors cannot enter the
retry loop. Leave host resolution to each connection attempt.

@tamird
tamird force-pushed the tcp-proxy-port-bounds branch from e3ba4b4 to d668992 Compare October 25, 2026 12:40

@manninglucas manninglucas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. The change is small, correct, and merges cleanly with master. It keeps behaviour for valid inputs: an IPv4 target still reaches netstack as a v4-mapped address and is unmapped in AddrNetProtoLocked, as before. Optional notes:

  1. Commit message: the change also fixes the swapped -client/-server help strings (client = netstack dial, server = netstack listen) and moves to pointer receivers. Please add a sentence about the help-string fix so it doesn't look accidental.
  2. Native forward port (optional): with -forward host:65537 and no -client, net.Dial fails with "invalid port" on every attempt, so the pre-bind loop still retries forever. net.SplitHostPort + net.LookupPort("tcp", port) up front would reject it without giving up per-attempt DNS. This would also make the description's second sentence true for both paths.
  3. Please confirm a normal tcp_benchmark.sh --client --server run still works, since this binary has no automated test coverage.

PR description / commit message:
Please also mention that the -client/-server help strings were swapped and are fixed here (client = netstack dial, server = netstack listen). The current description doesn't cover it.

Security: No security impact: the change only touches the out-of-sandbox tcp_proxy benchmark binary and makes its flag parsing stricter (16-bit port parse, netip.ParseAddrPort for the netstack target). It adds no host interactions, no seccomp or Sentry changes, and no paths reachable from sandboxed code or remote peers.


[🤖] This review is part of an experimental auto-review process to help speed up gVisor review velocity. It is a work in progress. Assisted-by: Claude

Comment thread test/benchmarks/tcp/tcp_proxy.go
A netstack listen or forwarding port of 65537 wraps to port 1. A bad
native forwarding port also makes the proxy retry a connection that
cannot succeed.

Parse the listen port at its 16-bit width and the netstack destination
as a numeric address and port before opening network devices. Check the
native forwarding port at startup so permanent errors cannot enter the
retry loop. Leave host resolution to each connection attempt.
@tamird
tamird force-pushed the tcp-proxy-port-bounds branch from d668992 to 641a5c3 Compare October 26, 2026 20:33
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