Conversation
📝 WalkthroughWalkthroughAdds ChangesMalformed message error and RST handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Peer
participant Serve
participant Conn as cc.Process
participant Helper as handleProcessError
Peer->>Serve: malformed confirmable datagram
Serve->>Conn: Process(cm, buf)
Conn-->>Serve: *MalformedMessageError
Serve->>Helper: classify malformed / unknown version
Helper->>Peer: write RST datagram with matching MID
Serve->>Serve: continue receive loop
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR adjusts the UDP server’s behavior on malformed inbound CoAP datagrams so that a single bad packet does not tear down the per-peer session, and (when possible) replies to malformed confirmable messages with a Reset (RST).
Changes:
- Add malformed-packet classification and best-effort extraction of header metadata on decode failure.
- Send an RST for malformed Confirmable messages when the CoAP header is present and usable.
- Add a regression test ensuring malformed packets don’t recreate/tear down the per-peer connection.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| udp/server/server.go | Keeps per-peer connection alive on decode failures and optionally sends RST for malformed CON packets. |
| udp/server_test.go | Adds a UDP-level test asserting RST behavior and connection reuse across malformed packets. |
| udp/coder/coder.go | Ensures header fields are populated early so callers can read Type/MID even when decode fails later. |
| udp/client/conn.go | Wraps decode failures in a MalformedMessageError carrying best-effort header metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #676 +/- ##
==========================================
- Coverage 76.63% 76.48% -0.15%
==========================================
Files 77 77
Lines 6095 6184 +89
==========================================
+ Hits 4671 4730 +59
- Misses 1042 1058 +16
- Partials 382 396 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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
🧹 Nitpick comments (2)
udp/client/conn.go (1)
944-962: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
errors.Isover re-parsing the raw version bits.Line 952 re-implements the version check that
coder.Decodealready performs and reports viacoder.ErrMessageInvalidVersion. Duplicating the bit-mask logic here couples this file to the coder's wire-format details and can silently drift if that layout changes.♻️ Proposed refactor
malformedErr := &MalformedMessageError{Err: err} - if len(datagram) >= 1 && datagram[0]>>6 != 1 { + if errors.Is(err, coder.ErrMessageInvalidVersion) { malformedErr.InvalidVersion = true } if len(datagram) >= 4 && !malformedErr.InvalidVersion {🤖 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 `@udp/client/conn.go` around lines 944 - 962, The version check in Conn.Process is duplicating coder wire-format logic by inspecting datagram[0] directly. Update the malformed-message handling to detect coder.ErrMessageInvalidVersion with errors.Is on the unmarshal error instead of re-parsing the version bits, and keep the existing MalformedMessageError population and message release flow in Conn.Process.udp/server/server.go (1)
122-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant/fragile classification alongside
errors.As.
maybeRejectMalformedConfirmablealready narrows to*client.MalformedMessageErrorviaerrors.As(which is the only type that reaches this path fromConn.Process). The extra sentinel-error whitelist here is a second, weaker gate that must be kept manually in sync with every decode-error type coder/message can produce — a future new decode error not added to this list would silently suppress the RST even thoughmalformedErris fully populated.Consider dropping
isMalformedMessageErrorand relying solely onerrors.As(processErr, &malformedErr)plus the existingHasHeader/Type/MID checks.♻️ Proposed simplification
-func isMalformedMessageError(err error) bool { - return errors.Is(err, coder.ErrMessageTruncated) || - errors.Is(err, coder.ErrMessageInvalidVersion) || - errors.Is(err, message.ErrInvalidTokenLen) || - errors.Is(err, message.ErrInvalidOptionHeaderExt) || - errors.Is(err, message.ErrOptionTruncated) || - errors.Is(err, message.ErrOptionUnexpectedExtendMarker) || - errors.Is(err, message.ErrOptionsTooSmall) || - errors.Is(err, message.ErrInvalidEncoding) -} - func makeResetDatagram(mid int32) []byte {func (s *Server) maybeRejectMalformedConfirmable(l *coapNet.UDPConn, cc *client.Conn, cm *coapNet.ControlMessage, processErr error) { - if !isMalformedMessageError(processErr) { - return - } var malformedErr *client.MalformedMessageError if !errors.As(processErr, &malformedErr) { return }🤖 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 `@udp/server/server.go` around lines 122 - 131, The malformed-message check in isMalformedMessageError is redundant and fragile because maybeRejectMalformedConfirmable already uses errors.As against *client.MalformedMessageError from Conn.Process. Remove the sentinel-error whitelist and rely on the existing errors.As-based narrowing plus the HasHeader/Type/MID validation in maybeRejectMalformedConfirmable, keeping the logic centered on the MalformedMessageError path.
🤖 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 `@udp/server/server.go`:
- Around line 238-243: The cc.Process error handling currently treats every
failure as malformed, but only client.MalformedMessageError should use
maybeRejectMalformedConfirmable and continue. Update the error path in server.go
around cc.Process to detect malformed confirmable cases explicitly, and for all
other cc.Process errors keep the close-on-error fallback so oversized-datagram
and request-monitor failures still close the connection while preserving the
existing malformed RST/continue behavior.
---
Nitpick comments:
In `@udp/client/conn.go`:
- Around line 944-962: The version check in Conn.Process is duplicating coder
wire-format logic by inspecting datagram[0] directly. Update the
malformed-message handling to detect coder.ErrMessageInvalidVersion with
errors.Is on the unmarshal error instead of re-parsing the version bits, and
keep the existing MalformedMessageError population and message release flow in
Conn.Process.
In `@udp/server/server.go`:
- Around line 122-131: The malformed-message check in isMalformedMessageError is
redundant and fragile because maybeRejectMalformedConfirmable already uses
errors.As against *client.MalformedMessageError from Conn.Process. Remove the
sentinel-error whitelist and rely on the existing errors.As-based narrowing plus
the HasHeader/Type/MID validation in maybeRejectMalformedConfirmable, keeping
the logic centered on the MalformedMessageError path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50eb4a65-e0bf-494b-a4aa-c93ed7178251
📒 Files selected for processing (4)
udp/client/conn.goudp/coder/coder.goudp/server/server.goudp/server_test.go
|
Fixes #664 point 1
Summary by CodeRabbit
Summary by CodeRabbit
#664point 1