Fix sending to IPv4 addresses from an IPv6 dual-stack socket (#640) #641
Conversation
📝 WalkthroughWalkthroughThis change fixes a UDP send failure on macOS by replacing conditional IPv4/IPv6 wrapper routing with a direct Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
Hi @ltlobaro . Thank you for your contributing. Pls there is an issue in the tests, discovered by CI on Ubuntu: Pls could you look at it :). Thx |
This commit ensures that the unit tests are actually using IPv6 when testing IPv6 functionality. However, this may add adtional runtime requirements to the unit tests: - The runtime needs a network interface which is not loopback, is up, and has multicast enabled - The interface needs to have at least one routable, unicast IP address, either link-local or global.
|
Hi @jkralik, thank you for your time! It turns out the tests and implementation are highly dependent on the underlying platform, i.e. linux or darwin. I fixed a bug in the writeTo method: The control message is now marshalled depending on the destination IP address type, which no longer triggers the error. Also, I changed the unit tests to actually use IPv6 when testing IPv6 functionality. Before, the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@net/connUDP_internal_test.go`:
- Around line 355-357: The filter in the test excludes link-local addresses but
the commit says link-local or global unicast should be allowed; update the
condition that currently checks ip.IsPrivate() and ip.IsGlobalUnicast() to also
accept link-local by including ip.IsLinkLocalUnicast() (i.e. only continue when
!ip.IsPrivate() && !ip.IsGlobalUnicast() && !ip.IsLinkLocalUnicast()), or
alternatively update the commit/test description to remove the "link-local"
claim—locate the check using ip.IsPrivate() and ip.IsGlobalUnicast() in
net/connUDP_internal_test.go and adjust it or the commit message accordingly.
- Around line 66-85: The dual-stack send logic was removed and must be restored
so IPv4 destinations use an IPv4 packet connection when the local socket is
IPv6; in the send path (where WriteMsgUDP is now invoked) detect when
!IsIPv6(raddr.IP) && c.packetConn.IsIPv6(), construct an ipv4 packet wrapper
(packetConnIPv4{packetConn: ipv4.NewPacketConn(c.connection)}) and call its
WriteTo(buffer, cm, raddr) instead of the direct WriteMsgUDP call to ensure the
correct IPv4 control message format is used.
🧹 Nitpick comments (1)
net/connUDP_internal_test.go (1)
30-32: Consider handling missing IPv6 addresses gracefully.If the selected interface doesn't have an IPv6 address that passes the filter (private or global unicast),
getIfaceAddr(t, iface, false)will fail. The subsequent tests relying onv6would then also fail.Consider skipping IPv6-specific tests when no suitable IPv6 address is available:
♻️ Suggested improvement
- ifaceIpv6 := getIfaceAddr(t, iface, false) - v6, err := net.ResolveUDPAddr(udpNetwork, "["+ifaceIpv6.String()+"]:2154") - require.NoError(t, err) + ifaceIpv6, hasIPv6 := tryGetIfaceAddr(t, iface, false) + var v6 *net.UDPAddr + if hasIPv6 { + v6, err = net.ResolveUDPAddr(udpNetwork, "["+ifaceIpv6.String()+"]:2154") + require.NoError(t, err) + }Then conditionally skip the IPv6 test cases when
v6 == nil.
net/connUDP_internal_test.go
Outdated
| { | ||
| name: "send to v4 from v6 socket", | ||
| args: args{ | ||
| ctx: context.Background(), | ||
| listenNetwork: udpNetwork, | ||
| udpAddr: b, | ||
| buffer: []byte("hello world"), | ||
| }, | ||
| wantErr: false, | ||
| }, | ||
| { | ||
| name: "send to v6 from v6 socket", | ||
| args: args{ | ||
| ctx: context.Background(), | ||
| listenNetwork: udpNetwork, | ||
| udpAddr: v6, | ||
| buffer: []byte("hello world"), | ||
| }, | ||
| wantErr: false, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the test file
fd -name "connUDP_internal_test.go" -type fRepository: plgd-dev/go-coap
Length of output: 176
🏁 Script executed:
# Check for ListenUDP usage and dual-stack/V6ONLY handling in the repository
rg -n "ListenUDP|V6ONLY|IPV6_V6ONLY|dualstack" --type=go -B 2 -A 2Repository: plgd-dev/go-coap
Length of output: 13570
🏁 Script executed:
# Let's also check the full context of the test file to understand the setup
fd -name "connUDP_internal_test.go" -type f -exec wc -l {} \;Repository: plgd-dev/go-coap
Length of output: 290
🏁 Script executed:
# Find the test file
find . -name "connUDP_internal_test.go" -type fRepository: plgd-dev/go-coap
Length of output: 90
🏁 Script executed:
# Search for V6ONLY and dualstack references using rg
rg "V6ONLY|IPV6_V6ONLY|dualstack" --type=go -B 2 -A 2Repository: plgd-dev/go-coap
Length of output: 42
🏁 Script executed:
# Get the file and check its size
find . -name "connUDP_internal_test.go" -type f -exec wc -l {} \;Repository: plgd-dev/go-coap
Length of output: 94
🏁 Script executed:
# Read the test file to see the test setup and lines 66-85
cat -n ./net/connUDP_internal_test.go | head -150Repository: plgd-dev/go-coap
Length of output: 4423
🏁 Script executed:
# Search for udpNetwork variable definition
rg "udpNetwork\s*:?=" ./net/connUDP_internal_test.go -B 2 -A 2Repository: plgd-dev/go-coap
Length of output: 133
🏁 Script executed:
# Also check for any socket option handling related to the tests
rg "SetIPv6Only|IPV6_V6ONLY|Control" ./net/connUDP_internal_test.go -B 2 -A 2Repository: plgd-dev/go-coap
Length of output: 841
🏁 Script executed:
# Look at the connUDP.go implementation to see how the socket is set up
cat -n ./net/connUDP.go | head -250Repository: plgd-dev/go-coap
Length of output: 8319
🏁 Script executed:
# Check what NewUDPConn does with the socket
rg -A 30 "func NewUDPConn" ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 921
🏁 Script executed:
# Check if there's any socket option setting for dual-stack
rg "SetIPv6Only|SO_REUSEADDR|Control|SyscallConn" ./net/connUDP.go -B 2 -A 5Repository: plgd-dev/go-coap
Length of output: 6402
🏁 Script executed:
# Read more of connUDP.go to see how newPacketConn determines IPv4 vs IPv6
sed -n '245,300p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 1553
🏁 Script executed:
# Check if there's any handling of the "udp" network type specifically
rg "network.*udp|newPacketConn" ./net/connUDP.go -B 3 -A 10Repository: plgd-dev/go-coap
Length of output: 3015
🏁 Script executed:
# Check if there's any socket option handling in the writeTo method
sed -n '380,450p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 2141
🏁 Script executed:
# Look for any SendMsgs or syscall-level handling
rg "sendmsg|SendMsg|sendto|Sendto|syscall" ./net/connUDP.go -B 3 -A 3Repository: plgd-dev/go-coap
Length of output: 42
🏁 Script executed:
# Check the test to understand what error message we'd expect on failure
rg "sendmsg.*invalid" ./net/connUDP_internal_test.goRepository: plgd-dev/go-coap
Length of output: 42
🏁 Script executed:
# Check for any recent fixes or workarounds related to dual-stack in git history
git log --oneline --all -20 | head -20Repository: plgd-dev/go-coap
Length of output: 1218
🏁 Script executed:
# Look at the writeTo method more completely to see control message handling
sed -n '312,375p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 1644
🏁 Script executed:
# Check if there are any platform-specific files
find ./net -name "*linux*" -o -name "*darwin*" -o -name "*windows*"Repository: plgd-dev/go-coap
Length of output: 82
🏁 Script executed:
# Check the recent commit that fixes control message passing
git show 4dd5869 --statRepository: plgd-dev/go-coap
Length of output: 11241
🏁 Script executed:
# Also check the Linux IPv4/IPv6 binding fix
git show 91a04ea --statRepository: plgd-dev/go-coap
Length of output: 838
🏁 Script executed:
# Look at the writeTo method specifically to see if control messages are correctly passed
sed -n '455,530p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 2263
🏁 Script executed:
# Look at the actual changes made in the recent fix commits
git show 91a04ea -- net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 1461
🏁 Script executed:
# Check the test file changes from that commit
git show 91a04ea -- net/connUDP_internal_test.goRepository: plgd-dev/go-coap
Length of output: 4600
🏁 Script executed:
# Check the most recent commit that touched the tests
git show 4dd5869 -- net/connUDP_internal_test.go | head -200Repository: plgd-dev/go-coap
Length of output: 5014
🏁 Script executed:
# Verify current state: are the dual-stack tests still present?
sed -n '48,105p' ./net/connUDP_internal_test.goRepository: plgd-dev/go-coap
Length of output: 1326
🏁 Script executed:
# Check if the fix from commit 91a04ea is currently in the code
sed -n '514,530p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 497
🏁 Script executed:
# Look at the writeTo implementation to confirm the fix is present
rg -A 15 "func \(c \*UDPConn\) writeTo" ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 999
🏁 Script executed:
# Get the complete writeTo function
sed -n '520,570p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 1008
🏁 Script executed:
# Search for the specific fix that checks IsIPv6(raddr.IP) && c.packetConn.IsIPv6()
rg "IsIPv6.*raddr.*packetConn" ./net/connUDP.go -B 3 -A 10Repository: plgd-dev/go-coap
Length of output: 42
🏁 Script executed:
# Let's check the exact current implementation to see if the fix is there or not
sed -n '513,545p' ./net/connUDP.goRepository: plgd-dev/go-coap
Length of output: 844
🏁 Script executed:
# Check git log to see if the fix was reverted
git log --oneline --all -- net/connUDP.go | head -20Repository: plgd-dev/go-coap
Length of output: 1068
🏁 Script executed:
# Check what the most recent commit (4dd5869) changed to writeTo
git show 4dd5869 -- net/connUDP.go | grep -A 50 "func (c \*UDPConn) writeTo"Repository: plgd-dev/go-coap
Length of output: 2647
🏁 Script executed:
# Also check if the previous fix was reverted
git show 4dd5869:net/connUDP.go | sed -n '513,545p'Repository: plgd-dev/go-coap
Length of output: 844
🏁 Script executed:
# Check if commit 91a04ea's fix is in the current HEAD
git show HEAD:net/connUDP.go | sed -n '513,540p'Repository: plgd-dev/go-coap
Length of output: 748
🏁 Script executed:
# Compare the two implementations side by side - check if the IPv4 wrapping logic exists
git show 91a04ea:net/connUDP.go | sed -n '513,540p'Repository: plgd-dev/go-coap
Length of output: 1115
🏁 Script executed:
# Check the differences between 91a04ea and 4dd5869 for writeTo
git diff 91a04ea 4dd5869 -- net/connUDP.go | grep -A 20 -B 5 "writeTo"Repository: plgd-dev/go-coap
Length of output: 1010
🏁 Script executed:
# Check what changed in the entire writeTo function between these commits
git show 91a04ea:net/connUDP.go | sed -n '513,530p' > /tmp/old_writeTo.txt
git show 4dd5869:net/connUDP.go | sed -n '513,530p' > /tmp/new_writeTo.txt
diff -u /tmp/old_writeTo.txt /tmp/new_writeTo.txtRepository: plgd-dev/go-coap
Length of output: 1068
These dual-stack test cases will fail on Linux and require protocol wrapping logic to be restored.
The tests attempt to send IPv4 and IPv6 packets from a dual-stack socket ("udp") with control messages. On Linux, when net.ListenUDP("udp", addr) creates a socket bound to [::], the code determines the protocol handler from the local address (IPv6), but then tries to send IPv4 packets with IPv6 control messages, causing sendmsg: invalid argument errors.
A fix for this was implemented in commit 91a04ea that wraps IPv4 destinations with an IPv4 packet connection when the socket is IPv6-based. However, this logic was removed in the most recent commit (4dd5869), which replaced it with direct WriteMsgUDP calls. The wrapping logic needs to be restored:
if !IsIPv6(raddr.IP) && c.packetConn.IsIPv6() {
pc := packetConnIPv4{packetConn: ipv4.NewPacketConn(c.connection)}
return pc.WriteTo(buffer, cm, raddr)
}This ensures that IPv4 traffic sent from a dual-stack socket uses the appropriate IPv4 control message format rather than attempting IPv6 control messages for IPv4 destinations.
🤖 Prompt for AI Agents
In `@net/connUDP_internal_test.go` around lines 66 - 85, The dual-stack send logic
was removed and must be restored so IPv4 destinations use an IPv4 packet
connection when the local socket is IPv6; in the send path (where WriteMsgUDP is
now invoked) detect when !IsIPv6(raddr.IP) && c.packetConn.IsIPv6(), construct
an ipv4 packet wrapper (packetConnIPv4{packetConn:
ipv4.NewPacketConn(c.connection)}) and call its WriteTo(buffer, cm, raddr)
instead of the direct WriteMsgUDP call to ensure the correct IPv4 control
message format is used.
…win" This reverts commit 4dd5869.
|
Ok I reverted my changes to the unit tests to let them pass. Btw how can I stop the slop? :( |
|
Thx for contributing. Good job 👍 |
2 participants
This PR resolves sending packets to IPv4 adresses on an IPv6 dual-stack socket. It resolves #640 and is probably related to #581.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.