fix: correct IPv4 option wire format for SOME/IP-SD interop#239
fix: correct IPv4 option wire format for SOME/IP-SD interop#239
Conversation
📝 WalkthroughWalkthroughIPv4 multicast and endpoint options in SD message serialization are restructured to include a Protocol field and adjust payload lengths to 9 bytes, aligning with AUTOSAR specifications and enabling interoperability with vsomeip. Public accessors and tests are updated accordingly. ChangesIPv4 Option Wire-Format Update
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
vtz
force-pushed
the
fix/sd-ipv4-option-interop-238
branch
2 times, most recently
from
bf73352 to
d1b7e99
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3581bfa632
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| constexpr size_t kPayloadBytes = 8; | ||
| if (offset + kPayloadBytes > data.size()) { |
There was a problem hiding this comment.
The new backward-compatibility path for length_=7 is internally inconsistent: this function accepts 7-byte legacy multicast options, but immediately requires kPayloadBytes = 8 and later reads protocol+port as if the extra byte exists. For old opensomeip packets (which actually have 7 payload bytes), deserialization now either fails outright (offset + 8 > data.size()) or, when another option follows, can consume one byte from the next option and corrupt parsing. This breaks interoperability with the very legacy format the change says it supports.
Useful? React with 👍 / 👎.
Fix two spec violations in IPv4EndpointOption and IPv4MulticastOption
that prevented interoperability with vsomeip (and any spec-compliant
SOME/IP-SD implementation):
1. Length field: the spec (feat_req_someipsd_129) mandates Length=0x0009
("all bytes except length and type fields"). opensomeip wrote 8,
counting only bytes after the full 4-byte header. The deserializer's
bounds check then rejected spec-compliant length=9 packets because
the base class had already consumed the Reserved byte.
2. Byte order: ipv4_address_ stores addr.s_addr (NBO in memory). The
shift-based serializer extracted integer MSB first, reversing IP
bytes on the wire on little-endian hosts. The same double-conversion
bug affected port fields via redundant htons/ntohs calls.
Both fixes are symmetric so opensomeip-to-opensomeip traffic is
unaffected; the deserializer accepts both length=8 (backward compat)
and length=9 (spec-correct).
Also fixes IPv4MulticastOption: adds missing L4-Proto byte per spec,
corrects length (7→9) and byte order, and removes a pre-existing
double-htons on multicast port in sd_server.cpp.
Closes #238
Made-with: Cursor
vtz
force-pushed
the
fix/sd-ipv4-option-interop-238
branch
from
d1b7e99 to
b47a0f4
Compare
|
Re: review comment on legacy length=7 path — Good catch, but this is moot now. The backward-compat path has been removed entirely in the latest push: both options now strictly enforce |
Simulates the exact wire bytes vsomeip sends for a server at 10.10.10.20:30510/UDP and verifies opensomeip deserializes the address and port correctly. Also confirms opensomeip's serialized output is byte-for-byte identical to vsomeip's wire format. Co-authored-by: Cursor <cursoragent@cursor.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
include/sd/sd_message.h (1)
168-172:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale/missing byte-order comments on
port_after the wire-format fix.The pre-existing comment on line 171 still says
// Port in network byte order, but after this PR the serialize/deserialize paths requireport_to be in host byte order (shifts produce NBO wire bytes directly fromport_, and tests assertoption.set_port(30509)followed byEXPECT_EQ(get_port(), 30509)round-trip without swapping). The newport_onIPv4MulticastOption(line 196) follows the same convention but has no comment at all. Leaving these as-is invites a future contributor to "fix" callers by pre-swapping withhtonsand silently reintroduce the bug this PR removed fromsd_server.cpp.📝 Proposed comment fix
private: uint8_t protocol_{0}; // 0x06 = TCP, 0x11 = UDP uint32_t ipv4_address_{0}; // IPv4 address in network byte order - uint16_t port_{0}; // Port in network byte order + uint16_t port_{0}; // Port in host byte order (serializer writes NBO on the wire) };private: uint32_t ipv4_address_{0}; // IPv4 address in network byte order uint8_t protocol_{0x11}; // L4 protocol (default UDP per spec) - uint16_t port_{0}; + uint16_t port_{0}; // Port in host byte order (serializer writes NBO on the wire) };Also applies to: 193-197
🤖 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 `@include/sd/sd_message.h` around lines 168 - 172, The comments on the member port_ in the IPv4Option (port_) and IPv4MulticastOption classes are stale: port_ is stored in host byte order (serialize/deserialize perform shifts to produce network bytes) so callers must not pre-swap with htons/ntohs; update the inline comments for port_ (and add one for IPv4MulticastOption::port_) to state "Port in host byte order (serialized as network byte order)" and mention that set_port/get_port use/return host-order values to prevent future callers from applying htons/ntohs erroneously. Ensure references to set_port/get_port are noted in the comment so reviewers immediately see the intended API contract.src/sd/sd_message.cpp (1)
288-297:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWarning prints octets from the NBO value instead of host order — inconsistent with the multicast path.
Inside the validation block, the shifts operate on
ipv4_address_, which after this PR holds the NBO (s_addr) value. On a little-endian host, that prints octets in reversed order. The bug is currently latent only because the predicate fires solely on0and0xFFFFFFFF(both palindromic), but the multicast counterpart at lines 386-389 already does the right thing using the localhost_addr— these two near-identical blocks should match so a future loosening of the predicate (e.g. to log loopback or non-routable addresses) doesn't suddenly emit reversed dotted-quads.🔧 Proposed fix — use the already-computed `host_addr`
// Validate IP address (REQ_SD_064_E01) if (ipv4_address_ == 0 || ipv4_address_ == 0xFFFFFFFFU) { std::cout << "Warning: Invalid IP address in endpoint option: " - << ((ipv4_address_ >> 24U) & 0xFFU) << "." - << ((ipv4_address_ >> 16U) & 0xFFU) << "." - << ((ipv4_address_ >> 8U) & 0xFFU) << "." - << (ipv4_address_ & 0xFFU) << '\n'; + << ((host_addr >> 24U) & 0xFFU) << "." + << ((host_addr >> 16U) & 0xFFU) << "." + << ((host_addr >> 8U) & 0xFFU) << "." + << (host_addr & 0xFFU) << '\n'; // Continue processing despite invalid address }🤖 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 `@src/sd/sd_message.cpp` around lines 288 - 297, The warning prints IP octets from the NBO-stored ipv4_address_ (reversed on little-endian hosts); update the validation block that checks ipv4_address_ (the "Validate IP address (REQ_SD_064_E01)" section) to print the already-computed host-order value host_addr instead of ipv4_address_ so it matches the multicast block behavior—locate the block referencing ipv4_address_ and replace the octet extraction to use host_addr when formatting the dotted-quad.tests/test_sd.cpp (1)
636-643: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMulticast wire-format test is too weak to lock in this PR's fixes.
IPv4MulticastOptionSerializationonly assertsserialized.size() > 0u. The PR changes the multicast option in three observable ways — payload length 7→9, address now written in NBO, new Protocol byte (default0x11), plus the dependentsd_server.cppremoval ofsomeip_htonson the multicast port — none of which are exercised here. A regression on any of them (e.g. someone re-introducinghtonson the port, or a builder forgetting to set protocol) would still pass this test.Recommend adding a byte-for-byte assertion mirroring
IPv4EndpointOptionSerializationso the multicast path has the same regression guard as the endpoint path.🧪 Proposed test expansion
TEST_F(SdTest, IPv4MulticastOptionSerialization) { IPv4MulticastOption original; original.set_ipv4_address(0xEFFFFFFB); // 239.255.255.251 (host-order int) original.set_port(30490); auto serialized = original.serialize(); - EXPECT_GT(serialized.size(), 0u); + // Length(2) + Type(1) + Reserved(1) + IPv4(4) + Reserved(1) + Proto(1) + Port(2) = 12 + ASSERT_EQ(serialized.size(), 12u); + + // Length = 0x0009 per spec + EXPECT_EQ(serialized[0], 0x00); + EXPECT_EQ(serialized[1], 0x09); + // Type = 0x14 (IPv4 multicast) + EXPECT_EQ(serialized[2], 0x14); + EXPECT_EQ(serialized[3], 0x00); // Reserved + // 239.255.255.251 in NBO + EXPECT_EQ(serialized[4], 0xEF); + EXPECT_EQ(serialized[5], 0xFF); + EXPECT_EQ(serialized[6], 0xFF); + EXPECT_EQ(serialized[7], 0xFB); + EXPECT_EQ(serialized[8], 0x00); // Reserved + EXPECT_EQ(serialized[9], 0x11); // Default protocol = UDP + // Port 30490 in NBO = 0x771A + EXPECT_EQ(serialized[10], 0x77); + EXPECT_EQ(serialized[11], 0x1A); }🤖 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 `@tests/test_sd.cpp` around lines 636 - 643, Update the TEST_F named IPv4MulticastOptionSerialization to assert the full serialized byte sequence instead of only size: build an expected byte vector for IPv4MulticastOption that reflects the new wire format (payload length 9, IPv4 address in NBO, protocol byte 0x11, and port in network byte order) and compare it against original.serialize(); use the IPv4MulticastOption methods (set_ipv4_address, set_port and any protocol setter if present) and the serialize() result to perform a byte-for-byte equality check to guard against regressions like reintroducing htons or omitting the protocol byte.
🤖 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.
Outside diff comments:
In `@include/sd/sd_message.h`:
- Around line 168-172: The comments on the member port_ in the IPv4Option
(port_) and IPv4MulticastOption classes are stale: port_ is stored in host byte
order (serialize/deserialize perform shifts to produce network bytes) so callers
must not pre-swap with htons/ntohs; update the inline comments for port_ (and
add one for IPv4MulticastOption::port_) to state "Port in host byte order
(serialized as network byte order)" and mention that set_port/get_port
use/return host-order values to prevent future callers from applying htons/ntohs
erroneously. Ensure references to set_port/get_port are noted in the comment so
reviewers immediately see the intended API contract.
In `@src/sd/sd_message.cpp`:
- Around line 288-297: The warning prints IP octets from the NBO-stored
ipv4_address_ (reversed on little-endian hosts); update the validation block
that checks ipv4_address_ (the "Validate IP address (REQ_SD_064_E01)" section)
to print the already-computed host-order value host_addr instead of
ipv4_address_ so it matches the multicast block behavior—locate the block
referencing ipv4_address_ and replace the octet extraction to use host_addr when
formatting the dotted-quad.
In `@tests/test_sd.cpp`:
- Around line 636-643: Update the TEST_F named IPv4MulticastOptionSerialization
to assert the full serialized byte sequence instead of only size: build an
expected byte vector for IPv4MulticastOption that reflects the new wire format
(payload length 9, IPv4 address in NBO, protocol byte 0x11, and port in network
byte order) and compare it against original.serialize(); use the
IPv4MulticastOption methods (set_ipv4_address, set_port and any protocol setter
if present) and the serialize() result to perform a byte-for-byte equality check
to guard against regressions like reintroducing htons or omitting the protocol
byte.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 668e0dec-60eb-481b-afdb-c989504deae9
📒 Files selected for processing (4)
include/sd/sd_message.hsrc/sd/sd_message.cppsrc/sd/sd_server.cpptests/test_sd.cpp
1 participant
Summary
Fixes two spec violations in
IPv4EndpointOptionandIPv4MulticastOptionthat prevented interoperability with vsomeip (and any spec-compliant SOME/IP-SD implementation):feat_req_someipsd_129) mandatesLength=0x0009("all bytes except length and type fields"). opensomeip wrote8, counting only bytes after the full 4-byte header. The deserializer then rejected spec-compliantlength=9packets from vsomeip because the bounds check used the declared length after the base class had already consumed the Reserved byte it counts.ipv4_address_storesaddr.s_addr(NBO in memory). The shift-based serializer extracted integer MSB first, reversing IP bytes on the wire on little-endian hosts (e.g.192.168.1.100→ wire bytes64 01 A8 C0instead ofC0 A8 01 64). The same double-conversion bug affected port fields via redundanthtons/ntohscalls around shifts.htonson multicast port insd_server.cpp.Both fixes are symmetric so opensomeip↔opensomeip traffic is unaffected. The deserializer accepts both
length=8(backward compat) andlength=9(spec-correct).Closes #238
Test plan
TC_SD_INTEROP_001through004) that reproduce the exact failures from issue Interoperability with VSome/IP #238IPv4EndpointOptionSerializationtest updated to verify spec-correct wire formatSummary by CodeRabbit
Release Notes
New Features
Bug Fixes