go-coap icon indicating copy to clipboard operation
go-coap copied to clipboard

FIX: response source address on [::]-bound servers are wrong

Open Theoyor opened this issue 2 months ago • 2 comments

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

  • Bug Fixes
    • Corrected UDP message response handling to properly propagate and route source and destination addresses in network communications.

Theoyor avatar Oct 20 '25 10:10 Theoyor

Walkthrough

Introduces a FlipSrcDst() method on ControlMessage to swap source and destination addresses. Updates message processing to propagate the control message from incoming requests to responses and invoke the flip method to correctly orient the response control message.

Changes

Cohort / File(s) Summary
UDP Control Message Enhancement
net/connUDP.go, udp/client/conn.go
Adds FlipSrcDst() method to ControlMessage with nil receiver check to safely swap Src/Dst addresses. Updates ProcessReceivedMessageWithHandler to copy control message from request to response and flip the addresses before sending.

Sequence Diagram

sequenceDiagram
    participant Handler as Message Handler
    participant Conn as Connection
    participant ControlMsg as ControlMessage
    
    Handler->>Conn: ProcessReceivedMessageWithHandler(request)
    Conn->>Conn: Acquire response message
    Conn->>Conn: Copy control message from request
    Conn->>ControlMsg: FlipSrcDst()
    ControlMsg->>ControlMsg: Swap Src ↔ Dst (if not nil)
    Conn->>Conn: Send response asynchronously

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

The changes involve straightforward logic: a simple method addition with nil safety checks and its direct application in a message processing flow. The scope is limited to two small, cohesive modifications with minimal risk surface.

Suggested labels

bug

Suggested reviewers

  • jlevesy
  • Danielius1922

Poem

🐰 A UDP hop-along to flip and swap, Where addresses dance at each stop, Request meets reply with grace, Src and Dst in their proper place, Messages hopping, never a flop! 📨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "FIX: response source address on [::]-bound servers are wrong" directly and accurately describes the primary objective of the changeset. The changes implement a fix by adding a FlipSrcDst() method to swap source and destination addresses in the control message, and then using this method in ProcessReceivedMessageWithHandler to correct response routing. The title is concise, specific enough for scanning commit history, and clearly communicates that this addresses a response source address issue on IPv6 wildcard-bound servers. It is neither vague nor misleading.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • [ ] 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

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.

coderabbitai[bot] avatar Oct 20 '25 10:10 coderabbitai[bot]

@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 *r.ControlMessage() from each original observe request, applying .FlipSrcDest() and then each time we send a new notification message I call m.SetControlMessage(&obs.controlMessage) before sending it. The ControlMessage contents on the outgoing message look correct but the UDP packet is sent the same as if SetControlMessage was not set. I haven't had time to dig further.

It feels to me like this part of the ControlMessage handling could be encapsulated in net.UDPConn somehow, as then it would apply to all outgoing packets associated with a single "connection", but I'm not really sure.

projectgus avatar Nov 10 '25 06:11 projectgus