FIX: response source address on [::]-bound servers are wrong#629
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:
📝 WalkthroughWalkthroughClient Conn stores interface index and local IP and centralizes attaching control information to outgoing messages. Server now tracks connections per (remote, local) UDP address by propagating laddr through creation and lookup paths. Tests and editor configs updated accordingly. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Network as Network
participant Server as Server
participant Conn as Conn\n(per raddr,laddr)
Client->>Network: UDP request
Network->>Server: Deliver packet + ControlMessage
Server->>Server: extract IfIndex and dest from ControlMessage
Server->>Conn: getOrCreateConn(raddr, laddr)
Conn->>Conn: store interfaceIndex (atomic) and localAddr (atomic)
Server->>Conn: ProcessReceivedMessage
Conn->>Conn: cache interface/local info
Server->>Conn: Prepare response
Conn->>Conn: upsertControlInformation(msg) — attach IfIndex/Src
Conn->>Network: Send response with control information
Network->>Client: UDP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
net/connUDP.go(1 hunks)udp/client/conn.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
udp/client/conn.go (1)
net/connUDP.go (1)
ControlMessage(27-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Fuzzing
- GitHub Check: test-for-fork (windows-latest)
🔇 Additional comments (1)
net/connUDP.go (1)
60-65: LGTM! Clean implementation of address swap.The nil check prevents panics and the tuple assignment correctly swaps the source and destination addresses in a single operation.
|
@Theoyor FWIW I can confirm this fixes the described issue for me, thanks for submitting it. Unfortunately Observer notifications also have the same problem, and they're not fixed by this PR because they use a different code path to build the outgoing message. I tried storing It feels to me like this part of the ControlMessage handling could be encapsulated in |
Theoyor
force-pushed
the
fix/wildcard-address-wrong-srcaddr
branch
from
a145b7c to
3054a3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@udp/server/server.go`:
- Around line 267-340: The connection key includes laddr but the created
client/session ignores laddr so outbound packets may use a wildcard source;
update getOrCreateConn to propagate the provided laddr into the created
session/client (e.g., pass laddr into NewSession or immediately set the
resulting client.Conn local/source address field that client/conn.go reads such
as Conn.localAddr) so Server.NewConn(..., laddr) actually controls the source
selection for outbound packets when calling NewSession and
client.NewConnWithOpts; ensure the change touches getOrCreateConn, NewSession
call site, and/or initialization of client.Conn so laddr is used consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0ac462fc-9916-4127-9ac0-25e8e35098f7
📒 Files selected for processing (4)
.vscode/launch.jsonudp/client/conn.goudp/server/server.goudp/server_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@udp/client/conn.go`:
- Around line 865-879: upsertControlInformation currently takes the address of a
stack-allocated cm and passes it to msg.UpsertControlMessage which stores the
pointer (via Message.SetControlMessage), causing a use-after-free; fix by
allocating the ControlMessage on the heap instead of a local variable (construct
and pass &coapNet.ControlMessage{IfIndex: ifIndex, Src: *localAddr} or
equivalent) so msg.UpsertControlMessage receives a heap pointer and no dangling
pointer is stored in Message.controlMessage.
- Around line 833-837: The code currently stores a pointer to the pooled
ControlMessage's destination (cc.localAddr.Store(&cm.Dst)), which becomes
invalid once req is released; instead allocate and store a copy of the IP bytes:
when handling req.ControlMessage() in the block that sets cc.localAddr and
cc.interfaceIndex, create a new net.IP (or copy the slice contents into a
freshly allocated slice) from cm.Dst and store that copy (not &cm.Dst) so later
dereferences of localAddr (used at the dereference site around where *localAddr
is read) reference stable, non-pooled memory.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@udp/client/conn.go`:
- Around line 834-839: The code currently unconditionally writes
cc.interfaceIndex.Store(int64(req.ControlMessage().GetIfIndex())) and may store
a zero-length localAddr when a control message exists but cm.Dst is empty;
update the logic to first fetch cm := req.ControlMessage() and only if cm != nil
call cc.interfaceIndex.Store(int64(cm.GetIfIndex())), and only call
cc.localAddr.Store(&dst) when cm.Dst is non-empty (len(cm.Dst) > 0) after
copying it; do not change interfaceIndex or localAddr when the control message
is absent so previously learned metadata is preserved.
|
Hi @jkralik, I had to work on other stuff for some time, but I had time to get back to this topic now. I reimplemented a solution that would work for observing as well (@projectgus). The idea is that connections are now also keyed by local address using the Dst field of an incoming control message if present. If it is a server initiated connection, the client has no assumptions on the content so the listeners LocalAddress is used (which could possibly be [::] or 0.0.0.0). The goal of that is to be able to save the local address in the client.Conn instances for that exchange. And as it is keyed that way, there shouldn't be any collisions/misassignments. The same was done for the interface as this also shouldn't change in one exchange. Both are upserted in the same control message (if they contain actual values) so that they don't block each other as in the first proposal and also don't overwrite purposefully set CMs. I tested the changes manually and locally both with libcoap and in our companies go project for standard request response, observe and multicast. That worked. The unit tests are also working apart from the lint and the windows tests which I saw failed for other PRs as well so I guess it is fine as is. I would be happy to get a review. |
… different addresses than sent to.
jkralik
force-pushed
the
fix/wildcard-address-wrong-srcaddr
branch
from
8f2823b to
7eb30ba
Compare
|
Hi @Theoyor, I’ve reviewed your changes and pushed a few additional commits. Could you please retest it? If everything works for you, I’ll go ahead and merge it 🙂 |
|
Hi @jkralik I tested again and it works well. Thanks for your additions and review. Would be lovely, if you merge it. Thank you! |
|
@Theoyor Thank you for your contribution 😃 |
3 participants
When there are multiple addresses (a,b) on an interface and the coap-server is bound to [::] a client bound to a, sending to b received the response from a. This causes severe issues.
Fixed in this PR handing over the control message to the response and inverting src and destination. I can not add a unit test for this case, as multiple addresses would need to be created in the test environment.
@jkralik can you have a look?
Summary by CodeRabbit
Refactor
Chores