Skip to content

[client] Fix IPv4-only in bind proxy#5154

Merged
lixmal merged 1 commit intomainfrom
fix-ipv6-endpoint-panic
Jan 23, 2026
Merged

[client] Fix IPv4-only in bind proxy#5154
lixmal merged 1 commit intomainfrom
fix-ipv6-endpoint-panic

Conversation

@lixmal
Copy link
Copy Markdown
Collaborator

@lixmal lixmal commented Jan 22, 2026

Describe your changes

Fixes:

2026-01-22T17:01:45+01:00 WARN client/iface/bind/activity.go:85: could not find record for address invalid AddrPort
panic: As4 called on IP zero value

goroutine 382 [running]:
net/netip.Addr.As4(...)
	/home/vma/dev/go/go/src/net/netip/netip.go:719
golang.zx2c4.com/wireguard/conn.(*StdNetBind).Send(0xc0000bc5a0, {0xc000ca27c8, 0x1, 0x1}, {0x121b03e0, 0xc000e96d00})
	/home/vma/dev/go/pkg/mod/github.com/netbirdio/wireguard-go@v0.0.0-20260107100953-33b7c9d03db0/conn/bind_std.go:389 +0xc05
github.com/netbirdio/netbird/client/iface/bind.(*ICEBind).Send(0xc0002089c0, {0xc000ca27c8, 0x1, 0x1}, {0x121b03e0, 0xc000e96d00})
	/home/vma/dev/netbird/client/iface/bind/ice_bind.go:159 +0x176
golang.zx2c4.com/wireguard/device.(*Peer).SendBuffers(0xc0006de308, {0xc000ca27c8, 0x1, 0x1})
	/home/vma/dev/go/pkg/mod/github.com/netbirdio/wireguard-go@v0.0.0-20260107100953-33b7c9d03db0/device/peer.go:136 +0x272
golang.zx2c4.com/wireguard/device.(*Peer).RoutineSequentialSender(0xc0006de308, 0x1)
	/home/vma/dev/go/pkg/mod/github.com/netbirdio/wireguard-go@v0.0.0-20260107100953-33b7c9d03db0/device/send.go:519 +0x686
created by golang.zx2c4.com/wireguard/device.(*Peer).Start in goroutine 269
	/home/vma/dev/go/pkg/mod/github.com/netbirdio/wireguard-go@v0.0.0-20260107100953-33b7c9d03db0/device/peer.go:208 +0x427

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • Bug Fixes
    • Strengthened network address conversion with improved error handling to detect and properly handle conversion failures
    • Added defensive validation checks for endpoint assignments to prevent edge cases with nil or invalid values
    • Enhanced error propagation in endpoint operations to improve overall proxy stability and reliability

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 22, 2026

📝 Walkthrough

Walkthrough

The changes add error handling and validation to endpoint address conversion in the WireGuard proxy binding layer. The addrToEndpoint function now returns an error alongside the endpoint, and callers (RedirectAs) validate endpoints before using them, with nil checks protecting against incomplete conversions.

Changes

Cohort / File(s) Summary
Error handling for endpoint conversion
client/iface/wgproxy/bind/proxy.go
addrToEndpoint signature changed to return (*bind.Endpoint, error). Function now validates nil input, uses netip.AddrFromSlice with ok check, unmaps IP, and propagates errors. RedirectAs checks for errors before updating wgCurrentUsed.
Nil-safe field assignment
client/iface/wgproxy/ebpf/wrapper.go
RedirectAs now guards assignment of wgEndpointCurrentUsedAddr, updating only when endpoint and endpoint.IP are non-nil rather than unconditionally assigning.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • pappz

Poem

🐰 A hop, a skip through error's bay,
Where nil and checks now save the day,
Endpoints validated, safe and sound,
No crashing crashes to be found!
The proxy hops with carefree cheer, 🎉

🚥 Pre-merge checks | ✅ 1 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The PR description includes a stack trace and identifies it as a bug fix, but lacks details about the root cause, specific code changes made, and how the fix addresses the panic. Add a clear explanation of the root cause (nil IP address handling), describe the specific changes made to addrToEndpoint and RedirectAs functions, and explain how error handling prevents the panic.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: addressing an IPv4-only issue in the bind proxy component, which aligns with the actual code changes.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

p.wgCurrentUsed = addrToEndpoint(endpoint)
ep, err := addrToEndpoint(endpoint)
if err != nil {
log.Errorf("failed to convert endpoint address: %v", err)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The address validation is on wrong place and handled in incorrect way.
If the endpoint is nil throw and panic. Never allowed to be nil!

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it'll stick to the old endpoint in that case

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That is wrong. The address to endpoint must to work without error. In this scope the error is not acceptable path.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The error was completely ignored before. How would you like to handle it?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The first expectation is that the endpoint should not be nil. If it is nil, a panic should be thrown. If the addr nor ipv4 nor ipv6 a panic should also be thrown. Another option is to pass the addr instead of the endpoint.

@lixmal lixmal merged commit 1a32e4c into main Jan 23, 2026
38 of 40 checks passed
@lixmal lixmal deleted the fix-ipv6-endpoint-panic branch January 23, 2026 14:15
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