begin adding lneto - #16
Open
soypat wants to merge 14 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a backend-agnostic netstack API, refactors the gvisor path onto it, introduces an lneto-backed implementation, wires build-tag selection and debug hooks, and expands integration coverage. It also updates module settings and one pool test assertion message. ChangesRepository setup and test message update
Netstack abstraction, gvisor refactor, lneto backend, and tests
Estimated code review effort: 4 (Complex) | ~75 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@go.mod`:
- Line 19: Remove the replace directive for github.com/soypat/lneto from go.mod
(the line referencing ../lneto) since the relative path does not exist in the
repository and breaks builds in CI and for external contributors. Instead,
create or update a go.work file in the repository root to include this local
override, allowing developers to use it locally while keeping go.mod
reproducible and resolvable on its own.
In `@tun/netstack/lneto.go`:
- Around line 399-400: The port conversion in DialContextTCP (line 400),
ListenTCP (line 423), ListenUDP (line 438), DialUDP (line 450), and another
location (line 454) directly casts int ports to uint16 without validation,
allowing invalid ports (negative or out-of-range values) to wrap instead of
being rejected. Create a validatePort function that checks if the port value is
within the valid range (0-65535) and returns an error for invalid values, then
call this validation function before passing the port to netip.AddrPortFrom in
each of these five locations.
- Around line 239-241: The mtu variable lacks an upper bound validation before
being cast to uint16 at line 296, allowing values greater than 65535 to silently
overflow and wrap to unintended values. Add an upper bound check in the mtu
validation block (around line 239-241 where the lower bound check is performed)
to ensure mtu does not exceed 65535, capping it to a reasonable maximum value
(such as 65535) if needed, preventing overflow when the value is later narrowed
to uint16 at line 296.
- Around line 655-657: The UDP case in DialContext does not pass the caller's
context to DialUDPAddrPort on line 656, causing any deadline or cancellation
from dialCtx to be ignored. Modify the UDP case to pass dialCtx to
DialUDPAddrPort (if it accepts a context parameter), or alternatively wrap the
call to respect the context's deadline and cancellation signals. This ensures
that UDP dialing honors the caller's timeout and cancellation requirements just
like other protocol types.
- Around line 645-647: The defer cancel() call on line 647 is inside the
dial-attempt loop, which means the context cancellation is deferred until the
entire function exits rather than being cleaned up after each iteration. This
causes multiple contexts and timers to remain alive unnecessarily. Move the
cancel() call to execute immediately after each dial attempt completes (after
the candidate address is tried) rather than deferring it. This can be done
either by calling cancel() explicitly at the end of each loop iteration, or by
using a closure pattern that wraps the dial attempt and handles cleanup per
iteration. The key is ensuring the context created with context.WithDeadline is
cancelled promptly after each candidate attempt, not waiting until function
exit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 581dff07-c9ed-4836-b596-8a4600298130
📒 Files selected for processing (4)
.gitignorego.modtun/netstack/lneto.gotun/netstack/lneto_test.go
| golang.org/x/time v0.7.0 // indirect | ||
| ) | ||
|
|
||
| replace github.com/soypat/lneto => ../lneto |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Checking go.mod replace directive:"
rg -n '^\s*replace\s+github\.com/soypat/lneto' go.mod
echo
echo "Checking whether target path exists from repo root:"
python - <<'PY'
from pathlib import Path
p = Path("../lneto")
print(f"path: {p}")
print(f"resolved: {p.resolve()}")
print(f"exists: {p.exists()}")
PYRepository: netbirdio/wireguard-go
Length of output: 261
Local replace directive breaks reproducible builds outside your workstation.
Line 19 references ../lneto, which does not exist in this repository. This prevents builds in CI and for external contributors. Move this local override to a developer-local go.work file instead, keeping go.mod resolvable on its own.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 19, Remove the replace directive for github.com/soypat/lneto
from go.mod (the line referencing ../lneto) since the relative path does not
exist in the repository and breaks builds in CI and for external contributors.
Instead, create or update a go.work file in the repository root to include this
local override, allowing developers to use it locally while keeping go.mod
reproducible and resolvable on its own.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tun/netstack/gvisor.go (1)
531-533:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
defer cancel()inside loop accumulates deferred calls.Similar to the issue in
net.go, thecancelfunction is deferred inside a for-loop. Each iteration throughuseUDP := []bool{true, false}creates a deferred cancel that won't execute untilexchangereturns.Proposed fix: cancel at end of each iteration
for _, useUDP := range []bool{true, false} { ctx, cancel := context.WithDeadline(ctx, time.Now().Add(timeout)) - defer cancel() var c net.Conn // ... dial and exchange logic ... c.Close() + cancel() if err != nil {Or use an anonymous function to scope each iteration:
for _, useUDP := range []bool{true, false} { p, h, err := func() (dnsmessage.Parser, dnsmessage.Header, error) { ctx, cancel := context.WithDeadline(ctx, time.Now().Add(timeout)) defer cancel() // ... rest of loop body }() // handle result }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tun/netstack/gvisor.go` around lines 531 - 533, The defer cancel() call inside the for loop that iterates over useUDP values accumulates multiple deferred cancellations that won't execute until the exchange function returns, causing improper context cleanup. Refactor by wrapping the loop body (starting from context.WithDeadline through to the end of the loop iteration) inside an anonymous function that returns the necessary values, ensuring the defer cancel() is scoped to and executes at the end of each iteration rather than accumulating across all iterations.
🧹 Nitpick comments (1)
tun/netstack/net.go (1)
329-343: 💤 Low valueDuplicate error message text.
errServerMisbehaving(line 334) anderrServerTemporarilyMisbehaving(line 337) both have the message"server misbehaving". While they're used in different contexts (temporary vs permanent), identical messages may confuse debugging. Consider making the temporary variant distinct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tun/netstack/net.go` around lines 329 - 343, The error variables errServerMisbehaving and errServerTemporarilyMisbehaving both have the identical error message text "server misbehaving", which makes it difficult to distinguish between the temporary and permanent failure cases during debugging. Update the error message for errServerTemporarilyMisbehaving to include a distinct message that clearly indicates the temporary nature of the error, such as including the word "temporarily" in the message string.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tun/netstack/net.go`:
- Around line 294-298: The `defer cancel()` statement inside the for-loop
accumulates context cancellation functions that won't execute until DialContext
returns, delaying cleanup. Instead of deferring the cancel call, invoke cancel()
directly at the end of each loop iteration after the dial attempt completes.
Remove the `defer cancel()` line and call `cancel()` explicitly after the switch
statement that handles the dial result, ensuring each context created with
`context.WithDeadline` is cancelled before moving to the next iteration.
---
Outside diff comments:
In `@tun/netstack/gvisor.go`:
- Around line 531-533: The defer cancel() call inside the for loop that iterates
over useUDP values accumulates multiple deferred cancellations that won't
execute until the exchange function returns, causing improper context cleanup.
Refactor by wrapping the loop body (starting from context.WithDeadline through
to the end of the loop iteration) inside an anonymous function that returns the
necessary values, ensuring the defer cancel() is scoped to and executes at the
end of each iteration rather than accumulating across all iterations.
---
Nitpick comments:
In `@tun/netstack/net.go`:
- Around line 329-343: The error variables errServerMisbehaving and
errServerTemporarilyMisbehaving both have the identical error message text
"server misbehaving", which makes it difficult to distinguish between the
temporary and permanent failure cases during debugging. Update the error message
for errServerTemporarilyMisbehaving to include a distinct message that clearly
indicates the temporary nature of the error, such as including the word
"temporarily" in the message string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4809ef0d-0ac5-4800-90ce-fbf0638aa968
📒 Files selected for processing (7)
device/pools_test.gotun/netstack/gvisor.gotun/netstack/lneto.gotun/netstack/lneto_test.gotun/netstack/net.gotun/netstack/net_gvisor.gotun/netstack/net_lneto.go
✅ Files skipped from review due to trivial changes (1)
- tun/netstack/net_lneto.go
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tun/netstack/net.go (1)
337-340: 💤 Low valueDuplicate error message text may hinder debugging.
Both
errServerMisbehavinganderrServerTemporarilyMisbehavinghave the identical message "server misbehaving". While they remain distinct error values forerrors.Is()comparisons, the identical text can confuse log analysis and debugging.🔧 Suggested fix
errServerMisbehaving = errors.New("server misbehaving") errInvalidDNSResponse = errors.New("invalid DNS response") errNoAnswerFromDNSServer = errors.New("no answer from DNS server") - errServerTemporarilyMisbehaving = errors.New("server misbehaving") + errServerTemporarilyMisbehaving = errors.New("server temporarily misbehaving")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tun/netstack/net.go` around lines 337 - 340, The error constants errServerMisbehaving and errServerTemporarilyMisbehaving have identical error message text "server misbehaving", which makes debugging and log analysis confusing since they should convey different situations. Change the error message for errServerTemporarilyMisbehaving to a distinct message that reflects its temporary nature (for example, something like "server temporarily misbehaving" or "temporary server error") to differentiate it from the permanent misbehavior error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tun/netstack/lneto_tuncount.go`:
- Around line 12-29: The global counter variables egressPkts, egressBytes,
ingressPkts, and ingressBytes are not protected from concurrent access, which
causes race conditions on multithreaded builds. Replace the plain int variables
with atomic types from the sync/atomic package (such as atomic.Int64 or
atomic.Uint64) and update the countEgress and countIngress functions to use
atomic operations (Add, Load) when incrementing and reading these counters
instead of direct variable assignments and reads.
---
Nitpick comments:
In `@tun/netstack/net.go`:
- Around line 337-340: The error constants errServerMisbehaving and
errServerTemporarilyMisbehaving have identical error message text "server
misbehaving", which makes debugging and log analysis confusing since they should
convey different situations. Change the error message for
errServerTemporarilyMisbehaving to a distinct message that reflects its
temporary nature (for example, something like "server temporarily misbehaving"
or "temporary server error") to differentiate it from the permanent misbehavior
error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63a3f7ba-392e-4bfa-a943-ff1da11baa1a
📒 Files selected for processing (7)
tun/netstack/gvisor.gotun/netstack/lneto.gotun/netstack/lneto_tuncount.gotun/netstack/lneto_tuncount_off.gotun/netstack/net.gotun/netstack/net_debug.gotun/netstack/net_debugoff.go
✅ Files skipped from review due to trivial changes (3)
- tun/netstack/lneto_tuncount_off.go
- tun/netstack/net_debugoff.go
- tun/netstack/net_debug.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tun/netstack/lneto.go
Comment on lines
+12
to
+29
| // not draining its tx. Single counters are fine: js/wasm is single-threaded. | ||
| var ( | ||
| egressPkts, egressBytes int | ||
| ingressPkts, ingressBytes int | ||
| ) | ||
|
|
||
| // countEgress records one non-empty packet pulled from the stack toward WireGuard. | ||
| func countEgress(n int) { | ||
| egressPkts++ | ||
| egressBytes += n | ||
| fmt.Printf("[TUNCOUNT] egress pkts=%d bytes=%d last=%d\n", egressPkts, egressBytes, n) | ||
| } | ||
|
|
||
| // countIngress records one IP packet handed from WireGuard into the stack. | ||
| func countIngress(n int) { | ||
| ingressPkts++ | ||
| ingressBytes += n | ||
| fmt.Printf("[TUNCOUNT] ingress pkts=%d bytes=%d last=%d\n", ingressPkts, ingressBytes, n) |
There was a problem hiding this comment.
Protect TUN counters from concurrent updates.
Line 12 assumes single-threaded execution, but this file is gated by debugheaplog, not js/wasm. On multithreaded builds, countEgress/countIngress can race on shared globals, producing incorrect counts and race warnings. Use atomics (or a mutex) for these counters.
Suggested fix
import "fmt"
+import "sync/atomic"
@@
var (
- egressPkts, egressBytes int
- ingressPkts, ingressBytes int
+ egressPkts, egressBytes atomic.Int64
+ ingressPkts, ingressBytes atomic.Int64
)
@@
func countEgress(n int) {
- egressPkts++
- egressBytes += n
- fmt.Printf("[TUNCOUNT] egress pkts=%d bytes=%d last=%d\n", egressPkts, egressBytes, n)
+ pkts := egressPkts.Add(1)
+ bytes := egressBytes.Add(int64(n))
+ fmt.Printf("[TUNCOUNT] egress pkts=%d bytes=%d last=%d\n", pkts, bytes, n)
}
@@
func countIngress(n int) {
- ingressPkts++
- ingressBytes += n
- fmt.Printf("[TUNCOUNT] ingress pkts=%d bytes=%d last=%d\n", ingressPkts, ingressBytes, n)
+ pkts := ingressPkts.Add(1)
+ bytes := ingressBytes.Add(int64(n))
+ fmt.Printf("[TUNCOUNT] ingress pkts=%d bytes=%d last=%d\n", pkts, bytes, n)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tun/netstack/lneto_tuncount.go` around lines 12 - 29, The global counter
variables egressPkts, egressBytes, ingressPkts, and ingressBytes are not
protected from concurrent access, which causes race conditions on multithreaded
builds. Replace the plain int variables with atomic types from the sync/atomic
package (such as atomic.Int64 or atomic.Uint64) and update the countEgress and
countIngress functions to use atomic operations (Add, Load) when incrementing
and reading these counters instead of direct variable assignments and reads.
Add options for retrying
mlsmaycon
reviewed
Jun 22, 2026
| @@ -1,8 +1,9 @@ | |||
| module golang.zx2c4.com/wireguard | |||
Author
There was a problem hiding this comment.
yes, I'm still waiting on fixes to lneto before fixing a version. we currently use replace directive for it to work
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tun/netstack/lneto.go`:
- Around line 195-196: The DNS-over-TCP lookup path is holding dnsScratch.mu
across dialing, writing, and reading, which can block other lookups past
LookupContextHost deadlines. Update the lookup flow in lneto.go so per-call
state is used instead of the shared dnsScratch lock during network I/O, or
switch to a pool and only guard buffer checkout/return with the mutex. Focus the
fix around dnsScratch and the DNS lookup helpers that perform the
dial/write/read sequence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41825dcb-c33a-4971-b744-5dc7d8f011a1
📒 Files selected for processing (2)
tun/netstack/lneto.gotun/netstack/lneto_test.go
Comment on lines
+195
to
+196
| // dnsScratch holds the reusable buffers for the DNS-over-TCP lookup path. | ||
| dnsScratch dnsScratch |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t hold the shared DNS scratch lock across network I/O.
dnsScratch.mu is held while dialing, writing, and reading, so concurrent lookups block on a non-context-aware mutex and can exceed their LookupContextHost deadline. Use per-call scratch state, or a pool, instead.
Proposed fix
- // dnsScratch holds the reusable buffers for the DNS-over-TCP lookup path.
- dnsScratch dnsScratch
@@
- // The shared dnsScratch buffers are reused across the whole round trip
- // (build → write → read → parse), so hold the lock for the entire function.
- // This serializes DNS lookups, which is fine: A and AAAA already run sequentially.
- s := &n.dnsScratch
- s.mu.Lock()
- defer s.mu.Unlock()
+ var s dnsScratch
@@
-// dnsScratch holds reusable buffers for building and parsing DNS-over-TCP
-// messages, retaining slice backing arrays across lookups. Not safe for
-// concurrent use: callers must hold mu across an entire build→read→parse
-// sequence because buf is reused for both the query and the response.
+// dnsScratch holds buffers for one DNS-over-TCP lookup.
type dnsScratch struct {
- mu sync.Mutex
msg dns.Message
buf []byte // length-prefixed wire buffer
addrs [16]netip.Addr // decode target
}
@@
-// bytes (aliases buf; valid until the next scratch use). Caller holds mu.
+// bytes (aliases buf; valid until the next scratch use).
@@
-// the message bytes (aliases buf; valid until the next scratch use). Caller holds mu.
+// the message bytes (aliases buf; valid until the next scratch use).
@@
-// Caller holds mu. Returns the DNS rcode as error when non-zero.
+// Returns the DNS rcode as error when non-zero.Also applies to: 522-527, 562-567, 573-576, 594-611
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tun/netstack/lneto.go` around lines 195 - 196, The DNS-over-TCP lookup path
is holding dnsScratch.mu across dialing, writing, and reading, which can block
other lookups past LookupContextHost deadlines. Update the lookup flow in
lneto.go so per-call state is used instead of the shared dnsScratch lock during
network I/O, or switch to a pool and only guard buffer checkout/return with the
mutex. Focus the fix around dnsScratch and the DNS lookup helpers that perform
the dial/write/read sequence.
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
3 participants
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.
WIP
Summary by CodeRabbit
Summary
Dial/ListenAPIs.0assignment, TCP/UDP echo (IPv4/IPv6), and DNS-over-TCP.