diff --git a/CHANGELOG.md b/CHANGELOG.md index 6bf7c3c..dcdd0eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,125 @@ # Changelog +## [0.13.0] + +Contains a breaking change to the serialization trait surface, so this release +takes the major position under this crate's 0.x convention. As in 0.10.0 +through 0.12.0, the `version` in `Cargo.toml` is bumped here rather than left +to release-plz so that `cargo-semver-checks` compares against the version this +change actually lands as. + +### Breaking — migrate wire encode/decode onto `automotive-wire-codec` 0.3 + +The hand-rolled `WireFormat` trait and its supporting `Option`-returning +parsers are gone, replaced by the `automotive-wire-codec` crate's `Encode` / +`Decode` / `DecodeIter` traits. This is a from-scratch rewrite of the +serialization layer's *trait surface*; on-wire bytes are unchanged (every +golden-bytes test, including under `--features server`, is green before and +after). + +- **`WireFormat` removed — replaced by `automotive_wire_codec::Encode`.** + `required_size(&self) -> usize` is now `encoded_size(&self) -> + Result` (fallible: some encodings, e.g. SD configuration + strings, can exceed representable bounds). `encode` keeps its + `embedded_io::Write`-based signature. The crate re-exports `Encode` (plus + `Decode`, `DecodeIter`, `DecodeIterator`, `EncodeToSliceError`) from the + crate root. `encode_to_slice` is now a codec-provided default method + returning `EncodeToSliceError` instead of `protocol::Error` directly; the + crate-local `EncodeExt::encode_to_vec` (std-only) extension trait replaces + the old inherent heap-allocating helper. + +- **Decode via `Decode` / `DecodeIter`.** `HeaderView`, `MessageView`, + `EntryView`, and `OptionView` now implement the codec's `Decode` trait, and + service discovery entries/options are exposed as lazy `DecodeIterator`s + (backed by a new lazy `SdBody`) instead of eagerly-materialized + collections. `MessageView::decode` returns `(value, rest)`, recovering any + trailing bytes (e.g. the next message in a multi-message datagram); the new + `decode_exact` is the strict form that errors on trailing bytes instead of + discarding them. `HeaderView::parse` / `SdHeaderView::parse` remain as thin + wrappers over the `Decode` impls for source compatibility. + +- **`protocol::Error` reworked.** `UnexpectedEof` is gone. New variants: + `Incomplete { needed, available }` (buffer too short to decode), `Trailing` + (unconsumed bytes after a strict decode), `InsufficientBuffer { needed, + available }` (output buffer too small to encode into), and + `InvalidLength(u32)` (SOME/IP `length` field below the 8-byte minimum). + `sd::Error::IncorrectOptionsSize` is now a struct variant `{ needed, + available }` instead of a tuple/unit variant. + +- **`sd_codec` parsers now return `Result`, not `Option`.** + `parse_someip_datagram` and `parse_someip_sd_datagram` surface + `protocol::Error` (distinguishing "too few bytes" / "not an SD message" / + "malformed SD payload" instead of collapsing all three into `None`). + `BuildError::BufferTooSmall` now carries the codec's `InsufficientBuffer` + (`needed` / `available`) instead of being a unit variant. + +- **`PayloadWireFormat` gains an `Encode` supertrait.** The inherent + `required_size` / `encode` methods are gone — implementors get them from + `Encode` instead. `from_payload_bytes` is unchanged (payloads aren't + self-identifying, so decoding still requires the caller to supply the + `MessageId`). + +- **`ReadBytesExt` removed.** Decode is now fully slice-based + (`Decode`/`DecodeIter` via `take`/`ensure_len`), leaving the + `embedded_io::Read`-backed `ReadBytesExt` trait with no remaining callers. + It has been deleted along with its blanket impl and unit tests. + `WriteBytesExt` is unaffected and remains available. + +- **New dependency:** `automotive-wire-codec = "0.3"`. + +- **Robustness fix:** SOME/IP datagrams with `length < 8` are now rejected + with `Error::InvalidLength` instead of risking an arithmetic-underflow + panic when computing `payload_size` (`length - 8`). + +- **`Options::write` removed.** It was a `pub` inherent method on + `sd::Options`; the equivalent is now the `Encode::encode` trait method, so + callers need `use automotive_wire_codec::Encode` (or the crate-root + re-export) in scope. Nothing else changes — the bytes written are identical. + +- **`ServiceEntry::required_size()` / `EventGroupEntry::required_size()` + returned 16 but wrote 15 bytes.** Their replacement `encoded_size()` + returns `Ok(15)`, which is what `encode` actually writes. The 16 belongs to + the enclosing `Entry` (1 type byte + 15 body bytes = `ENTRY_SIZE`), and + these two body types had inherited it. Anyone who sized a buffer from + `required_size()` on these types gets a different number now; it is the + correct one, and this is a latent-bug fix rather than a rename. + +- **`Message::new_sd` returns `Result`.** It previously + swallowed a failed `SdHeader::encoded_size()` with `unwrap_or(0)`, building + a header that declared the bare 8-byte SD length while `encode` went on to + write the full payload. Receivers truncate at the declared length, so the + failure mode was silent wire corruption rather than an error. No in-tree SD + header can fail — `sd::Header::encoded_size` is unconditionally `Ok` — so + in-tree callers only gain a `?` or an `expect`. + +- **`PayloadWireFormat::SdHeader` is bounded `Encode`.** This matches the bound the trait already places on + `Self` and is what makes the error above nameable and therefore + propagatable. Every concrete `SdHeader` already used `protocol::Error`, so + in-tree this is a no-op; a downstream implementor with a different error + type must change it. + +- **`BuildError::ListFull { needed, capacity }` added.** A fixed-capacity + entry/option list overflow used to be reported as + `BufferTooSmall(InsufficientBuffer { .. })`, whose fields are documented in + *bytes* — it was putting element counts in them. The path is unreachable + (`take(N)` bounds the loop), but the error would have been actively + misleading if it ever fired. Exhaustive matches on `BuildError` need a new + arm. + +- **Under-length SD options are rejected instead of panicking.** + `OptionView::decode` required a 4-byte option header but then took + `length + 3` bytes, so a declared `length` below 1 produced a view shorter + than the header it had just insisted on, and the accessors indexed it + unconditionally: option bytes `00 02 04 00 00` panicked in `as_ipv4` via + `to_owned`, and a zero-length Configuration option panicked in + `configuration_bytes`. `decode` now rejects a wire size below the option + header, and `as_ipv4` / `as_ipv6` / `as_load_balancing` check their own span + before indexing, returning `IncorrectOptionsSize`. The crate's own paths + went through `SdHeaderView::parse`, whose eager `validate()` walk already + rejected these, so this was not reachable via `parse_someip_sd_datagram` — + but the lazy `SdBody` / `Decode` surface the docs point at is public. + ## [0.12.0] Contains a breaking change to the public error enums, so this release takes the diff --git a/Cargo.lock b/Cargo.lock index 601d404..4502e7f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -44,6 +44,15 @@ dependencies = [ "stable_deref_trait", ] +[[package]] +name = "automotive-wire-codec" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3619f2db27c70d93c5390ce2e90482c26c99675a85392271d2bfe07451234f6" +dependencies = [ + "embedded-io 0.7.1", +] + [[package]] name = "bare_metal_client" version = "0.0.0" @@ -633,8 +642,9 @@ dependencies = [ [[package]] name = "simple-someip" -version = "0.12.0" +version = "0.13.0" dependencies = [ + "automotive-wire-codec", "crc", "critical-section", "embassy-executor", diff --git a/Cargo.toml b/Cargo.toml index 3105fc3..a04f266 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -24,7 +24,7 @@ exclude = ["tools/size_probe"] [package] name = "simple-someip" -version = "0.12.0" +version = "0.13.0" edition = "2024" license = "MIT OR Apache-2.0" description = "A lightweight SOME/IP serialization and communication library" @@ -48,6 +48,9 @@ embassy-sync = { version = "0.6", optional = true } embassy-executor = { version = "0.6", default-features = false, features = [ "nightly", ], optional = true } +# L0 shared wire-codec traits (Decode/DecodeIter/Encode) and big-endian leaf +# helpers. `no_std`, no-alloc; shares `embedded-io = "0.7"` with this crate. +automotive-wire-codec = "0.3" embedded-io = { version = "0.7" } # `futures` pulls in `futures-util` which provides the executor-agnostic # `select!` macro and `FutureExt::fuse` / `pin_mut!` helpers — used by diff --git a/LICENSE-APACHE b/LICENSE-APACHE new file mode 100644 index 0000000..1b5ec8b --- /dev/null +++ b/LICENSE-APACHE @@ -0,0 +1,176 @@ + Apache License + Version 2.0, January 2004 + http://www.apache.org/licenses/ + +TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + +1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright owner + or by an individual or Legal Entity authorized to submit on behalf of + the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + +2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + +3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + (except as stated in this section) patent license to make, have made, + use, offer to sell, sell, import, and otherwise transfer the Work, + where such license applies only to those patent claims licensable + by such Contributor that are necessarily infringed by their + Contribution(s) alone or by combination of their Contribution(s) + with the Work to which such Contribution(s) was submitted. If You + institute patent litigation against any entity (including a + cross-claim or counterclaim in a lawsuit) alleging that the Work + or a Contribution incorporated within the Work constitutes direct + or contributory patent infringement, then any patent licenses + granted to You under this License for that Work shall terminate + as of the date such litigation is filed. + +4. Redistribution. You may reproduce and distribute copies of the + Work or Derivative Works thereof in any medium, with or without + modifications, and in Source or Object form, provided that You + meet the following conditions: + + (a) You must give any other recipients of the Work or + Derivative Works a copy of this License; and + + (b) You must cause any modified files to carry prominent notices + stating that You changed the files; and + + (c) You must retain, in the Source form of any Derivative Works + that You distribute, all copyright, patent, trademark, and + attribution notices from the Source form of the Work, + excluding those notices that do not pertain to any part of + the Derivative Works; and + + (d) If the Work includes a "NOTICE" text file as part of its + distribution, then any Derivative Works that You distribute must + include a readable copy of the attribution notices contained + within such NOTICE file, excluding those notices that do not + pertain to any part of the Derivative Works, in at least one + of the following places: within a NOTICE text file distributed + as part of the Derivative Works; within the Source form or + documentation, if provided along with the Derivative Works; or, + within a display generated by the Derivative Works, if and + wherever such third-party notices normally appear. The contents + of the NOTICE file are for informational purposes only and + do not modify the License. You may add Your own attribution + notices within Derivative Works that You distribute, alongside + or as an addendum to the NOTICE text from the Work, provided + that such additional attribution notices cannot be construed + as modifying the License. + + You may add Your own copyright statement to Your modifications and + may provide additional or different license terms and conditions + for use, reproduction, or distribution of Your modifications, or + for any such Derivative Works as a whole, provided Your use, + reproduction, and distribution of the Work otherwise complies with + the conditions stated in this License. + +5. Submission of Contributions. Unless You explicitly state otherwise, + any Contribution intentionally submitted for inclusion in the Work + by You to the Licensor shall be under the terms and conditions of + this License, without any additional terms or conditions. + Notwithstanding the above, nothing herein shall supersede or modify + the terms of any separate license agreement you may have executed + with Licensor regarding such Contributions. + +6. Trademarks. This License does not grant permission to use the trade + names, trademarks, service marks, or product names of the Licensor, + except as required for reasonable and customary use in describing the + origin of the Work and reproducing the content of the NOTICE file. + +7. Disclaimer of Warranty. Unless required by applicable law or + agreed to in writing, Licensor provides the Work (and each + Contributor provides its Contributions) on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + implied, including, without limitation, any warranties or conditions + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A + PARTICULAR PURPOSE. You are solely responsible for determining the + appropriateness of using or redistributing the Work and assume any + risks associated with Your exercise of permissions under this License. + +8. Limitation of Liability. In no event and under no legal theory, + whether in tort (including negligence), contract, or otherwise, + unless required by applicable law (such as deliberate and grossly + negligent acts) or agreed to in writing, shall any Contributor be + liable to You for damages, including any direct, indirect, special, + incidental, or consequential damages of any character arising as a + result of this License or out of the use or inability to use the + Work (including but not limited to damages for loss of goodwill, + work stoppage, computer failure or malfunction, or any and all + other commercial damages or losses), even if such Contributor + has been advised of the possibility of such damages. + +9. Accepting Warranty or Additional Liability. While redistributing + the Work or Derivative Works thereof, You may choose to offer, + and charge a fee for, acceptance of support, warranty, indemnity, + or other liability obligations and/or rights consistent with this + License. However, in accepting such obligations, You may act only + on Your own behalf and on Your sole responsibility, not on behalf + of any other Contributor, and only if You agree to indemnify, + defend, and hold each Contributor harmless for any liability + incurred by, or claims asserted against, such Contributor by reason + of your accepting any such warranty or additional liability. + +END OF TERMS AND CONDITIONS diff --git a/LICENSE-MIT b/LICENSE-MIT new file mode 100644 index 0000000..31aa793 --- /dev/null +++ b/LICENSE-MIT @@ -0,0 +1,23 @@ +Permission is hereby granted, free of charge, to any +person obtaining a copy of this software and associated +documentation files (the "Software"), to deal in the +Software without restriction, including without +limitation the rights to use, copy, modify, merge, +publish, distribute, sublicense, and/or sell copies of +the Software, and to permit persons to whom the Software +is furnished to do so, subject to the following +conditions: + +The above copyright notice and this permission notice +shall be included in all copies or substantial portions +of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF +ANY KIND, EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED +TO THE WARRANTIES OF MERCHANTABILITY, FITNESS FOR A +PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT +SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY +CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION +OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR +IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +DEALINGS IN THE SOFTWARE. diff --git a/README.md b/README.md index e5c2a43..cf0221b 100644 --- a/README.md +++ b/README.md @@ -20,7 +20,7 @@ The library supports both `std` and `no_std` environments, making it suitable fo ## Modules - `protocol` — Wire format layer: SOME/IP header, `MessageId`, `MessageType`, `ReturnCode`, SD entries/options -- `traits` — `WireFormat` and `PayloadWireFormat` traits for custom message types +- `traits` — `PayloadWireFormat` trait (built on `automotive_wire_codec::Encode`) for custom message types - `transport` — Executor-agnostic UDP socket / factory / timer / spawner traits (no_std-compatible) - `e2e` — End-to-End protection profiles (always available, no heap allocation) - `tokio_transport` — Default `std + tokio` impls of the transport traits (requires `feature = "client-tokio"` or `feature = "server-tokio"`) diff --git a/simple-someip-embassy-net/Cargo.toml b/simple-someip-embassy-net/Cargo.toml index 7cacaae..faf31a6 100644 --- a/simple-someip-embassy-net/Cargo.toml +++ b/simple-someip-embassy-net/Cargo.toml @@ -19,7 +19,7 @@ readme = "README.md" # [dependencies] -simple-someip = { path = "..", version = "0.12", default-features = false, features = [ +simple-someip = { path = "..", version = "0.13", default-features = false, features = [ "client", "server", "bare_metal", diff --git a/src/bare_metal_tasks.rs b/src/bare_metal_tasks.rs index fbce370..a0c2696 100644 --- a/src/bare_metal_tasks.rs +++ b/src/bare_metal_tasks.rs @@ -128,7 +128,7 @@ pub async fn event_rx_dispatch_future<'a, S, R>( Ok(d) => (d.bytes_received, d.source), Err(_) => continue, }; - let Some(parsed) = parse_someip_datagram(&buf[..n]) else { + let Ok(parsed) = parse_someip_datagram(&buf[..n]) else { continue; }; let (status, body) = if e2e_enabled { diff --git a/src/client/inner.rs b/src/client/inner.rs index 5520158..7deebd6 100644 --- a/src/client/inner.rs +++ b/src/client/inner.rs @@ -863,19 +863,34 @@ where } } Some(discovery_socket) => { - let message = Message::::new_sd( + // Sizing the SD header is fallible for a + // downstream `PayloadWireFormat`. Report it rather + // than sending a header whose declared length + // disagrees with the payload that follows it. + match Message::::new_sd( u32::from(discovery_socket.session_id()), &header, - ); - debug!("Sending {:?} to {}", &message, target); - let send_result = self - .discovery_socket - .as_mut() - .unwrap() - .send(target, message) - .await; - if response.send(send_result).is_err() { - debug!("SendSD: caller dropped the response receiver"); + ) { + Ok(message) => { + debug!("Sending {:?} to {}", &message, target); + let send_result = self + .discovery_socket + .as_mut() + .unwrap() + .send(target, message) + .await; + if response.send(send_result).is_err() { + debug!("SendSD: caller dropped the response receiver"); + } + } + Err(e) => { + debug!("SendSD: sizing the SD header failed: {e}"); + if response.send(Err(e.into())).is_err() { + debug!( + "SendSD (size-err path): caller dropped the response receiver" + ); + } + } } } } @@ -1115,21 +1130,34 @@ where discovery_socket.reboot_flag(), ); let session_id = u32::from(discovery_socket.session_id()); - let message = - Message::::new_sd(session_id, &sd_header); let target = SocketAddrV4::new(*provider.ip(), protocol::sd::MULTICAST_PORT); - debug!("Sending Subscribe {:?} to {}", &message, target); - let send_result = self - .discovery_socket - .as_mut() - .unwrap() - .send(target, message) - .await; - if response.send(send_result).is_err() { - debug!( - "Subscribe: caller dropped the response receiver (expected for subscribe_no_wait)" - ); + // See the SendSD arm: a downstream SD header can + // fail to size, and a mis-declared length is worse + // on the wire than a reported error. + match Message::::new_sd(session_id, &sd_header) { + Ok(message) => { + debug!("Sending Subscribe {:?} to {}", &message, target); + let send_result = self + .discovery_socket + .as_mut() + .unwrap() + .send(target, message) + .await; + if response.send(send_result).is_err() { + debug!( + "Subscribe: caller dropped the response receiver (expected for subscribe_no_wait)" + ); + } + } + Err(e) => { + debug!("Subscribe: sizing the SD header failed: {e}"); + if response.send(Err(e.into())).is_err() { + debug!( + "Subscribe (size-err path): caller dropped the response receiver" + ); + } + } } } } @@ -1368,7 +1396,8 @@ mod tests { let (_rx, msg) = TestControl::remove_endpoint(lh_key(0x1234, 5000)); assert!(matches!(msg, ControlMessage::RemoveEndpoint(..))); - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (_send_rx, _resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); assert!(matches!(msg, ControlMessage::SendToService { .. })); @@ -1437,7 +1466,8 @@ mod tests { // SendToService carries two senders — both must be notified so that // neither `send_rx.recv().await.unwrap()?` nor `PendingResponse::response()` // panics. - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (send_rx, resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); msg.reject_with_capacity(CapacityKind::RequestQueue); expect_capacity(send_rx.recv(), "SendToService.send_complete"); @@ -1482,7 +1512,8 @@ mod tests { let s = format!("{msg:?}"); assert!(s.contains("RemoveEndpoint")); - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (_send_rx, _resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); let s = format!("{msg:?}"); assert!(s.contains("SendToService")); @@ -2007,7 +2038,8 @@ mod tests { #[tokio::test] async fn test_send_to_service_constructor_returns_two_receivers() { - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (send_rx, resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); // Extract the senders from the control message @@ -2108,7 +2140,8 @@ mod tests { rx.recv().await.unwrap().unwrap(); // Send SendToService with the send_complete receiver dropped - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (send_rx, _resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); drop(send_rx); control_sender.send(msg).await.unwrap(); @@ -2217,7 +2250,8 @@ mod tests { control_sender.send(msg).await.unwrap(); rx.recv().await.unwrap().unwrap(); - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (send_rx, _resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); control_sender.send(msg).await.unwrap(); let result = tokio::time::timeout(std::time::Duration::from_secs(2), send_rx.recv()) @@ -2354,13 +2388,15 @@ mod tests { rx.recv().await.unwrap().unwrap(); // First send auto-binds unicast - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (send_rx, _resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); control_sender.send(msg).await.unwrap(); send_rx.recv().await.unwrap().unwrap(); // Second send reuses the existing socket (no auto-bind needed) - let message = Message::::new_sd(1, &empty_sd_header()); + let message = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (send_rx, _resp_rx, msg) = TestControl::send_to_service(lh_key(0x1234, 5000), message); control_sender.send(msg).await.unwrap(); let result = tokio::time::timeout(std::time::Duration::from_secs(2), send_rx.recv()) @@ -2607,9 +2643,9 @@ mod tests { #[test] #[allow(clippy::too_many_lines)] fn handle_discovery_datagram_keys_offers_by_device_ip() { + use crate::Encode; use crate::RawPayload; use crate::protocol::sd::{self, Entry, Options, OptionsCount, ServiceEntry}; - use crate::traits::WireFormat; use core::net::{Ipv4Addr, SocketAddr, SocketAddrV4}; type RawInner = Inner< @@ -2675,7 +2711,10 @@ mod tests { [(1u32, DEVICE_A, addr_a), (2u32, DEVICE_B, addr_b)] { let sd_header = offer_header(service_addr, true); - let someip_header = protocol::Header::new_sd(request_id, sd_header.required_size()); + let someip_header = protocol::Header::new_sd( + request_id, + sd_header.encoded_size().expect("encoded_size"), + ); RawInner::handle_discovery_datagram( SocketAddr::new(source_ip.into(), sd::MULTICAST_PORT), TransportKind::Multicast, @@ -2706,7 +2745,8 @@ mod tests { // ad515c3, the registry was keyed by (service_id, instance_id) // alone, so removing A's entry would have removed B's too. let stop_header = offer_header(addr_a, false); - let someip_header = protocol::Header::new_sd(3, stop_header.required_size()); + let someip_header = + protocol::Header::new_sd(3, stop_header.encoded_size().expect("encoded_size")); RawInner::handle_discovery_datagram( SocketAddr::new(DEVICE_A.into(), sd::MULTICAST_PORT), TransportKind::Multicast, diff --git a/src/client/mod.rs b/src/client/mod.rs index 779d8f3..74a06b6 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -1441,7 +1441,7 @@ where mod tests { use super::*; use crate::protocol::sd::test_support::{TestPayload, empty_sd_header}; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; use std::format; type TestClient = @@ -1471,7 +1471,7 @@ mod tests { // DiscoveryUpdated let sd_header = empty_sd_header(); - let someip_header = crate::protocol::Header::new_sd(1, sd_header.required_size()); + let someip_header = crate::protocol::Header::new_sd(1, sd_header.encoded_size().unwrap()); let discovery_msg = DiscoveryMessage { source: SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 30490), someip_header, @@ -1488,7 +1488,8 @@ mod tests { assert!(debug_str.contains("SenderRebooted")); // Unicast - let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); + let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let update: ClientUpdate = ClientUpdate::Unicast { message: msg, e2e_status: None, @@ -1506,7 +1507,8 @@ mod tests { #[test] fn unicast_update_carries_source() { let src = SocketAddr::new(Ipv4Addr::new(192, 168, 11, 101).into(), 30640); - let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); + let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let update: ClientUpdate = ClientUpdate::Unicast { message: msg, e2e_status: None, @@ -1598,7 +1600,8 @@ mod tests { } // Inner loop must still be responsive after the stress. - let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); + let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let result = tokio::time::timeout( std::time::Duration::from_secs(2), client.request( @@ -1657,7 +1660,8 @@ mod tests { async fn test_send_to_service_unknown_returns_error() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); let _run_handle = tokio::spawn(run_fut); - let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); + let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let result = client .send_to_service( ServiceEndpointKey::udp( @@ -1752,7 +1756,8 @@ mod tests { ) .await .unwrap(); - let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); + let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); // send_to_service succeeds (send completes), returning a PendingResponse let pending = client .send_to_service(ServiceEndpointKey::udp(0x1234, SocketAddr::V4(addr)), msg) @@ -1809,7 +1814,8 @@ mod tests { async fn test_request_unknown_service_returns_error() { let (client, _updates, run_fut) = TestClient::new(Ipv4Addr::LOCALHOST); let _run_handle = tokio::spawn(run_fut); - let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()); + let msg = crate::protocol::Message::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let result = client .request( ServiceEndpointKey::udp( diff --git a/src/client/socket_manager.rs b/src/client/socket_manager.rs index 92b2a4d..4943c2c 100644 --- a/src/client/socket_manager.rs +++ b/src/client/socket_manager.rs @@ -46,7 +46,7 @@ use crate::{ buffer_pool::BufferLease, e2e::{E2ECheckStatus, E2EKey}, protocol::{Message, MessageView, sd}, - traits::{PayloadWireFormat, WireFormat}, + traits::PayloadWireFormat, transport::{ ChannelFactory, E2ERegistryHandle, LocalSpawner, MpscRecv, MpscSend, OneshotRecv, OneshotSend, ReceivedDatagram, SocketOptions, Spawner, TransportFactory, TransportSocket, @@ -55,6 +55,7 @@ use crate::{ use super::error::Error; use crate::log::{debug, error, info, trace, warn}; +use automotive_wire_codec::Encode; use core::{ net::{Ipv4Addr, SocketAddr, SocketAddrV4}, task::{Context, Poll}, @@ -557,7 +558,10 @@ where // overload signal regardless of which path produced the oversize // message. Without this, an oversize encode would surface as a // protocol-level I/O error from inside the socket loop. - let required = message.required_size(); + // Propagated, not unwrapped: `encoded_size` is fallible for any + // downstream `PayloadWireFormat`, and this runs in the client's async + // socket task. `server::EventPublisher` already uses `?` here. + let required = message.encoded_size()?; // Coarse fail-fast: `send()` has no leased buffer in scope, so // UDP_BUFFER_SIZE is the only bound available here. The socket // loop's `buf.len()` check is the authoritative guard; E2E @@ -727,7 +731,17 @@ where // a caller-sized bare-metal pool may hand out a buffer // smaller than `UDP_BUFFER_SIZE`, and the message must fit // the buffer we actually encode into. - let required = send_message.message.required_size(); + // Same fallibility as the `send` path above, but this + // is the loop: report to the waiting caller and carry on + // rather than panicking the socket task. + let required = match send_message.message.encoded_size() { + Ok(required) => required, + Err(e) => { + warn!("outgoing message could not be sized: {e}"); + let _ = send_message.response.send(Err(e.into())); + continue; + } + }; if required > buf.len() { warn!( "outgoing message size {required} exceeds claimed buffer ({}); rejecting with Capacity(\"udp_buffer\")", @@ -988,6 +1002,30 @@ mod tests { TokioBufferProvider::new().claim().expect("fresh pool slot") } + /// A payload whose `encoded_size` fails must surface as an error, not + /// panic the caller. `.expect()` here was safe only for the two in-tree + /// payloads; `PayloadWireFormat` is public. The server side already does + /// this correctly with `?` in `event_publisher`. (PR #153 review.) + #[tokio::test] + async fn send_surfaces_a_failing_encoded_size_instead_of_panicking() { + use crate::protocol::sd::test_support::{FailingPayload, FailingSdHeader}; + + let mut sm = SocketManager::::bind(0, test_registry()) + .await + .expect("bind ephemeral"); + let message = Message::new( + crate::protocol::Header::new_sd(1, 0), + FailingPayload { + header: FailingSdHeader, + }, + ); + let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 30490); + assert!( + sm.send(target, message).await.is_err(), + "a failing encoded_size must be reported, not panicked on", + ); + } + async fn bind_ephemeral_spawned() -> TestSocketManager { TestSocketManager::bind(0, test_registry()).await.unwrap() } @@ -1136,7 +1174,8 @@ mod tests { async fn test_send_message_new() { use crate::transport::OneshotRecv; let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1234); - let msg = Message::new_sd(1, &empty_sd_header()); + let msg = + Message::new_sd(1, &empty_sd_header()).expect("in-tree SdHeader sizing is infallible"); let (rx, send_msg) = SendMessage::::new(target, msg); assert_eq!(send_msg.target_addr, target); // Verify the oneshot channel works @@ -1159,7 +1198,8 @@ mod tests { let raw_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); // Build and encode an SD message - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let mut buf = vec![0u8; 128]; let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); @@ -1189,7 +1229,8 @@ mod tests { // Send a message to the socket manager from a raw socket let raw_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let mut buf = vec![0u8; 128]; let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); raw_socket @@ -1221,19 +1262,22 @@ mod tests { let raw_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let raw_port = raw_socket.local_addr().unwrap().port(); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, raw_port); - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); sm.send(target, msg).await.unwrap(); assert_eq!(sm.session_id(), 2); // Second send increments session - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); sm.send(target, msg).await.unwrap(); assert_eq!(sm.session_id(), 3); } #[tokio::test] async fn test_received_message_debug() { - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let received = ReceivedMessage { message: msg, source: SocketAddr::new(Ipv4Addr::LOCALHOST.into(), 5000), @@ -1246,7 +1290,8 @@ mod tests { #[tokio::test] async fn test_send_message_debug() { let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, 1234); - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let (_rx, send_msg) = SendMessage::::new(target, msg); let s = format!("{send_msg:?}"); assert!(s.contains("SendMessage")); @@ -1268,7 +1313,8 @@ mod tests { let raw_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let raw_port = raw_socket.local_addr().unwrap().port(); - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, raw_port); sm.send(target, msg.clone()).await.unwrap(); @@ -1313,7 +1359,10 @@ mod tests { let raw_socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let target = SocketAddrV4::new(Ipv4Addr::LOCALHOST, raw_socket.local_addr().unwrap().port()); - let msg = || Message::::new_sd(1, &empty_sd_header()); + let msg = || { + Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible") + }; // Set session_id to one before the wrap point sm.session_id = u16::MAX - 1; @@ -1470,7 +1519,8 @@ mod tests { let recv = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let recv_port = recv.local_addr().unwrap().port(); - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); sm.send(SocketAddrV4::new(Ipv4Addr::LOCALHOST, recv_port), msg) .await .expect("send_to via custom-factory-built socket"); @@ -1586,7 +1636,8 @@ mod tests { let recv = UdpSocket::bind("127.0.0.1:0").await.unwrap(); let recv_port = recv.local_addr().unwrap().port(); - let msg = Message::::new_sd(1, &empty_sd_header()); + let msg = Message::::new_sd(1, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); sm.send(SocketAddrV4::new(Ipv4Addr::LOCALHOST, recv_port), msg) .await .expect("send via wrapping factory"); diff --git a/src/e2e/crc.rs b/src/e2e/crc.rs index a72854a..1c19fd0 100644 --- a/src/e2e/crc.rs +++ b/src/e2e/crc.rs @@ -1,5 +1,21 @@ //! CRC computation helpers for E2E profiles. - +//! +//! # Little-endian `DataID`/CRC framing (Profile 5) — intentional, do not "fix" +//! +//! Profile 5's CRC input and on-wire CRC field use **little-endian** byte +//! order for `DataID` and the CRC value itself (see `data_id.to_le_bytes()` +//! and `crc.to_le_bytes()` below), whereas the rest of this crate — SOME/IP +//! headers, Profile 4, and every `Encode`/`Decode` impl built on +//! `automotive-wire-codec` — is big-endian. This is not an inconsistency to +//! reconcile: it is spec-correct per E2E Profile 5, which defines its CRC +//! and header fields as little-endian. +//! +//! `automotive-wire-codec` 0.3 only ships big-endian leaf helpers (`u16`/`u32` +//! big-endian encode/decode), so E2E cannot express this LE framing through +//! the codec's `Encode`/`Decode` traits even if it otherwise migrated onto +//! them. This gap has been recorded as codec feedback (tracked as F2) rather +//! than worked around here — do **not** introduce ad hoc LE codec helpers in +//! this crate; the LE byte order below must stay exactly as written. use crc::{CRC_16_IBM_3740, CRC_32_AUTOSAR, Crc}; /// CRC-32P4 algorithm used by E2E Profile 4. @@ -57,7 +73,8 @@ pub fn compute_crc16_p5(data_id: u16, counter: u8, payload: &[u8]) -> u16 { // Payload digest.update(payload); - // DataID (little-endian) + // DataID (little-endian per E2E Profile 5 — see module-level doc comment; + // intentional, not a bug, and not to be "corrected" to big-endian). let data_id_bytes = data_id.to_le_bytes(); digest.update(&data_id_bytes); @@ -96,6 +113,8 @@ pub fn compute_crc16_p5_with_header( digest.update(&upper_header); digest.update(&[counter]); digest.update(payload); + // DataID (little-endian per E2E Profile 5 — see module-level doc comment; + // intentional, not a bug, and not to be "corrected" to big-endian). digest.update(&data_id.to_le_bytes()); let crc = digest.finalize(); diff --git a/src/e2e/e2e_checker.rs b/src/e2e/e2e_checker.rs index 1921273..95af09f 100644 --- a/src/e2e/e2e_checker.rs +++ b/src/e2e/e2e_checker.rs @@ -102,6 +102,7 @@ pub fn check_profile5<'a>( } // Parse header: CRC (2, little-endian) + Counter (1) + // CRC field is little-endian per E2E Profile 5 (see `crc` module doc); intentional. let received_crc = u16::from_le_bytes([protected[0], protected[1]]); let counter = protected[2]; @@ -164,6 +165,7 @@ pub fn check_profile5_with_header<'a>( return E2ECheckResult::error(E2ECheckStatus::BadArgument); } + // CRC field is little-endian per E2E Profile 5 (see `crc` module doc); intentional. let received_crc = u16::from_le_bytes([protected[0], protected[1]]); let counter = protected[2]; let payload = &protected[PROFILE5_HEADER_SIZE..]; diff --git a/src/e2e/e2e_protector.rs b/src/e2e/e2e_protector.rs index 9a3d48d..ebd84b1 100644 --- a/src/e2e/e2e_protector.rs +++ b/src/e2e/e2e_protector.rs @@ -121,7 +121,8 @@ pub fn protect_profile5( // Compute CRC over: Counter + Payload + DataID (LE) let crc = compute_crc16_p5(config.data_id, counter, payload); - // Header: CRC (2, little-endian) + Counter (1) + // Header: CRC (2, little-endian per E2E Profile 5 — see `crc` module doc; + // intentional, do not change to big-endian) + Counter (1) output[0..2].copy_from_slice(&crc.to_le_bytes()); output[2] = counter; @@ -178,7 +179,8 @@ pub fn protect_profile5_with_header( let counter = state.protect_counter; let crc = compute_crc16_p5_with_header(config.data_id, counter, payload, upper_header); - // Header: CRC (2, little-endian) + Counter (1) + // Header: CRC (2, little-endian per E2E Profile 5 — see `crc` module doc; + // intentional, do not change to big-endian) + Counter (1) output[0..2].copy_from_slice(&crc.to_le_bytes()); output[2] = counter; diff --git a/src/e2e/mod.rs b/src/e2e/mod.rs index 7196a3f..0e28d07 100644 --- a/src/e2e/mod.rs +++ b/src/e2e/mod.rs @@ -3,6 +3,35 @@ //! This module implements E2E Profile 4 and Profile 5 protection as specified //! in the [Open SOME/IP Specification](https://github.com/some-ip-com/open-someip-spec). //! +//! # Why E2E does not implement `Encode`/`Decode` +//! +//! Unlike the rest of the wire path (headers, SD entries/options, payloads), +//! E2E deliberately stays on its own `protect`/`check` API instead of +//! `automotive_wire_codec::{Encode, Decode}`. Two structural mismatches drive +//! this, and both are intentional — not gaps to be closed later: +//! +//! - **In-place mutation, not a fresh encode.** `protect` writes a header in +//! front of an already-serialized payload and, at the call site (see +//! `server::event_publisher::EventPublisher::publish_event`), the +//! surrounding SOME/IP length field gets rewritten *after* protection +//! because the final length depends on protect's output size. `Encode` is +//! a single forward pass; it has no place to express "go back and patch +//! bytes already written based on bytes written later." The codec's own +//! README scopes this kind of size-changing, post-hoc transform out of +//! `Encode` and points to a two-phase, consumer-owned API for it — which +//! is exactly what `protect`/`check` are. +//! - **Status results, not `Result<_, Error>`.** `check_profile4`/`check_profile5` +//! return an [`E2ECheckResult`](crate::e2e::E2ECheckResult) carrying an [`E2ECheckStatus`] (`Ok`, +//! `CrcError`, `Repeated`, `WrongSequence`, `OkSomeLost`, `BadArgument`, +//! `Unchecked`) rather than an error. Several of those statuses (e.g. +//! `OkSomeLost`) are still *successful* checks that also carry diagnostic +//! information — that doesn't fit `Decode`'s binary success/error split. +//! +//! [`crate::e2e::Error`] (used only by `protect`'s buffer-sizing failure) is +//! bridged onto [`crate::protocol::Error`] via `impl From for +//! protocol::Error` (see `protocol::error`) so callers that want one error +//! type can still get it, without forcing E2E itself onto the codec traits. +//! //! # Example //! //! ``` diff --git a/src/heapless_payload.rs b/src/heapless_payload.rs index ef493a0..455d1d2 100644 --- a/src/heapless_payload.rs +++ b/src/heapless_payload.rs @@ -31,7 +31,8 @@ use embedded_io::Error as _; use heapless::Vec as HVec; use crate::protocol::{self, MessageId, sd}; -use crate::traits::{PayloadWireFormat, WireFormat}; +use crate::traits::PayloadWireFormat; +use automotive_wire_codec::Encode; /// Max SD entries in a single payload. See module-level docs. pub const ENTRY_CAP: usize = 8; @@ -54,12 +55,14 @@ pub struct HeaplessSdHeader { pub options: HVec, } -impl WireFormat for HeaplessSdHeader { - fn required_size(&self) -> usize { - sd::Header::new(self.flags, &self.entries, &self.options).required_size() +impl Encode for HeaplessSdHeader { + type Error = protocol::Error; + + fn encoded_size(&self) -> Result { + sd::Header::new(self.flags, &self.entries, &self.options).encoded_size() } - fn encode(&self, writer: &mut T) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { sd::Header::new(self.flags, &self.entries, &self.options).encode(writer) } } @@ -100,6 +103,29 @@ impl HeaplessPayload { } } +impl Encode for HeaplessPayload { + type Error = protocol::Error; + + fn encoded_size(&self) -> Result { + match &self.kind { + HeaplessPayloadKind::Sd(header) => header.encoded_size(), + HeaplessPayloadKind::Raw(bytes) => Ok(bytes.len()), + } + } + + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + match &self.kind { + HeaplessPayloadKind::Sd(header) => header.encode(writer), + HeaplessPayloadKind::Raw(bytes) => { + writer + .write_all(bytes) + .map_err(|e| protocol::Error::Io(e.kind()))?; + Ok(bytes.len()) + } + } + } +} + impl PayloadWireFormat for HeaplessPayload { type SdHeader = HeaplessSdHeader; @@ -165,25 +191,6 @@ impl PayloadWireFormat for HeaplessPayload { } } - fn required_size(&self) -> usize { - match &self.kind { - HeaplessPayloadKind::Sd(header) => header.required_size(), - HeaplessPayloadKind::Raw(bytes) => bytes.len(), - } - } - - fn encode(&self, writer: &mut T) -> Result { - match &self.kind { - HeaplessPayloadKind::Sd(header) => header.encode(writer), - HeaplessPayloadKind::Raw(bytes) => { - writer - .write_all(bytes) - .map_err(|e| protocol::Error::Io(e.kind()))?; - Ok(bytes.len()) - } - } - } - fn new_subscription_sd_header( service_id: u16, instance_id: u16, diff --git a/src/lib.rs b/src/lib.rs index aaca103..c796842 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -18,7 +18,7 @@ //! |--------|----------|-------------| //! | [`protocol`] | Yes | Wire format: headers, messages, message types, return codes, and service discovery (SD) entries/options | //! | [`e2e`] | Yes | End-to-End protection — Profile 4 (CRC-32) and Profile 5 (CRC-16) | -//! | [`WireFormat`] / [`PayloadWireFormat`] | Yes | Traits for serializing messages and defining custom payload types | +//! | [`Encode`] / [`PayloadWireFormat`] | Yes | Traits for serializing messages and defining custom payload types | //! | `client` | No | Async client trait surface — service discovery, subscriptions, request/response (feature `client`; add `client-tokio` for `Client::new`) | //! | `server` | No | Async server trait surface — service offering, event publishing, subscription management (feature `server`; add `server-tokio` for `Server::new`) | //! @@ -51,7 +51,7 @@ //! ### Encoding a SOME/IP-SD header (`no_std`) //! //! ```rust -//! use simple_someip::WireFormat; +//! use simple_someip::Encode; //! use simple_someip::protocol::sd::{self, Entry, RebootFlag, ServiceEntry}; //! //! // Build an SD header with a FindService entry @@ -257,12 +257,13 @@ mod traits; /// because the target module is feature-gated and would break /// default-feature rustdoc builds. pub mod transport; +pub use automotive_wire_codec::{Decode, DecodeIter, DecodeIterator, Encode, EncodeToSliceError}; #[cfg(feature = "bare_metal")] pub use heapless_payload::{HeaplessPayload, HeaplessSdHeader}; pub use net_endpoint::{NetEndpoint, TransportProtocol}; #[cfg(feature = "std")] pub use raw_payload::{RawPayload, VecSdHeader}; -pub use traits::{OfferedEndpoint, PayloadWireFormat, WireFormat}; +pub use traits::{EncodeExt, OfferedEndpoint, PayloadWireFormat}; #[cfg(feature = "client")] pub use client::{ diff --git a/src/protocol/byte_order.rs b/src/protocol/byte_order.rs index 9c41106..9c7519d 100644 --- a/src/protocol/byte_order.rs +++ b/src/protocol/byte_order.rs @@ -1,155 +1,6 @@ use crate::protocol::Error; use embedded_io::Error as _; -/// Extension trait for reading big-endian values from a byte stream. -/// -/// The only required method is [`read_bytes`](ReadBytesExt::read_bytes). -/// Backed by `embedded_io::Read` via a blanket impl. -pub trait ReadBytesExt { - /// Read exactly `buf.len()` bytes from the stream. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_bytes(&mut self, buf: &mut [u8]) -> Result<(), Error>; - - /// Read a single `u8`. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_u8(&mut self) -> Result { - let mut buf = [0u8; 1]; - self.read_bytes(&mut buf)?; - Ok(buf[0]) - } - - /// Read an `i8`. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_i8(&mut self) -> Result { - self.read_u8().map(u8::cast_signed) - } - - /// Read a `u16` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_u16_be(&mut self) -> Result { - let mut buf = [0u8; 2]; - self.read_bytes(&mut buf)?; - Ok(u16::from_be_bytes(buf)) - } - - /// Read an `i16` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_i16_be(&mut self) -> Result { - let mut buf = [0u8; 2]; - self.read_bytes(&mut buf)?; - Ok(i16::from_be_bytes(buf)) - } - - /// Read the next 3 bytes as the lower 3 bytes of a `u32` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_u24_be(&mut self) -> Result { - let mut buf = [0u8; 3]; - self.read_bytes(&mut buf)?; - Ok(u32::from_be_bytes([0, buf[0], buf[1], buf[2]])) - } - - /// Read a `u32` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_u32_be(&mut self) -> Result { - let mut buf = [0u8; 4]; - self.read_bytes(&mut buf)?; - Ok(u32::from_be_bytes(buf)) - } - - /// Read an `i32` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_i32_be(&mut self) -> Result { - let mut buf = [0u8; 4]; - self.read_bytes(&mut buf)?; - Ok(i32::from_be_bytes(buf)) - } - - /// Read a `u64` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_u64_be(&mut self) -> Result { - let mut buf = [0u8; 8]; - self.read_bytes(&mut buf)?; - Ok(u64::from_be_bytes(buf)) - } - - /// Read an `i64` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_i64_be(&mut self) -> Result { - let mut buf = [0u8; 8]; - self.read_bytes(&mut buf)?; - Ok(i64::from_be_bytes(buf)) - } - - /// Read a `u128` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_u128_be(&mut self) -> Result { - let mut buf = [0u8; 16]; - self.read_bytes(&mut buf)?; - Ok(u128::from_be_bytes(buf)) - } - - /// Read an `i128` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_i128_be(&mut self) -> Result { - let mut buf = [0u8; 16]; - self.read_bytes(&mut buf)?; - Ok(i128::from_be_bytes(buf)) - } - - /// Read an `f32` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_f32_be(&mut self) -> Result { - let mut buf = [0u8; 4]; - self.read_bytes(&mut buf)?; - Ok(f32::from_be_bytes(buf)) - } - - /// Read an `f64` in big-endian byte order. - /// - /// # Errors - /// Returns [`Error::Io`] if the underlying reader fails. - fn read_f64_be(&mut self) -> Result { - let mut buf = [0u8; 8]; - self.read_bytes(&mut buf)?; - Ok(f64::from_be_bytes(buf)) - } -} - -impl ReadBytesExt for T { - fn read_bytes(&mut self, buf: &mut [u8]) -> Result<(), Error> { - self.read_exact(buf).map_err(|e| match e { - embedded_io::ReadExactError::UnexpectedEof => Error::Io(embedded_io::ErrorKind::Other), - embedded_io::ReadExactError::Other(e) => Error::Io(e.kind()), - }) - } -} - /// Extension trait for writing big-endian values to a byte stream. /// /// The only required method is [`write_bytes`](WriteBytesExt::write_bytes). @@ -274,8 +125,8 @@ impl WriteBytesExt for T { #[cfg(test)] // Strict float equality is correct here: these tests verify byte-level -// round-tripping of `to_be_bytes` / `read_f*_be`, where the result must -// be bitwise-identical to the input. +// encoding via `to_be_bytes`, where the result must be bitwise-identical +// to the input. #[allow(clippy::float_cmp)] mod tests { use super::*; @@ -296,18 +147,6 @@ mod tests { } } - struct FailingReader; - - impl embedded_io::ErrorType for FailingReader { - type Error = embedded_io::ErrorKind; - } - - impl embedded_io::Read for FailingReader { - fn read(&mut self, _buf: &mut [u8]) -> Result { - Err(embedded_io::ErrorKind::BrokenPipe) - } - } - // --- Error mapping --- #[test] @@ -318,102 +157,6 @@ mod tests { )); } - #[test] - fn read_io_error_maps_to_error_io() { - assert!(matches!( - FailingReader.read_u8(), - Err(Error::Io(embedded_io::ErrorKind::BrokenPipe)) - )); - } - - // --- ReadBytesExt --- - - #[test] - fn read_u8_decodes_correctly() { - let buf: &[u8] = &[0xAB]; - assert_eq!((&mut &*buf).read_u8().unwrap(), 0xAB); - } - - #[test] - fn read_i8_decodes_correctly() { - let buf: &[u8] = &[0xFF]; - assert_eq!((&mut &*buf).read_i8().unwrap(), -1); - } - - #[test] - fn read_u16_be_decodes_correctly() { - let buf: &[u8] = &[0x01, 0x02]; - assert_eq!((&mut &*buf).read_u16_be().unwrap(), 0x0102); - } - - #[test] - fn read_i16_be_decodes_correctly() { - let buf: &[u8] = &[0xFF, 0xFE]; - assert_eq!((&mut &*buf).read_i16_be().unwrap(), -2); - } - - #[test] - fn read_u24_be_decodes_correctly() { - let buf: &[u8] = &[0x01, 0x02, 0x03]; - assert_eq!((&mut &*buf).read_u24_be().unwrap(), 0x0001_0203); - } - - #[test] - fn read_u32_be_decodes_correctly() { - let buf: &[u8] = &[0x01, 0x02, 0x03, 0x04]; - assert_eq!((&mut &*buf).read_u32_be().unwrap(), 0x0102_0304); - } - - #[test] - fn read_i32_be_decodes_correctly() { - let buf: &[u8] = &[0xFF, 0xFF, 0xFF, 0xFE]; - assert_eq!((&mut &*buf).read_i32_be().unwrap(), -2); - } - - #[test] - fn read_u64_be_decodes_correctly() { - let buf: &[u8] = &[0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08]; - assert_eq!((&mut &*buf).read_u64_be().unwrap(), 0x0102_0304_0506_0708); - } - - #[test] - fn read_i64_be_decodes_correctly() { - let buf: &[u8] = &[0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFE]; - assert_eq!((&mut &*buf).read_i64_be().unwrap(), -2); - } - - #[test] - fn read_u128_be_decodes_correctly() { - let buf: &[u8] = &[ - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0x00, 0x01, - ]; - assert_eq!((&mut &*buf).read_u128_be().unwrap(), 1); - } - - #[test] - fn read_i128_be_decodes_correctly() { - let buf: &[u8] = &[ - 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, - 0xFF, 0xFE, - ]; - assert_eq!((&mut &*buf).read_i128_be().unwrap(), -2); - } - - #[test] - fn read_f32_be_decodes_correctly() { - let expected: f32 = 1.0; - let buf = expected.to_be_bytes(); - assert_eq!((&mut buf.as_slice()).read_f32_be().unwrap(), expected); - } - - #[test] - fn read_f64_be_decodes_correctly() { - let expected: f64 = 1.0; - let buf = expected.to_be_bytes(); - assert_eq!((&mut buf.as_slice()).read_f64_be().unwrap(), expected); - } - // --- WriteBytesExt --- #[test] @@ -511,22 +254,4 @@ mod tests { buf.as_mut_slice().write_f64_be(val).unwrap(); assert_eq!(buf, val.to_be_bytes()); } - - // --- Round-trip --- - - #[test] - fn round_trip_f32() { - let val: f32 = core::f32::consts::PI; - let mut buf = [0u8; 4]; - buf.as_mut_slice().write_f32_be(val).unwrap(); - assert_eq!((&mut buf.as_slice()).read_f32_be().unwrap(), val); - } - - #[test] - fn round_trip_f64() { - let val: f64 = core::f64::consts::PI; - let mut buf = [0u8; 8]; - buf.as_mut_slice().write_f64_be(val).unwrap(); - assert_eq!((&mut buf.as_slice()).read_f64_be().unwrap(), val); - } } diff --git a/src/protocol/error.rs b/src/protocol/error.rs index f914442..fe35129 100644 --- a/src/protocol/error.rs +++ b/src/protocol/error.rs @@ -7,9 +7,15 @@ pub enum Error { /// An I/O error occurred while reading or writing bytes. #[error("I/O error: {0:?}")] Io(embedded_io::ErrorKind), - /// The input buffer ended before the expected number of bytes could be read. - #[error("Unexpected end of input")] - UnexpectedEof, + /// Input ended before the expected number of bytes could be read. + #[error("incomplete: need {} bytes, have {}", .0.needed, .0.available)] + Incomplete(#[from] automotive_wire_codec::Incomplete), + /// Bytes remained after a value that should have consumed the whole buffer. + #[error("trailing bytes: {} left over", .0.0)] + Trailing(#[from] automotive_wire_codec::TrailingBytes), + /// An output slice was too small for the bytes an encode needed to write. + #[error("insufficient buffer: need {} bytes, have {}", .0.needed, .0.available)] + InsufficientBuffer(#[from] automotive_wire_codec::InsufficientBuffer), /// The protocol version field contains an unsupported value. #[error("Invalid protocol version: {0:X}")] InvalidProtocolVersion(u8), @@ -19,6 +25,9 @@ pub enum Error { /// The return code field contains an unrecognized value. #[error("Invalid value in ReturnCode field: {0:X}")] InvalidReturnCode(u8), + /// The SOME/IP length field was smaller than the 8-byte minimum (`request_id..return_code`). + #[error("Invalid SOME/IP length field: {0} (minimum 8)")] + InvalidLength(u32), /// The message ID is not supported by the payload implementation. #[error("Unsupported MessageID {0:X?}")] UnsupportedMessageID(super::MessageId), @@ -26,3 +35,76 @@ pub enum Error { #[error(transparent)] Sd(#[from] super::sd::Error), } + +impl From for Error { + fn from(k: embedded_io::ErrorKind) -> Self { + Error::Io(k) + } +} + +impl From> for Error { + fn from(e: automotive_wire_codec::EncodeToSliceError) -> Self { + use automotive_wire_codec::EncodeToSliceError::{Encode, InsufficientBuffer}; + match e { + InsufficientBuffer(ib) => Error::InsufficientBuffer(ib), + Encode(inner) => inner, + } + } +} + +/// Bridges [`crate::e2e::Error`] onto `protocol::Error` so E2E failures can be +/// reported through the same error type as the rest of the wire path. +/// +/// E2E deliberately does **not** implement `Encode`/`Decode` (see the +/// `src/e2e` module docs), so it keeps its own `Error` type. This impl only +/// aligns the *shape* of that error with `protocol::Error` for callers that +/// want a single error type to propagate; it does not change E2E's +/// protect/check behavior or on-wire bytes. +/// +/// # Mapping +/// +/// `e2e::Error` currently has exactly one variant: +/// +/// - [`crate::e2e::Error::BufferTooSmall`] `{ needed, actual }` → maps to +/// [`Error::InsufficientBuffer`], **not** [`Error::Incomplete`]. Although +/// `Incomplete { needed, available }` has the identical field shape, its +/// semantics are decode-direction ("input ended before enough bytes could +/// be *read*"). `BufferTooSmall` instead means an output slice was too +/// small to hold the bytes E2E `protect` needed to *write* — the same +/// direction as `automotive_wire_codec::InsufficientBuffer` ("An output +/// slice was too small for the bytes an encode needed to write"). That is +/// the semantically correct counterpart, so `needed`/`actual` map directly +/// onto `InsufficientBuffer`'s `needed`/`available` fields. +impl From for Error { + fn from(err: crate::e2e::Error) -> Self { + match err { + crate::e2e::Error::BufferTooSmall { needed, actual } => { + Error::InsufficientBuffer(automotive_wire_codec::InsufficientBuffer { + needed, + available: actual, + }) + } + } + } +} + +#[cfg(test)] +mod e2e_bridge_tests { + use super::Error; + + #[test] + fn buffer_too_small_maps_to_insufficient_buffer() { + let e2e_err = crate::e2e::Error::BufferTooSmall { + needed: 16, + actual: 10, + }; + let mapped: Error = e2e_err.into(); + match mapped { + Error::InsufficientBuffer(ib) => { + assert_eq!(ib.needed, 16); + assert_eq!(ib.available, 10); + } + other => panic!("expected Error::InsufficientBuffer, got {other:?}"), + } + } +} diff --git a/src/protocol/header.rs b/src/protocol/header.rs index c92899d..561115f 100644 --- a/src/protocol/header.rs +++ b/src/protocol/header.rs @@ -1,7 +1,5 @@ -use crate::{ - protocol::{Error, MessageId, MessageTypeField, ReturnCode, byte_order::WriteBytesExt}, - traits::WireFormat, -}; +use crate::protocol::{Error, MessageId, MessageTypeField, ReturnCode, byte_order::WriteBytesExt}; +use automotive_wire_codec::Decode; /// SOME/IP header #[derive(Clone, Debug, Eq, PartialEq)] @@ -195,7 +193,7 @@ impl Header { /// Returns the payload size in bytes (`length - 8`). #[must_use] pub const fn payload_size(&self) -> usize { - self.length as usize - 8 + (self.length as usize).saturating_sub(8) } /// Sets the request ID field. @@ -220,24 +218,11 @@ impl<'a> HeaderView<'a> { /// # Panics /// /// Cannot panic — the `expect` is guarded by a length check above it. + /// + /// This is a thin wrapper over the [`Decode`] impl, which is the single + /// source of decode logic for this type. pub fn parse(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { - if buf.len() < 16 { - return Err(Error::UnexpectedEof); - } - let header_bytes: &[u8; 16] = buf[..16].try_into().expect("length checked above"); - let view = Self(header_bytes); - - // Validate protocol version - let pv = view.protocol_version(); - if pv != 0x01 { - return Err(Error::InvalidProtocolVersion(pv)); - } - // Validate message type - MessageTypeField::try_from(header_bytes[14])?; - // Validate return code - ReturnCode::try_from(header_bytes[15])?; - - Ok((view, &buf[16..])) + Self::decode(buf) } /// Returns the message ID (service ID + method ID). @@ -263,7 +248,7 @@ impl<'a> HeaderView<'a> { /// Returns the payload size in bytes (`length - 8`). #[must_use] pub fn payload_size(&self) -> usize { - self.length() as usize - 8 + (self.length() as usize).saturating_sub(8) } /// Returns header bytes 8..16: the request ID, protocol and interface @@ -332,12 +317,54 @@ impl<'a> HeaderView<'a> { } } -impl WireFormat for Header { - fn required_size(&self) -> usize { - 16 +impl<'a> Decode<'a> for HeaderView<'a> { + type Error = Error; + + /// Decode and validate a SOME/IP header from the front of `buf`. + /// + /// Returns `(view, remaining_bytes)` on success. + /// + /// # Errors + /// + /// Returns an error if `buf` is shorter than 16 bytes, the protocol version is + /// not `0x01`, the message type byte is unrecognized, or the return code is invalid. + /// + /// # Panics + /// + /// Cannot panic — the `expect` is guarded by a length check above it. + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { + if buf.len() < 16 { + return Err(automotive_wire_codec::Incomplete { + needed: 16, + available: buf.len(), + } + .into()); + } + let header_bytes: &[u8; 16] = buf[..16].try_into().expect("length checked above"); + let view = Self(header_bytes); + + // Validate protocol version + let pv = view.protocol_version(); + if pv != 0x01 { + return Err(Error::InvalidProtocolVersion(pv)); + } + // Validate message type + MessageTypeField::try_from(header_bytes[14])?; + // Validate return code + ReturnCode::try_from(header_bytes[15])?; + + Ok((view, &buf[16..])) + } +} + +impl automotive_wire_codec::Encode for Header { + type Error = Error; + + fn encoded_size(&self) -> Result { + Ok(16) } - fn encode(&self, writer: &mut T) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { writer.write_u32_be(self.message_id.message_id())?; writer.write_u32_be(self.length)?; writer.write_u32_be(self.request_id)?; @@ -353,6 +380,8 @@ impl WireFormat for Header { mod tests { use super::*; use crate::protocol::{Error, MessageId, MessageTypeField, ReturnCode}; + use crate::traits::EncodeExt; + use automotive_wire_codec::Encode; fn make_header() -> Header { Header { @@ -437,7 +466,7 @@ mod tests { #[test] fn required_size_is_16() { - assert_eq!(make_header().required_size(), 16); + assert_eq!(make_header().encoded_size().unwrap(), 16); } // --- encode / parse round-trip --- @@ -527,8 +556,38 @@ mod tests { let buf: [u8; 4] = [0x00, 0x00, 0x00, 0x00]; assert!(matches!( HeaderView::parse(&buf[..]), - Err(Error::UnexpectedEof) + Err(Error::Incomplete(automotive_wire_codec::Incomplete { + needed: 16, + available: 4, + })) + )); + } + + // --- Decode trait (Phase 3) --- + + #[test] + fn decode_returns_header_and_remainder() { + use automotive_wire_codec::Decode; + let h = make_header(); + let mut buf = [0u8; 20]; + buf[..16].copy_from_slice(&encode_header(&h)); + buf[16..].copy_from_slice(&[0xAA, 0xBB, 0xCC, 0xDD]); + let (view, rest) = HeaderView::decode(&buf).unwrap(); + assert_eq!(view.to_owned(), h); + assert_eq!(rest, &[0xAA, 0xBB, 0xCC, 0xDD]); + } + + #[test] + fn decode_exact_rejects_trailing() { + use automotive_wire_codec::Decode; + let h = make_header(); + let mut buf = [0u8; 17]; + buf[..16].copy_from_slice(&encode_header(&h)); + assert!(matches!( + HeaderView::decode_exact(&buf), + Err(Error::Trailing(_)) )); + assert!(HeaderView::decode_exact(&buf[..16]).is_ok()); } // --- from_fields --- @@ -597,7 +656,7 @@ mod tests { assert_eq!(view.is_sd(), h.is_sd()); } - // --- WireFormat default methods --- + // --- Encode/EncodeExt default methods (encode_to_slice / encode_to_vec) --- #[test] fn encode_to_slice_works() { @@ -618,4 +677,31 @@ mod tests { let (view, _) = HeaderView::parse(&buf).unwrap(); assert_eq!(view.to_owned(), h); } + + // --- Encode size-exactness invariant --- + + #[test] + fn encoded_size_matches_bytes_written() { + use automotive_wire_codec::CountingSink; + let h = make_header(); + let mut sink = CountingSink::new(); + let written = h.encode(&mut sink).unwrap(); + assert_eq!(written, h.encoded_size().unwrap()); + assert_eq!(written, sink.count()); + } + + #[test] + fn encode_to_slice_too_small_yields_insufficient_buffer() { + use automotive_wire_codec::{EncodeToSliceError, InsufficientBuffer}; + let h = make_header(); + let mut buf = [0u8; 4]; + let err = h.encode_to_slice(&mut buf).unwrap_err(); + assert!(matches!( + err, + EncodeToSliceError::InsufficientBuffer(InsufficientBuffer { + needed: 16, + available: 4, + }) + )); + } } diff --git a/src/protocol/message.rs b/src/protocol/message.rs index ea121e1..565c7f3 100644 --- a/src/protocol/message.rs +++ b/src/protocol/message.rs @@ -1,7 +1,8 @@ use crate::{ protocol::{Error, Header, MessageType, ReturnCode, header::HeaderView, sd::SdHeaderView}, - traits::{PayloadWireFormat, WireFormat}, + traits::PayloadWireFormat, }; +use automotive_wire_codec::{Decode, Encode}; /// A SOME/IP message consisting of a [`Header`] and a payload. #[derive(Clone, Debug, Eq, PartialEq)] @@ -17,16 +18,27 @@ impl Message { } /// Creates a new SOME/IP-SD message from a request ID and SD header. - #[must_use] + /// + /// # Errors + /// + /// Returns the error from [`Encode::encoded_size`] on the SD header. No + /// in-tree `SdHeader` can fail this -- `sd::Header::encoded_size` is + /// unconditionally `Ok` -- but [`PayloadWireFormat`] is public, and a + /// downstream implementation may. pub fn new_sd( request_id: u32, sd_header: &::SdHeader, - ) -> Self { - let sd_header_size = sd_header.required_size(); - Self::new( + ) -> Result { + // Propagated rather than defaulted. `unwrap_or(0)` produced + // `Header::new_sd(request_id, 0)` -- a header declaring the bare + // 8-byte SD length -- and `encode` then wrote the full payload after + // it. Receivers truncate at the declared length, so a failure here + // used to become silent wire corruption instead of an error. + let sd_header_size = sd_header.encoded_size()?; + Ok(Self::new( Header::new_sd(request_id, sd_header_size), PayloadDefinition::new_sd_payload(sd_header), - ) + )) } /// Returns a reference to the message header. @@ -80,41 +92,16 @@ impl<'a> MessageView<'a> { /// /// Returns an error if the header is invalid, the buffer is too short for the /// declared payload, or SD-specific validation fails. + /// + /// Any bytes past the declared payload are silently discarded. Use the + /// [`Decode`] impl's [`decode`](Decode::decode) to recover the trailing + /// bytes (the next message in a multi-message datagram), or + /// [`decode_exact`](Decode::decode_exact) to reject them. + /// + /// This is a thin wrapper over the [`Decode`] impl, which is the single + /// source of decode logic for this type. pub fn parse(buf: &'a [u8]) -> Result { - let (header, remaining) = HeaderView::parse(buf)?; - let payload_size = header.payload_size(); - - if remaining.len() < payload_size { - return Err(Error::UnexpectedEof); - } - - // SD-specific validation - if header.is_sd() { - if payload_size < 12 { - return Err( - crate::protocol::sd::Error::InvalidMessage("SD message too short").into(), - ); - } - if header.interface_version() != 0x01 { - return Err(crate::protocol::sd::Error::InvalidMessage( - "SD interface version mismatch", - ) - .into()); - } - if header.message_type().message_type() != MessageType::Notification { - return Err( - crate::protocol::sd::Error::InvalidMessage("SD message type mismatch").into(), - ); - } - if header.return_code() != ReturnCode::Ok { - return Err( - crate::protocol::sd::Error::InvalidMessage("SD return code mismatch").into(), - ); - } - } - - let payload = &remaining[..payload_size]; - Ok(Self { header, payload }) + Ok(Self::decode(buf)?.0) } /// Returns the header view. @@ -151,12 +138,75 @@ impl<'a> MessageView<'a> { } } -impl WireFormat for Message { - fn required_size(&self) -> usize { - self.header.required_size() + self.payload.required_size() +impl<'a> Decode<'a> for MessageView<'a> { + type Error = Error; + + /// Decode a single SOME/IP message from the front of `buf`. + /// + /// Validates the header, checks that the buffer contains enough data for the + /// declared payload, and for SD messages validates SD-specific constraints. + /// Returns `(message, remaining_bytes)`, where the remainder is any bytes + /// past this message's declared payload (the next message in a + /// multi-message datagram). + /// + /// # Errors + /// + /// Returns an error if the header is invalid, the buffer is too short for the + /// declared payload, or SD-specific validation fails. + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Error> { + let (header, remaining) = HeaderView::decode(buf)?; + if header.length() < 8 { + return Err(Error::InvalidLength(header.length())); + } + let payload_size = header.payload_size(); + + if remaining.len() < payload_size { + return Err(automotive_wire_codec::Incomplete { + needed: payload_size, + available: remaining.len(), + } + .into()); + } + + // SD-specific validation + if header.is_sd() { + if payload_size < 12 { + return Err( + crate::protocol::sd::Error::InvalidMessage("SD message too short").into(), + ); + } + if header.interface_version() != 0x01 { + return Err(crate::protocol::sd::Error::InvalidMessage( + "SD interface version mismatch", + ) + .into()); + } + if header.message_type().message_type() != MessageType::Notification { + return Err( + crate::protocol::sd::Error::InvalidMessage("SD message type mismatch").into(), + ); + } + if header.return_code() != ReturnCode::Ok { + return Err( + crate::protocol::sd::Error::InvalidMessage("SD return code mismatch").into(), + ); + } + } + + let payload = &remaining[..payload_size]; + let rest = &remaining[payload_size..]; + Ok((Self { header, payload }, rest)) + } +} + +impl Encode for Message { + type Error = Error; + + fn encoded_size(&self) -> Result { + Ok(self.header.encoded_size()? + self.payload.encoded_size()?) } - fn encode(&self, writer: &mut W) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { Ok(self.header.encode(writer)? + self.payload.encode(writer)?) } } @@ -174,7 +224,25 @@ mod tests { } fn make_sd_message() -> Msg { - Msg::new_sd(0x0000_0001, &minimal_sd_header()) + Msg::new_sd(0x0000_0001, &minimal_sd_header()).expect("in-tree SdHeader cannot fail") + } + + /// A failing `SdHeader::encoded_size` must surface as an error, not as a + /// header declaring the bare 8-byte SD length. + /// + /// `unwrap_or(0)` built `Header::new_sd(request_id, 0)` on `Err`, and + /// `Message::encode` then wrote the full payload after it. Receivers + /// truncate at the declared length, so the failure mode was silent wire + /// corruption rather than an error. (PR #153 review.) + #[test] + fn new_sd_surfaces_a_failing_sd_header_size() { + use crate::protocol::sd::test_support::{FailingPayload, FailingSdHeader}; + + assert!( + FailingSdHeader.encoded_size().is_err(), + "fixture must actually fail, or this test proves nothing", + ); + assert!(Message::::new_sd(0x1, &FailingSdHeader).is_err()); } // --- new --- @@ -244,23 +312,23 @@ mod tests { assert_eq!(msg.sd_header().unwrap(), &sd_hdr); } - // --- WireFormat: required_size --- + // --- Encode: encoded_size --- #[test] fn required_size_is_header_plus_payload() { let msg = make_sd_message(); - let expected = msg.header().required_size() + msg.payload().required_size(); - assert_eq!(msg.required_size(), expected); + let expected = msg.header().encoded_size().unwrap() + msg.payload().encoded_size().unwrap(); + assert_eq!(msg.encoded_size().unwrap(), expected); } - // --- WireFormat: encode / MessageView::parse round-trip --- + // --- Encode: encode / MessageView::parse round-trip --- #[test] fn encode_parse_round_trip() { let msg = make_sd_message(); let mut buf = [0u8; 64]; let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); - assert_eq!(n, msg.required_size()); + assert_eq!(n, msg.encoded_size().unwrap()); let view = MessageView::parse(&buf[..n]).unwrap(); assert!(view.is_sd()); assert_eq!(view.header().to_owned(), *msg.header()); @@ -277,7 +345,7 @@ mod tests { entries, options: heapless::Vec::new(), }; - let msg = Msg::new_sd(0x42, &sd_hdr); + let msg = Msg::new_sd(0x42, &sd_hdr).expect("in-tree SdHeader cannot fail"); let mut buf = [0u8; 64]; let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); let view = MessageView::parse(&buf[..n]).unwrap(); @@ -287,6 +355,47 @@ mod tests { assert_eq!(entry.service_id(), 0xABCD); } + // --- Decode: trailing bytes are the next message --- + + #[test] + fn decode_returns_trailing_bytes_as_remainder() { + let msg = make_sd_message(); + let mut buf = [0u8; 128]; + let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); + // Append 5 trailing bytes past the message. + for (i, b) in [0xDE, 0xAD, 0xBE, 0xEF, 0x42].into_iter().enumerate() { + buf[n + i] = b; + } + let (view, rest) = MessageView::decode(&buf[..n + 5]).unwrap(); + assert_eq!(view.header().to_owned(), *msg.header()); + assert_eq!(rest, &[0xDE, 0xAD, 0xBE, 0xEF, 0x42]); + } + + #[test] + fn parse_silently_discards_trailing_bytes() { + let msg = make_sd_message(); + let mut buf = [0u8; 128]; + let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); + buf[n] = 0xFF; + // parse (the thin wrapper) drops the remainder without error. + let view = MessageView::parse(&buf[..=n]).unwrap(); + assert_eq!(view.header().to_owned(), *msg.header()); + } + + #[test] + fn decode_exact_rejects_trailing_bytes() { + let msg = make_sd_message(); + let mut buf = [0u8; 128]; + let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); + buf[n] = 0xFF; + assert!(matches!( + MessageView::decode_exact(&buf[..=n]), + Err(Error::Trailing(_)) + )); + // Exactly-sized succeeds. + assert!(MessageView::decode_exact(&buf[..n]).is_ok()); + } + // --- parse with exactly-sized slice --- #[test] @@ -307,7 +416,46 @@ mod tests { let buf: [u8; 4] = [0; 4]; assert!(matches!( MessageView::parse(&buf[..]), - Err(Error::UnexpectedEof) + Err(Error::Incomplete(automotive_wire_codec::Incomplete { + needed: 16, + available: 4, + })) + )); + } + + #[test] + fn parse_payload_truncated_reports_needed_and_available() { + let msg = make_sd_message(); + let mut buf = [0u8; 64]; + let n = msg.encode(&mut buf.as_mut_slice()).unwrap(); + let payload_size = msg.header().payload_size(); + // Keep the full 16-byte header but chop one byte off the payload. + let short = &buf[..n - 1]; + assert!(matches!( + MessageView::parse(short), + Err(Error::Incomplete(automotive_wire_codec::Incomplete { + needed, + available, + })) if needed == payload_size && available == payload_size - 1 + )); + } + + #[test] + fn decode_rejects_length_below_8() { + let msg = make_sd_message(); + let mut buf = [0u8; 64]; + msg.encode(&mut buf.as_mut_slice()).unwrap(); + // Overwrite the length field (bytes 4..8) with a value below the + // 8-byte minimum. This must be rejected, not underflow/panic. + let bad_len: u32 = 4; + buf[4..8].copy_from_slice(&bad_len.to_be_bytes()); + assert!(matches!( + MessageView::decode(&buf[..]), + Err(Error::InvalidLength(4)) + )); + assert!(matches!( + MessageView::decode_exact(&buf[..16]), + Err(Error::InvalidLength(4)) )); } diff --git a/src/protocol/sd/entry.rs b/src/protocol/sd/entry.rs index 2d4284f..9f58b93 100644 --- a/src/protocol/sd/entry.rs +++ b/src/protocol/sd/entry.rs @@ -1,5 +1,6 @@ use super::Error; -use crate::{protocol::byte_order::WriteBytesExt, traits::WireFormat}; +use crate::protocol::byte_order::WriteBytesExt; +use automotive_wire_codec::{Decode, DecodeIter, Encode, take}; pub const ENTRY_SIZE: usize = 16; @@ -133,15 +134,14 @@ impl EventGroupEntry { } } -impl WireFormat for EventGroupEntry { - fn required_size(&self) -> usize { - 16 +impl Encode for EventGroupEntry { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + Ok(15) } - fn encode( - &self, - writer: &mut T, - ) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { writer.write_u8(self.index_first_options_run)?; writer.write_u8(self.index_second_options_run)?; writer.write_u8(u8::from(self.options_count))?; @@ -151,7 +151,7 @@ impl WireFormat for EventGroupEntry { writer.write_u24_be(self.ttl)?; writer.write_u16_be(self.counter)?; writer.write_u16_be(self.event_group_id)?; - Ok(16) + Ok(15) } } @@ -193,15 +193,14 @@ impl ServiceEntry { } } -impl WireFormat for ServiceEntry { - fn required_size(&self) -> usize { - 16 +impl Encode for ServiceEntry { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + Ok(15) } - fn encode( - &self, - writer: &mut W, - ) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { writer.write_u8(self.index_first_options_run)?; writer.write_u8(self.index_second_options_run)?; writer.write_u8(u8::from(self.options_count))?; @@ -210,7 +209,7 @@ impl WireFormat for ServiceEntry { writer.write_u8(self.major_version)?; writer.write_u24_be(self.ttl)?; writer.write_u32_be(self.minor_version)?; - Ok(16) + Ok(15) } } @@ -269,43 +268,39 @@ impl Entry { } } -impl WireFormat for Entry { - fn required_size(&self) -> usize { - 1 + match self { - Entry::FindService(service_entry) - | Entry::OfferService(service_entry) - | Entry::StopOfferService(service_entry) => service_entry.required_size(), - Entry::SubscribeEventGroup(event_group_entry) - | Entry::SubscribeAckEventGroup(event_group_entry) => event_group_entry.required_size(), - } +impl Encode for Entry { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + // 1 type byte + 15 body bytes = 16 (ENTRY_SIZE) for every variant. + Ok(ENTRY_SIZE) } - fn encode( - &self, - writer: &mut W, - ) -> Result { - match self { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + let body = match self { Entry::FindService(service_entry) => { writer.write_u8(u8::from(EntryType::FindService))?; - service_entry.encode(writer) + service_entry.encode(writer)? } Entry::OfferService(service_entry) => { writer.write_u8(u8::from(EntryType::OfferService))?; - service_entry.encode(writer) + service_entry.encode(writer)? } Entry::StopOfferService(service_entry) => { writer.write_u8(u8::from(EntryType::StopOfferService))?; - service_entry.encode(writer) + service_entry.encode(writer)? } Entry::SubscribeEventGroup(event_group_entry) => { writer.write_u8(u8::from(EntryType::Subscribe))?; - event_group_entry.encode(writer) + event_group_entry.encode(writer)? } Entry::SubscribeAckEventGroup(event_group_entry) => { writer.write_u8(u8::from(EntryType::SubscribeAck))?; - event_group_entry.encode(writer) + event_group_entry.encode(writer)? } - } + }; + // 1 type byte + `body` (15) = 16. + Ok(1 + body) } } @@ -443,6 +438,56 @@ impl EntryView<'_> { } } +impl<'a> Decode<'a> for EntryView<'a> { + type Error = crate::protocol::Error; + + /// Decode a single 16-byte SD entry from the front of `buf`. + /// + /// This is a pure fixed-stride slice: it does NOT validate the entry-type + /// byte. Validation is deferred to [`EntryView::entry_type`] / + /// [`EntryView::to_owned`] (the L2 validation pass), keeping this a lazy + /// zero-copy view. + /// + /// # Errors + /// + /// Returns [`Incomplete`](automotive_wire_codec::Incomplete) if fewer than + /// `ENTRY_SIZE` (16) bytes remain. + /// + /// # Panics + /// + /// Cannot panic — `take` guarantees exactly `ENTRY_SIZE` bytes. + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Self::Error> { + let (head, rest) = take(buf, ENTRY_SIZE)?; + let entry_bytes: &'a [u8; ENTRY_SIZE] = + head.try_into().expect("take guarantees ENTRY_SIZE bytes"); + Ok((EntryView(entry_bytes), rest)) + } +} + +impl<'a> DecodeIter<'a> for EntryView<'a> { + type Error = crate::protocol::Error; + + /// SD entries have a fixed 16-byte (`ENTRY_SIZE`) stride, enabling + /// [`DecodeIterator::remaining_len`](automotive_wire_codec::DecodeIterator::remaining_len). + const WIRE_SIZE: Option = Some(ENTRY_SIZE); + + /// Decode the next entry, or `Ok(None)` at a clean end of buffer. + /// + /// A partial (non-multiple-of-16) trailing element is surfaced as an + /// `Err` rather than silently dropped. + /// + /// # Errors + /// + /// Returns [`Incomplete`](automotive_wire_codec::Incomplete) if a partial + /// entry remains after a good start. + fn decode_next(buf: &'a [u8]) -> Result, Self::Error> { + if buf.is_empty() { + return Ok(None); + } + Self::decode(buf).map(Some) + } +} + /// Iterator over 16-byte SD entries in a validated buffer. /// Entries are guaranteed valid (validated upfront in `SdHeaderView::parse`). pub struct EntryIter<'a> { @@ -559,33 +604,48 @@ mod tests { // --- required_size --- #[test] - fn service_entry_required_size() { - assert_eq!(make_service_entry().required_size(), 16); + fn service_entry_encoded_size() { + // 15 body bytes (no leading type byte — that belongs to `Entry`). + assert_eq!(make_service_entry().encoded_size().unwrap(), 15); } #[test] - fn event_group_entry_required_size() { - assert_eq!(make_event_group_entry().required_size(), 16); + fn event_group_entry_encoded_size() { + assert_eq!(make_event_group_entry().encoded_size().unwrap(), 15); } #[test] - fn entry_required_size_all_variants() { - assert_eq!(Entry::FindService(make_service_entry()).required_size(), 17); + fn entry_encoded_size_all_variants() { + // 1 type byte + 15 body bytes = 16 (ENTRY_SIZE) for every variant. + assert_eq!( + Entry::FindService(make_service_entry()) + .encoded_size() + .unwrap(), + 16 + ); assert_eq!( - Entry::OfferService(make_service_entry()).required_size(), - 17 + Entry::OfferService(make_service_entry()) + .encoded_size() + .unwrap(), + 16 ); assert_eq!( - Entry::StopOfferService(make_service_entry()).required_size(), - 17 + Entry::StopOfferService(make_service_entry()) + .encoded_size() + .unwrap(), + 16 ); assert_eq!( - Entry::SubscribeEventGroup(make_event_group_entry()).required_size(), - 17 + Entry::SubscribeEventGroup(make_event_group_entry()) + .encoded_size() + .unwrap(), + 16 ); assert_eq!( - Entry::SubscribeAckEventGroup(make_event_group_entry()).required_size(), - 17 + Entry::SubscribeAckEventGroup(make_event_group_entry()) + .encoded_size() + .unwrap(), + 16 ); } @@ -702,4 +762,162 @@ mod tests { assert_eq!(iter.next().unwrap().to_owned().unwrap(), e2); assert!(iter.next().is_none()); } + + // --- Decode / DecodeIter (Phase 3 lazy L1) --- + + fn two_entry_buf(e1: &Entry, e2: &Entry) -> [u8; 32] { + let b1 = encode_entry(e1); + let b2 = encode_entry(e2); + let mut combined = [0u8; 32]; + combined[..16].copy_from_slice(&b1[..16]); + combined[16..32].copy_from_slice(&b2[..16]); + combined + } + + #[test] + fn decode_yields_entry_and_remainder() { + let e1 = Entry::FindService(make_service_entry()); + let e2 = Entry::SubscribeEventGroup(make_event_group_entry()); + let buf = two_entry_buf(&e1, &e2); + let (view, rest) = EntryView::decode(&buf).unwrap(); + assert_eq!(view.to_owned().unwrap(), e1); + assert_eq!(rest.len(), 16); + let (view2, rest2) = EntryView::decode(rest).unwrap(); + assert_eq!(view2.to_owned().unwrap(), e2); + assert!(rest2.is_empty()); + } + + #[test] + fn decode_truncated_is_incomplete() { + let e1 = Entry::FindService(make_service_entry()); + let buf = encode_entry(&e1); + assert!(matches!( + EntryView::decode(&buf[..15]), + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 16, + available: 15, + } + )) + )); + } + + #[test] + fn decode_exact_rejects_trailing() { + let e1 = Entry::FindService(make_service_entry()); + let e2 = Entry::OfferService(make_service_entry()); + let buf = two_entry_buf(&e1, &e2); + assert!(matches!( + EntryView::decode_exact(&buf), + Err(crate::protocol::Error::Trailing(_)) + )); + // Single entry consumes the whole buffer. + assert!(EntryView::decode_exact(&buf[..16]).is_ok()); + } + + #[test] + fn decode_iter_yields_all_then_none() { + let e1 = Entry::FindService(make_service_entry()); + let e2 = Entry::SubscribeEventGroup(make_event_group_entry()); + let buf = two_entry_buf(&e1, &e2); + let mut iter = EntryView::iter(&buf); + assert_eq!(iter.next().unwrap().unwrap().to_owned().unwrap(), e1); + assert_eq!(iter.next().unwrap().unwrap().to_owned().unwrap(), e2); + assert!(iter.next().is_none()); + } + + #[test] + fn decode_iter_surfaces_truncated_tail_as_err() { + let e1 = Entry::FindService(make_service_entry()); + let buf = two_entry_buf(&e1, &Entry::OfferService(make_service_entry())); + // One full entry plus a partial (8-byte) tail. + let mut iter = EntryView::iter(&buf[..24]); + assert!(matches!(iter.next(), Some(Ok(_)))); + assert!(matches!( + iter.next(), + Some(Err(crate::protocol::Error::Incomplete(_))) + )); + // Adapter fuses after the first error. + assert!(iter.next().is_none()); + } + + #[test] + fn decode_iter_empty_is_immediately_none() { + let mut iter = EntryView::iter(&[]); + assert!(iter.next().is_none()); + } + + #[test] + fn decode_iter_remaining_len_counts_entries() { + let e1 = Entry::FindService(make_service_entry()); + let e2 = Entry::SubscribeEventGroup(make_event_group_entry()); + let buf = two_entry_buf(&e1, &e2); + let mut iter = EntryView::iter(&buf); + assert_eq!(iter.remaining_len(), Some(2)); + iter.next(); + assert_eq!(iter.remaining_len(), Some(1)); + iter.next(); + iter.next(); // Ok(None) -> done + assert_eq!(iter.remaining_len(), Some(0)); + } + + #[test] + fn decode_iter_does_not_validate_entry_type() { + // An invalid entry-type byte (0x03) must still decode as a view — the + // lazy path defers type validation to `to_owned` / `entry_type`. + let buf = [0x03u8; ENTRY_SIZE]; + let mut iter = EntryView::iter(&buf); + let view = iter.next().unwrap().unwrap(); + assert!(matches!( + view.to_owned(), + Err(Error::InvalidEntryType(0x03)) + )); + } + + // --- Encode size-exactness invariant --- + + #[test] + fn entry_encoded_size_matches_bytes_written() { + use automotive_wire_codec::CountingSink; + for entry in [ + Entry::FindService(make_service_entry()), + Entry::SubscribeEventGroup(make_event_group_entry()), + ] { + let mut sink = CountingSink::new(); + let written = entry.encode(&mut sink).unwrap(); + assert_eq!(written, entry.encoded_size().unwrap()); + assert_eq!(written, sink.count()); + } + } + + #[test] + fn service_entry_encoded_size_matches_bytes_written() { + use automotive_wire_codec::CountingSink; + let se = make_service_entry(); + let mut sink = CountingSink::new(); + let written = se.encode(&mut sink).unwrap(); + assert_eq!(written, se.encoded_size().unwrap()); + assert_eq!(written, sink.count()); + + let eg = make_event_group_entry(); + let mut sink = CountingSink::new(); + let written = eg.encode(&mut sink).unwrap(); + assert_eq!(written, eg.encoded_size().unwrap()); + assert_eq!(written, sink.count()); + } + + #[test] + fn entry_encode_to_slice_too_small_yields_insufficient_buffer() { + use automotive_wire_codec::{EncodeToSliceError, InsufficientBuffer}; + let entry = Entry::FindService(make_service_entry()); + let mut buf = [0u8; 4]; // far smaller than 16 + let err = entry.encode_to_slice(&mut buf).unwrap_err(); + assert!(matches!( + err, + EncodeToSliceError::InsufficientBuffer(InsufficientBuffer { + needed: 16, + available: 4, + }) + )); + } } diff --git a/src/protocol/sd/error.rs b/src/protocol/sd/error.rs index f30ee15..525cfa7 100644 --- a/src/protocol/sd/error.rs +++ b/src/protocol/sd/error.rs @@ -14,8 +14,13 @@ pub enum Error { #[error("Invalid value for Service Discovery Option Transport Protocol: {0:X}")] InvalidOptionTransportProtocol(u8), /// The declared options size does not match the actual data. - #[error("Incorrect options size, {0} bytes remaining")] - IncorrectOptionsSize(usize), + #[error("Incorrect options size: need {needed} bytes, have {available}")] + IncorrectOptionsSize { + /// The number of bytes required for the option header or body. + needed: usize, + /// The number of bytes actually remaining in the buffer. + available: usize, + }, /// An option's length field does not match the expected size for its type. #[error( "Invalid SD option length for type 0x{option_type:02X}: expected {expected}, got {actual}" diff --git a/src/protocol/sd/header.rs b/src/protocol/sd/header.rs index 2321a31..6a75997 100644 --- a/src/protocol/sd/header.rs +++ b/src/protocol/sd/header.rs @@ -1,11 +1,11 @@ use crate::protocol::byte_order::WriteBytesExt; -use crate::traits::WireFormat; +use automotive_wire_codec::{Decode, DecodeIter, DecodeIterator, Encode}; use super::{ - Entry, Flags, Options, - entry::{ENTRY_SIZE, EntryIter, EntryType}, - options::{OptionIter, validate_option}, + Entry, EntryView, Flags, OptionView, Options, + entry::{ENTRY_SIZE, EntryIter}, + options::OptionIter, }; /// An SD header that borrows its entries and options slices. @@ -39,11 +39,28 @@ impl<'a> Header<'a> { /// Created by [`SdHeaderView::parse`], which fully validates the SD header, /// entries, and options upfront. This makes the entry and option iterators /// infallible. +/// +/// # Validation-proof invariant (candidate "c") +/// +/// The type system carries no proof tying the cached `entry_count` / +/// `option_count` to `entries_buf` / `options_buf`. `parse` runs ONE eager +/// validating walk (draining the lazy L1 [`DecodeIterator`]s) and caches the +/// element counts; the infallible accessors ([`entries`](SdHeaderView::entries) +/// / [`options`](SdHeaderView::options)) then re-slice those *already-validated* +/// buffers with purpose-built iterators that advance by stride/length WITHOUT +/// re-running the type/length/transport checks. They TRUST the construction-time +/// walk. Nothing but this `parse`-only construction path may populate the +/// buffers, so the trust holds — the same invariant `OptionIter` has always +/// relied on. #[derive(Clone, Copy, Debug)] pub struct SdHeaderView<'a> { flags: Flags, entries_buf: &'a [u8], options_buf: &'a [u8], + /// Number of valid entries found during the construction-time walk. + entry_count: usize, + /// Number of valid options found during the construction-time walk. + option_count: usize, } impl<'a> SdHeaderView<'a> { @@ -62,61 +79,42 @@ impl<'a> SdHeaderView<'a> { /// any entry type byte is invalid, or any option has an invalid type, length, or /// transport protocol byte. pub fn parse(buf: &'a [u8]) -> Result { - // Minimum: 4 (flags+reserved) + 4 (entries_size) + 4 (options_size) = 12 - if buf.len() < 12 { - return Err(crate::protocol::Error::UnexpectedEof); - } - - let flags = Flags::from(buf[0]); - // bytes [1..4] are reserved - - let entries_size = u32::from_be_bytes([buf[4], buf[5], buf[6], buf[7]]) as usize; - - if !entries_size.is_multiple_of(ENTRY_SIZE) { - return Err(super::Error::IncorrectEntriesSize(entries_size).into()); + // The O(1) slicing + length checks (flags/reserved, entries_size, + // options_size, and the section bounds, incl. overflow hardening) live + // in the single decode source, `SdBody::decode`. This L2 path is + // re-founded (Phase 4) on top of that lazy L1 layer: it runs ONE eager + // validating walk by draining the L1 `DecodeIterator`s over the + // already-sliced entries and options sections, surfacing the first + // `Err` via `?`, and caches the element counts. The infallible + // accessors then re-slice these validated buffers without re-validating + // (candidate "c" — see the type-level docs for the trust invariant). + let (body, _rest) = SdBody::decode(buf)?; + + // Eager validating walk over the entries section. `EntryView`'s L1 + // decode only slices the fixed 16-byte stride (surfacing truncation); + // `entry_type()` validates the type byte. A partial trailing entry is + // surfaced here as an `Err`, not silently truncated at accessor time. + let mut entry_count = 0usize; + for entry in body.entries() { + entry?.entry_type()?; + entry_count += 1; } - // Need entries data + 4 bytes for options_size field - if buf.len() < 8 + entries_size + 4 { - return Err(crate::protocol::Error::UnexpectedEof); - } - - let entries_buf = &buf[8..8 + entries_size]; - - // Validate all entry type bytes - let mut offset = 0; - while offset < entries_size { - EntryType::try_from(entries_buf[offset])?; - offset += ENTRY_SIZE; - } - - let options_size_offset = 8 + entries_size; - let options_size = u32::from_be_bytes([ - buf[options_size_offset], - buf[options_size_offset + 1], - buf[options_size_offset + 2], - buf[options_size_offset + 3], - ]) as usize; - - let options_start = options_size_offset + 4; - if buf.len() < options_start + options_size { - return Err(crate::protocol::Error::UnexpectedEof); - } - - let options_buf = &buf[options_start..options_start + options_size]; - - // Validate all options - let mut opt_offset = 0; - while opt_offset < options_size { - let remaining = &options_buf[opt_offset..]; - let wire_size = validate_option(remaining)?; - opt_offset += wire_size; + // Eager validating walk over the options section. `OptionView`'s L1 + // decode only slices by the length field (surfacing truncation); + // `validate()` checks type / per-type length / transport-protocol byte. + let mut option_count = 0usize; + for option in body.options() { + option?.validate()?; + option_count += 1; } Ok(Self { - flags, - entries_buf, - options_buf, + flags: body.flags, + entries_buf: body.entries_buf, + options_buf: body.options_buf, + entry_count, + option_count, }) } @@ -126,38 +124,201 @@ impl<'a> SdHeaderView<'a> { self.flags } - /// Returns an iterator over the SD entries. + /// Returns an infallible iterator over the SD entries. + /// + /// Re-slices the already-validated `entries_buf` at the fixed 16-byte + /// stride; it never re-runs entry-type validation (done once in + /// [`parse`](SdHeaderView::parse)). + /// The returned [`EntryIter`] is [`ExactSizeIterator`] — its length comes + /// for free from the fixed stride. #[must_use] pub fn entries(&self) -> EntryIter<'a> { EntryIter::new(self.entries_buf) } - /// Returns an iterator over the SD options. + /// Returns an infallible iterator over the SD options. + /// + /// Re-slices the already-validated `options_buf` by each option's length + /// field; it never re-runs option validation (done once in + /// [`parse`](SdHeaderView::parse)). + /// Options have no fixed stride, so [`OptionIter`] is not itself + /// [`ExactSizeIterator`]; use [`option_count`](SdHeaderView::option_count) + /// for the cached element count. #[must_use] pub fn options(&self) -> OptionIter<'a> { OptionIter::new(self.options_buf) } /// Returns the number of entries in this SD header. + /// + /// This is the count cached by the construction-time validating walk. #[must_use] pub fn entry_count(&self) -> usize { - self.entries_buf.len() / ENTRY_SIZE + self.entry_count + } + + /// Returns the number of options in this SD header. + /// + /// This is the count cached by the construction-time validating walk. + /// Because options have no fixed stride, this cached count is the analogue + /// of `EntryIter`'s free `ExactSizeIterator::len` for the options section. + #[must_use] + pub fn option_count(&self) -> usize { + self.option_count + } +} + +/// Lazy zero-copy view over an SD payload body. +/// +/// [`SdBody::decode`] performs only the O(1) flag decode and section slicing +/// (with the accompanying length / `entries_size`-multiple checks); it does NOT +/// walk the entries validating their type bytes, nor the options validating +/// their type / length / transport-protocol bytes. That per-element validation +/// is the job of the lazy [`DecodeIter`] adapters returned by [`SdBody::entries`] +/// / [`SdBody::options`], or of the L2 validation pass ([`SdHeaderView::parse`]). +/// +/// Contrast with [`SdHeaderView`], which validates everything upfront so its +/// iterators are infallible. +#[derive(Clone, Copy, Debug)] +pub struct SdBody<'a> { + flags: Flags, + entries_buf: &'a [u8], + options_buf: &'a [u8], +} + +impl<'a> SdBody<'a> { + /// Returns the SD flags. + #[must_use] + pub fn flags(&self) -> Flags { + self.flags + } + + /// Returns a lazy iterator over the SD entries. + /// + /// Each item is a `Result`; a malformed/truncated entry + /// surfaces as an `Err`. The entry-type byte is not validated here — call + /// [`EntryView::entry_type`] / [`EntryView::to_owned`] to validate it. + #[must_use] + pub fn entries(&self) -> DecodeIterator<'a, EntryView<'a>> { + EntryView::iter(self.entries_buf) + } + + /// Returns a lazy iterator over the SD options. + /// + /// Each item is a `Result`; a malformed/truncated option + /// surfaces as an `Err`. Option type / length / transport-protocol bytes + /// are not validated here — validate them via the `OptionView` accessors. + #[must_use] + pub fn options(&self) -> DecodeIterator<'a, OptionView<'a>> { + OptionView::iter(self.options_buf) } } -impl WireFormat for Header<'_> { - fn required_size(&self) -> usize { +impl<'a> Decode<'a> for SdBody<'a> { + type Error = crate::protocol::Error; + + /// Decode and slice an SD payload body from the front of `buf`. + /// + /// Performs only the flag decode and the section slicing / length checks + /// (buffer minimum, `entries_size` multiple-of-16, and section bounds). It + /// deliberately does NOT validate entry-type bytes or option contents — + /// see the type-level docs. + /// + /// # Errors + /// + /// Returns [`Incomplete`](automotive_wire_codec::Incomplete) if the buffer + /// is too short for the declared sections, or + /// [`IncorrectEntriesSize`](super::Error::IncorrectEntriesSize) if + /// `entries_size` is not a multiple of `ENTRY_SIZE` (16). + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Self::Error> { + // Minimum: 4 (flags+reserved) + 4 (entries_size) + 4 (options_size) = 12 + if buf.len() < 12 { + return Err(automotive_wire_codec::Incomplete { + needed: 12, + available: buf.len(), + } + .into()); + } + + let flags = Flags::from(buf[0]); + // bytes [1..4] are reserved + + let entries_size = u32::from_be_bytes([buf[4], buf[5], buf[6], buf[7]]) as usize; + + if !entries_size.is_multiple_of(ENTRY_SIZE) { + return Err(super::Error::IncorrectEntriesSize(entries_size).into()); + } + + // All section-bound arithmetic is `checked_add`: on a 32-bit `usize` + // (`no_std` embedded targets) a hostile `entries_size` / `options_size` + // near `u32::MAX` would otherwise wrap `8 + entries_size + 4` or + // `options_start + options_size` and pass the length check with a bogus + // small bound. An overflow means the buffer cannot possibly hold the + // declared sections, so it is reported as `Incomplete`. + let overflow = || automotive_wire_codec::Incomplete { + needed: usize::MAX, + available: buf.len(), + }; + + // Need entries data + 4 bytes for options_size field. + let entries_end = 8usize.checked_add(entries_size).ok_or_else(overflow)?; + let options_size_offset = entries_end; + let entries_section_end = entries_end.checked_add(4).ok_or_else(overflow)?; + if buf.len() < entries_section_end { + return Err(automotive_wire_codec::Incomplete { + needed: entries_section_end, + available: buf.len(), + } + .into()); + } + + let entries_buf = &buf[8..options_size_offset]; + + let options_size = u32::from_be_bytes([ + buf[options_size_offset], + buf[options_size_offset + 1], + buf[options_size_offset + 2], + buf[options_size_offset + 3], + ]) as usize; + + let options_start = entries_section_end; + let options_end = options_start + .checked_add(options_size) + .ok_or_else(overflow)?; + if buf.len() < options_end { + return Err(automotive_wire_codec::Incomplete { + needed: options_end, + available: buf.len(), + } + .into()); + } + + let options_buf = &buf[options_start..options_end]; + let rest = &buf[options_end..]; + + Ok(( + Self { + flags, + entries_buf, + options_buf, + }, + rest, + )) + } +} + +impl Encode for Header<'_> { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { let mut size = 12 + self.entries.len() * ENTRY_SIZE; for option in self.options { size += option.size(); } - size + Ok(size) } - fn encode( - &self, - writer: &mut T, - ) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { writer.write_u8(u8::from(self.flags))?; let reserved: [u8; 3] = [0; 3]; writer.write_bytes(&reserved)?; @@ -172,7 +333,7 @@ impl WireFormat for Header<'_> { } writer.write_u32_be(u32::try_from(options_size).expect("options size fits u32"))?; for option in self.options { - option.write(writer)?; + option.encode(writer)?; } Ok(12 + entries_size as usize + options_size) } @@ -183,17 +344,15 @@ mod tests { use core::net::Ipv4Addr; use super::*; - use crate::{ - protocol::sd::{ - Error as SdError, EventGroupEntry, OptionType, OptionsCount, RebootFlag, ServiceEntry, - TransportProtocol, - options::{ - IPV4_OPTION_IP_OFFSET, IPV4_OPTION_LENGTH_FIELD, IPV4_OPTION_PORT_OFFSET, - IPV4_OPTION_PROTOCOL_OFFSET, IPV4_OPTION_WIRE_SIZE, - }, + use crate::protocol::sd::{ + Error as SdError, EventGroupEntry, OptionType, OptionsCount, RebootFlag, ServiceEntry, + TransportProtocol, + options::{ + IPV4_OPTION_IP_OFFSET, IPV4_OPTION_LENGTH_FIELD, IPV4_OPTION_PORT_OFFSET, + IPV4_OPTION_PROTOCOL_OFFSET, IPV4_OPTION_WIRE_SIZE, }, - traits::WireFormat, }; + use automotive_wire_codec::Encode; fn ipv4_endpoint_bytes(ip: [u8; 4], protocol: u8, port: u16) -> [u8; IPV4_OPTION_WIRE_SIZE] { let mut b = [0u8; IPV4_OPTION_WIRE_SIZE]; @@ -252,10 +411,10 @@ mod tests { &entries, &options, ); - assert_eq!(h.required_size(), 40); + assert_eq!(h.encoded_size().unwrap(), 40); let mut buf = [0u8; 64]; h.encode(&mut buf.as_mut_slice()).unwrap(); - let view = SdHeaderView::parse(&buf[..h.required_size()]).unwrap(); + let view = SdHeaderView::parse(&buf[..h.encoded_size().unwrap()]).unwrap(); assert_eq!(view.entry_count(), 1); let entry_view = view.entries().next().unwrap(); assert_eq!(entry_view.service_id(), 0x1234); @@ -268,10 +427,10 @@ mod tests { )); let entries = [entry]; let h = Header::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &entries, &[]); - assert_eq!(h.required_size(), 28); + assert_eq!(h.encoded_size().unwrap(), 28); let mut buf = [0u8; 32]; h.encode(&mut buf.as_mut_slice()).unwrap(); - let view = SdHeaderView::parse(&buf[..h.required_size()]).unwrap(); + let view = SdHeaderView::parse(&buf[..h.encoded_size().unwrap()]).unwrap(); assert_eq!(view.entry_count(), 1); } @@ -307,12 +466,21 @@ mod tests { #[test] fn parse_options_size_below_minimum_returns_error() { + // The eager L2 walk now drains the L1 option `DecodeIterator`, so a + // truncated options section surfaces the L1 `Incomplete` (the needed / + // available byte counts are unchanged) rather than the old hand-rolled + // `IncorrectOptionsSize`. let prefix = raw_header(0, 2); let mut buf = [0u8; 14]; buf[..12].copy_from_slice(&prefix); assert!(matches!( SdHeaderView::parse(&buf), - Err(crate::protocol::Error::Sd(SdError::IncorrectOptionsSize(2))) + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 4, + available: 2, + } + )) )); } @@ -325,7 +493,12 @@ mod tests { buf[12..24].copy_from_slice(&option); assert!(matches!( SdHeaderView::parse(&buf), - Err(crate::protocol::Error::Sd(SdError::IncorrectOptionsSize(5))) + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 12, + available: 5, + } + )) )); } @@ -340,8 +513,69 @@ mod tests { let h = Header::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &entries, &[]); let mut buf = [0u8; 64]; h.encode(&mut buf.as_mut_slice()).unwrap(); - let view = SdHeaderView::parse(&buf[..h.required_size()]).unwrap(); + let view = SdHeaderView::parse(&buf[..h.encoded_size().unwrap()]).unwrap(); + assert_eq!(view.entry_count(), 2); + } + + #[test] + fn sd_header_view_accessors_yield_cached_counts() { + // After a successful parse, the infallible accessors yield exactly the + // cached counts and never panic. + let ip = Ipv4Addr::new(192, 168, 1, 10); + let entries = [ + Entry::FindService(ServiceEntry::find(0x0001)), + Entry::FindService(ServiceEntry::find(0x0002)), + ]; + let options = [ + Options::IpV4Endpoint { + ip, + protocol: TransportProtocol::Udp, + port: 30509, + }, + Options::IpV4Endpoint { + ip, + protocol: TransportProtocol::Tcp, + port: 30510, + }, + ]; + let h = Header::new( + Flags::new_sd(RebootFlag::RecentlyRebooted), + &entries, + &options, + ); + let mut buf = [0u8; 128]; + let n = h.encode(&mut buf.as_mut_slice()).unwrap(); + let view = SdHeaderView::parse(&buf[..n]).unwrap(); assert_eq!(view.entry_count(), 2); + assert_eq!(view.option_count(), 2); + // Infallible accessors walk without panicking and match the counts. + assert_eq!(view.entries().count(), view.entry_count()); + assert_eq!(view.options().count(), view.option_count()); + // EntryIter is ExactSizeIterator: its len matches the cached count. + assert_eq!(view.entries().len(), view.entry_count()); + } + + #[test] + fn parse_rejects_trailing_partial_option() { + // options_size declares 12 bytes, but the single option's length field + // claims a wire size of 16 (length = 13). The eager walk must reject + // this at parse rather than silently truncating at accessor time. + let prefix = raw_header(0, 12); + let mut option = ipv4_endpoint_bytes([10, 0, 0, 1], 0x11, 30490); + // Overwrite the length field to claim more bytes than are present. + option[0..2].copy_from_slice(&13u16.to_be_bytes()); + let mut buf = [0u8; 24]; + buf[..12].copy_from_slice(&prefix); + buf[12..24].copy_from_slice(&option); + assert!(matches!( + SdHeaderView::parse(&buf), + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 16, + available: 12, + } + )) + )); } #[test] @@ -349,7 +583,7 @@ mod tests { let h = Header::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &[], &[]); let mut buf = [0u8; 16]; h.encode(&mut buf.as_mut_slice()).unwrap(); - let view = SdHeaderView::parse(&buf[..h.required_size()]).unwrap(); + let view = SdHeaderView::parse(&buf[..h.encoded_size().unwrap()]).unwrap(); assert_eq!(view.flags(), h.flags); } @@ -383,4 +617,137 @@ mod tests { )) )); } + + // --- SdBody (Phase 3 lazy L1 decode) --- + + #[test] + fn sd_body_decode_slices_sections() { + let ip = Ipv4Addr::new(192, 168, 1, 10); + let entry = Entry::OfferService(ServiceEntry { + service_id: 0x1234, + instance_id: 0x0001, + major_version: 1, + ttl: 0xFF_FFFF, + index_first_options_run: 0, + index_second_options_run: 0, + options_count: OptionsCount::new(1, 0), + minor_version: 0, + }); + let endpoint = Options::IpV4Endpoint { + ip, + protocol: TransportProtocol::Udp, + port: 30509, + }; + let entries = [entry]; + let options = [endpoint]; + let h = Header::new( + Flags::new_sd(RebootFlag::RecentlyRebooted), + &entries, + &options, + ); + let mut buf = [0u8; 64]; + let n = h.encode(&mut buf.as_mut_slice()).unwrap(); + let (body, rest) = SdBody::decode(&buf[..n]).unwrap(); + assert!(rest.is_empty()); + assert_eq!(body.flags(), h.flags); + // Lazy iterators recover the entry and option. + let entry_view = body.entries().next().unwrap().unwrap(); + assert_eq!(entry_view.service_id(), 0x1234); + let opt_view = body.options().next().unwrap().unwrap(); + assert_eq!(opt_view.as_ipv4().unwrap().0, ip); + } + + #[test] + fn sd_body_decode_returns_trailing_remainder() { + let h = Header::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &[], &[]); + let mut buf = [0u8; 32]; + let n = h.encode(&mut buf.as_mut_slice()).unwrap(); + // Append 3 extra trailing bytes past the SD body. + buf[n] = 0xDE; + buf[n + 1] = 0xAD; + buf[n + 2] = 0xBE; + let (_body, rest) = SdBody::decode(&buf[..n + 3]).unwrap(); + assert_eq!(rest, &[0xDE, 0xAD, 0xBE]); + } + + #[test] + fn sd_body_decode_defers_entry_type_validation() { + // A body whose single entry has an invalid entry-type byte (0x03) + // must still decode successfully — SdBody does NOT walk entry types. + // entries_size = 16 (valid multiple), options_size = 0. + let mut buf = [0u8; 28]; + buf[4..8].copy_from_slice(&16u32.to_be_bytes()); + buf[8] = 0x03; // invalid entry type byte + // bytes 24..28 = options_size = 0 + let (body, rest) = SdBody::decode(&buf).unwrap(); + assert!(rest.is_empty()); + // The lazy iterator produces the view; validation only fails on to_owned. + let entry_view = body.entries().next().unwrap().unwrap(); + assert!(matches!( + entry_view.to_owned(), + Err(SdError::InvalidEntryType(0x03)) + )); + // But SdHeaderView::parse (the eager L2 walk) DOES reject it. + assert!(matches!( + SdHeaderView::parse(&buf), + Err(crate::protocol::Error::Sd(SdError::InvalidEntryType(0x03))) + )); + } + + #[test] + fn sd_body_decode_defers_option_validation() { + // options_size = 12 with an IPv4 option carrying an invalid transport + // protocol byte. SdBody slices it without complaint. + const PREFIX: usize = 12; + let options_size = u32::try_from(IPV4_OPTION_WIRE_SIZE).unwrap(); + let prefix = raw_header(0, options_size); + let option = ipv4_endpoint_bytes([10, 0, 0, 1], 0xAB, 30490); + let mut buf = [0u8; PREFIX + IPV4_OPTION_WIRE_SIZE]; + buf[..PREFIX].copy_from_slice(&prefix); + buf[PREFIX..].copy_from_slice(&option); + let (body, rest) = SdBody::decode(&buf).unwrap(); + assert!(rest.is_empty()); + let opt_view = body.options().next().unwrap().unwrap(); + assert!(matches!( + opt_view.as_ipv4(), + Err(SdError::InvalidOptionTransportProtocol(0xAB)) + )); + } + + #[test] + fn sd_body_decode_too_short_is_incomplete() { + let buf = [0u8; 8]; + assert!(matches!( + SdBody::decode(&buf), + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 12, + available: 8, + } + )) + )); + } + + #[test] + fn sd_body_decode_rejects_non_multiple_entries_size() { + let mut buf = [0u8; 12]; + buf[4..8].copy_from_slice(&5u32.to_be_bytes()); + assert!(matches!( + SdBody::decode(&buf), + Err(crate::protocol::Error::Sd(SdError::IncorrectEntriesSize(5))) + )); + } + + #[test] + fn sd_body_entries_remaining_len_reports_count() { + let entries = [ + Entry::FindService(ServiceEntry::find(0x0001)), + Entry::FindService(ServiceEntry::find(0x0002)), + ]; + let h = Header::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &entries, &[]); + let mut buf = [0u8; 64]; + let n = h.encode(&mut buf.as_mut_slice()).unwrap(); + let (body, _rest) = SdBody::decode(&buf[..n]).unwrap(); + assert_eq!(body.entries().remaining_len(), Some(2)); + } } diff --git a/src/protocol/sd/mod.rs b/src/protocol/sd/mod.rs index 7599fc7..6d893db 100644 --- a/src/protocol/sd/mod.rs +++ b/src/protocol/sd/mod.rs @@ -23,7 +23,7 @@ pub use entry::{ }; pub use error::Error; pub use flags::{Flags, RebootFlag}; -pub use header::{Header, SdHeaderView}; +pub use header::{Header, SdBody, SdHeaderView}; pub use options::{ MAX_CONFIGURATION_STRING_LENGTH, OptionIter, OptionType, OptionView, Options, TransportProtocol, extract_ipv4_endpoint, diff --git a/src/protocol/sd/options.rs b/src/protocol/sd/options.rs index 4087ccd..f9d8753 100644 --- a/src/protocol/sd/options.rs +++ b/src/protocol/sd/options.rs @@ -2,6 +2,7 @@ use core::net::{Ipv4Addr, Ipv6Addr}; use super::Error; use crate::protocol::byte_order::WriteBytesExt; +use automotive_wire_codec::{Decode, DecodeIter, Encode, ensure_len, take}; /// Maximum length of an SD configuration option string in bytes. pub const MAX_CONFIGURATION_STRING_LENGTH: usize = 256; @@ -226,6 +227,14 @@ impl Options { | Options::IpV6SD { .. } => IPV6_OPTION_WIRE_SIZE, } } +} + +impl Encode for Options { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + Ok(self.size()) + } /// Serializes this option to a writer. /// @@ -237,10 +246,7 @@ impl Options { /// /// Panics if the option size minus `OPTION_LENGTH_SIZE_DELTA` exceeds `u16::MAX` /// (unreachable in practice). - pub fn write( - &self, - writer: &mut T, - ) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { writer.write_u16_be( u16::try_from(self.size() - OPTION_LENGTH_SIZE_DELTA).expect("option size fits u16"), )?; @@ -359,6 +365,34 @@ impl<'a> OptionView<'a> { usize::from(length) + OPTION_LENGTH_SIZE_DELTA } + /// Fully validate this option's wire format (type, per-type length, and + /// transport-protocol byte for IP-bearing options). + /// + /// Used by the eager L2 validation walk in + /// [`SdHeaderView::parse`](super::SdHeaderView::parse) so that its + /// infallible option accessors can trust the buffer thereafter. + /// + /// # Errors + /// + /// Returns an error if the option type, length, or transport-protocol byte + /// is invalid. + pub(crate) fn validate(&self) -> Result<(), Error> { + validate_option(self.0).map(|_| ()) + } + + /// A view is only guaranteed to hold the 4-byte option header -- `decode` + /// is deliberately lazy about the type and per-type length -- so each + /// accessor that reads a body checks its own span before indexing. + fn ensure_body_len(&self, needed: usize) -> Result<(), Error> { + if self.0.len() < needed { + return Err(Error::IncorrectOptionsSize { + needed, + available: self.0.len(), + }); + } + Ok(()) + } + /// Parse as IPv4 endpoint/multicast/SD option. /// Returns `(ip, protocol, port)`. /// @@ -370,6 +404,7 @@ impl<'a> OptionView<'a> { /// it is retained only to keep the API usable if an `OptionView` is ever constructed /// outside the validated parse path. pub fn as_ipv4(&self) -> Result<(Ipv4Addr, TransportProtocol, u16), Error> { + self.ensure_body_len(IPV4_OPTION_WIRE_SIZE)?; let ip = Ipv4Addr::from_bits(u32::from_be_bytes([ self.0[IPV4_OPTION_IP_OFFSET], self.0[IPV4_OPTION_IP_OFFSET + 1], @@ -395,6 +430,7 @@ impl<'a> OptionView<'a> { /// it is retained only to keep the API usable if an `OptionView` is ever constructed /// outside the validated parse path. pub fn as_ipv6(&self) -> Result<(Ipv6Addr, TransportProtocol, u16), Error> { + self.ensure_body_len(IPV6_OPTION_WIRE_SIZE)?; let mut octets = [0u8; 16]; octets.copy_from_slice(&self.0[IPV6_OPTION_IP_OFFSET..IPV6_OPTION_IP_END]); let ip = Ipv6Addr::from(octets); @@ -420,6 +456,7 @@ impl<'a> OptionView<'a> { /// /// Currently always succeeds; the `Result` return type is reserved for future validation. pub fn as_load_balancing(&self) -> Result<(u16, u16), Error> { + self.ensure_body_len(LOAD_BALANCING_OPTION_WIRE_SIZE)?; let priority = u16::from_be_bytes([ self.0[OPTION_PAYLOAD_OFFSET], self.0[OPTION_PAYLOAD_OFFSET + 1], @@ -491,6 +528,67 @@ impl<'a> OptionView<'a> { } } +impl<'a> Decode<'a> for OptionView<'a> { + type Error = crate::protocol::Error; + + /// Decode a single variable-length SD option from the front of `buf`. + /// + /// The stride comes from the option's 2-byte length field. This slices + /// only; it does NOT validate the option type, per-type length, or + /// transport-protocol byte. Validation is deferred to the accessors + /// (`option_type` / `as_ipv4` / `to_owned`) — the L2 validation pass — + /// keeping this a lazy zero-copy view. + /// + /// # Errors + /// + /// Returns [`Incomplete`](automotive_wire_codec::Incomplete) if fewer than + /// the fixed option header remains, or if the declared wire size exceeds + /// the remaining bytes, and + /// [`IncorrectOptionsSize`](Error::IncorrectOptionsSize) if the declared + /// wire size is smaller than the option header itself. + fn decode(buf: &'a [u8]) -> Result<(Self, &'a [u8]), Self::Error> { + ensure_len(buf, OPTION_HEADER_SIZE)?; + let length = u16::from_be_bytes([buf[0], buf[1]]); + let wire_size = usize::from(length) + OPTION_LENGTH_SIZE_DELTA; + // `wire_size` is `length + 3`, so a declared `length` below 1 produces + // a view shorter than the 4-byte header this function just required -- + // a view that contradicts its own header. `configuration_bytes` reads + // from `OPTION_PAYLOAD_OFFSET` unconditionally and would index past the + // end of such a view. Reject it here so no accessor can see one. + if wire_size < OPTION_HEADER_SIZE { + return Err(Error::IncorrectOptionsSize { + needed: OPTION_HEADER_SIZE, + available: wire_size, + } + .into()); + } + let (head, rest) = take(buf, wire_size)?; + Ok((OptionView(head), rest)) + } +} + +impl<'a> DecodeIter<'a> for OptionView<'a> { + type Error = crate::protocol::Error; + + // Variable stride (from the length field): keep the default WIRE_SIZE = None. + + /// Decode the next option, or `Ok(None)` at a clean end of buffer. + /// + /// A partial/truncated trailing option after a good start is surfaced as an + /// `Err` rather than silently dropped. + /// + /// # Errors + /// + /// Returns [`Incomplete`](automotive_wire_codec::Incomplete) if a partial + /// option remains after a good start. + fn decode_next(buf: &'a [u8]) -> Result, Self::Error> { + if buf.is_empty() { + return Ok(None); + } + Self::decode(buf).map(Some) + } +} + /// Iterator over variable-length SD options in a validated buffer. /// Options are guaranteed valid (validated upfront in `SdHeaderView::parse`). /// @@ -541,12 +639,18 @@ impl<'a> Iterator for OptionIter<'a> { /// protocol byte. pub(crate) fn validate_option(buf: &[u8]) -> Result { if buf.len() < OPTION_HEADER_SIZE { - return Err(Error::IncorrectOptionsSize(buf.len())); + return Err(Error::IncorrectOptionsSize { + needed: OPTION_HEADER_SIZE, + available: buf.len(), + }); } let length = u16::from_be_bytes([buf[0], buf[1]]); let wire_size = usize::from(length) + OPTION_LENGTH_SIZE_DELTA; if wire_size > buf.len() { - return Err(Error::IncorrectOptionsSize(buf.len())); + return Err(Error::IncorrectOptionsSize { + needed: wire_size, + available: buf.len(), + }); } let option_type_byte = buf[OPTION_TYPE_OFFSET]; let option_type = OptionType::try_from(option_type_byte)?; @@ -598,6 +702,82 @@ mod tests { use super::*; + // --- Under-length option views (PR #153 review, blocking finding) --- + // + // `decode` required OPTION_HEADER_SIZE (4) but then took + // `length + OPTION_LENGTH_SIZE_DELTA` (3), so a small declared `length` + // produced a view shorter than the header `decode` had just insisted on, + // and the accessors indexed it unconditionally. + + /// `length = 0` yields a 3-byte view — shorter than the 4-byte header + /// `decode` just required. `configuration_bytes` then indexed past its end. + #[test] + fn decode_rejects_a_wire_size_below_the_option_header() { + let buf = [0x00, 0x00, 0x01, 0x00]; + assert!( + matches!( + OptionView::decode(&buf), + Err(crate::protocol::Error::Sd( + Error::IncorrectOptionsSize { .. } + )) + ), + "a 3-byte view cannot satisfy the 4-byte option header", + ); + } + + /// The review's repro: `length = 2` gives a 5-byte view typed as IPv4 + /// Endpoint (0x04), which needs 12. + #[test] + fn as_ipv4_rejects_a_view_too_short_for_an_ipv4_option() { + let buf = [0x00, 0x02, 0x04, 0x00, 0x00]; + let (view, _) = OptionView::decode(&buf).expect("header-sized view decodes"); + assert!(matches!( + view.as_ipv4(), + Err(Error::IncorrectOptionsSize { .. }) + )); + } + + /// Same shape reached through the documented lazy path the review cites. + #[test] + fn to_owned_rejects_a_short_ipv4_option_instead_of_panicking() { + let buf = [0x00, 0x02, 0x04, 0x00, 0x00]; + let (view, _) = OptionView::decode(&buf).expect("header-sized view decodes"); + assert!(view.to_owned().is_err()); + } + + #[test] + fn as_ipv6_rejects_a_view_too_short_for_an_ipv6_option() { + let buf = [0x00, 0x02, 0x06, 0x00, 0x00]; + let (view, _) = OptionView::decode(&buf).expect("header-sized view decodes"); + assert!(matches!( + view.as_ipv6(), + Err(Error::IncorrectOptionsSize { .. }) + )); + } + + #[test] + fn as_load_balancing_rejects_a_view_too_short_for_the_option() { + let buf = [0x00, 0x02, 0x02, 0x00, 0x00]; + let (view, _) = OptionView::decode(&buf).expect("header-sized view decodes"); + assert!(matches!( + view.as_load_balancing(), + Err(Error::IncorrectOptionsSize { .. }) + )); + } + + /// A well-formed option must keep decoding — the guards must not reject + /// valid input. + #[test] + fn a_well_formed_ipv4_option_still_decodes() { + let mut buf = [0u8; IPV4_OPTION_WIRE_SIZE]; + buf[0..2].copy_from_slice(&IPV4_OPTION_LENGTH_FIELD.to_be_bytes()); + buf[OPTION_TYPE_OFFSET] = 0x04; + buf[IPV4_OPTION_PROTOCOL_OFFSET] = 0x11; + let (view, rest) = OptionView::decode(&buf).expect("valid option decodes"); + assert!(rest.is_empty()); + assert!(view.as_ipv4().is_ok()); + } + // --- TransportProtocol --- #[test] @@ -654,7 +834,7 @@ mod tests { fn round_trip(option: &Options) { let size = option.size(); let mut buf = [0u8; 4 + MAX_CONFIGURATION_STRING_LENGTH]; - let written = option.write(&mut &mut buf[..size]).unwrap(); + let written = option.encode(&mut &mut buf[..size]).unwrap(); assert_eq!(written, size); let view = OptionView(&buf[..size]); let parsed = view.to_owned().unwrap(); @@ -977,8 +1157,8 @@ mod tests { weight: 200, }; let mut buf = [0u8; 24]; // 12 + 8 = 20 - let n1 = opt1.write(&mut &mut buf[..12]).unwrap(); - let n2 = opt2.write(&mut &mut buf[12..20]).unwrap(); + let n1 = opt1.encode(&mut &mut buf[..12]).unwrap(); + let n2 = opt2.encode(&mut &mut buf[12..20]).unwrap(); let total = n1 + n2; let mut iter = OptionIter::new(&buf[..total]); @@ -1005,8 +1185,8 @@ mod tests { port: 30491, }; let mut buf = [0u8; 24]; - let n1 = opt1.write(&mut &mut buf[..12]).unwrap(); - let n2 = opt2.write(&mut &mut buf[12..24]).unwrap(); + let n1 = opt1.encode(&mut &mut buf[..12]).unwrap(); + let n2 = opt2.encode(&mut &mut buf[12..24]).unwrap(); let total = n1 + n2; let iter = OptionIter::new(&buf[..total]); @@ -1046,8 +1226,8 @@ mod tests { port: 30491, }; let mut buf = [0u8; 24]; - let n1 = opt1.write(&mut &mut buf[..12]).unwrap(); - let n2 = opt2.write(&mut &mut buf[12..24]).unwrap(); + let n1 = opt1.encode(&mut &mut buf[..12]).unwrap(); + let n2 = opt2.encode(&mut &mut buf[12..24]).unwrap(); let total = n1 + n2; let mut iter = OptionIter::new(&buf[..total]); @@ -1061,4 +1241,162 @@ mod tests { assert!(clone.next().is_none()); assert_eq!(remaining, opt2); } + + // --- Decode / DecodeIter (Phase 3 lazy L1) --- + + fn two_option_buf() -> ([u8; 24], usize, Options, Options) { + let opt1 = Options::IpV4Endpoint { + ip: Ipv4Addr::new(10, 0, 0, 1), + protocol: TransportProtocol::Udp, + port: 30490, + }; + let opt2 = Options::LoadBalancing { + priority: 100, + weight: 200, + }; + let mut buf = [0u8; 24]; + let n1 = opt1.encode(&mut &mut buf[..12]).unwrap(); + let n2 = opt2.encode(&mut &mut buf[12..20]).unwrap(); + (buf, n1 + n2, opt1, opt2) + } + + #[test] + fn decode_yields_option_and_remainder() { + let (buf, total, opt1, opt2) = two_option_buf(); + let (view, rest) = OptionView::decode(&buf[..total]).unwrap(); + assert_eq!(view.to_owned().unwrap(), opt1); + assert_eq!(rest.len(), 8); + let (view2, rest2) = OptionView::decode(rest).unwrap(); + assert_eq!(view2.to_owned().unwrap(), opt2); + assert!(rest2.is_empty()); + } + + #[test] + fn decode_short_header_is_incomplete() { + assert!(matches!( + OptionView::decode(&[0x00, 0x09, 0x04]), + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 4, + available: 3, + } + )) + )); + } + + #[test] + fn decode_truncated_body_is_incomplete() { + let (buf, _total, _opt1, _opt2) = two_option_buf(); + // A well-formed 12-byte IPv4 option header declaring 12 bytes, but + // only 8 present. + assert!(matches!( + OptionView::decode(&buf[..8]), + Err(crate::protocol::Error::Incomplete( + automotive_wire_codec::Incomplete { + needed: 12, + available: 8, + } + )) + )); + } + + #[test] + fn decode_iter_yields_all_then_none() { + let (buf, total, opt1, opt2) = two_option_buf(); + let mut iter = OptionView::iter(&buf[..total]); + assert_eq!(iter.next().unwrap().unwrap().to_owned().unwrap(), opt1); + assert_eq!(iter.next().unwrap().unwrap().to_owned().unwrap(), opt2); + assert!(iter.next().is_none()); + } + + #[test] + fn decode_iter_surfaces_truncated_tail_as_err() { + let (buf, total, _opt1, _opt2) = two_option_buf(); + // First option (12 bytes) is complete; chop the second short. + let mut iter = OptionView::iter(&buf[..total - 2]); + assert!(matches!(iter.next(), Some(Ok(_)))); + assert!(matches!( + iter.next(), + Some(Err(crate::protocol::Error::Incomplete(_))) + )); + assert!(iter.next().is_none()); + } + + #[test] + fn decode_iter_empty_is_immediately_none() { + let mut iter = OptionView::iter(&[]); + assert!(iter.next().is_none()); + } + + #[test] + fn decode_iter_variable_width_has_no_remaining_len() { + let (buf, total, _opt1, _opt2) = two_option_buf(); + let iter = OptionView::iter(&buf[..total]); + assert_eq!(iter.remaining_len(), None); + } + + #[test] + fn decode_does_not_validate_option_type() { + // Option type byte 0xFF is invalid, but decode only slices by length. + let buf: [u8; 4] = [0x00, 0x01, 0xFF, 0x00]; // length = 1, wire_size = 4 + let (view, rest) = OptionView::decode(&buf).unwrap(); + assert!(rest.is_empty()); + assert!(matches!( + view.option_type(), + Err(Error::InvalidOptionType(0xFF)) + )); + } + + // --- Encode size-exactness invariant --- + + #[test] + fn encoded_size_matches_bytes_written_for_each_variant() { + use automotive_wire_codec::CountingSink; + let mut config_string = heapless::Vec::::new(); + config_string.extend_from_slice(b"k=v").unwrap(); + let options = [ + Options::Configuration { + configuration_string: config_string, + }, + Options::LoadBalancing { + priority: 1, + weight: 2, + }, + Options::IpV4Endpoint { + ip: Ipv4Addr::new(10, 0, 0, 1), + protocol: TransportProtocol::Udp, + port: 30490, + }, + Options::IpV6Endpoint { + ip: Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 1), + protocol: TransportProtocol::Tcp, + port: 8080, + }, + ]; + for option in &options { + let mut sink = CountingSink::new(); + let written = option.encode(&mut sink).unwrap(); + assert_eq!(written, option.encoded_size().unwrap()); + assert_eq!(written, sink.count()); + } + } + + #[test] + fn encode_to_slice_too_small_yields_insufficient_buffer() { + use automotive_wire_codec::{EncodeToSliceError, InsufficientBuffer}; + let option = Options::IpV4Endpoint { + ip: Ipv4Addr::new(10, 0, 0, 1), + protocol: TransportProtocol::Udp, + port: 30490, + }; + let mut buf = [0u8; 4]; // needs 12 + let err = option.encode_to_slice(&mut buf).unwrap_err(); + assert!(matches!( + err, + EncodeToSliceError::InsufficientBuffer(InsufficientBuffer { + needed: 12, + available: 4, + }) + )); + } } diff --git a/src/protocol/sd/test_support.rs b/src/protocol/sd/test_support.rs index 557b06e..5d85ca2 100644 --- a/src/protocol/sd/test_support.rs +++ b/src/protocol/sd/test_support.rs @@ -1,5 +1,6 @@ use crate::protocol::sd; -use crate::traits::{PayloadWireFormat, WireFormat}; +use crate::traits::PayloadWireFormat; +use automotive_wire_codec::Encode; #[derive(Clone, Debug, Eq, PartialEq)] pub(crate) struct TestSdHeader { @@ -8,14 +9,13 @@ pub(crate) struct TestSdHeader { pub options: heapless::Vec, } -impl WireFormat for TestSdHeader { - fn required_size(&self) -> usize { - sd::Header::new(self.flags, &self.entries, &self.options).required_size() +impl Encode for TestSdHeader { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + sd::Header::new(self.flags, &self.entries, &self.options).encoded_size() } - fn encode( - &self, - writer: &mut T, - ) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { sd::Header::new(self.flags, &self.entries, &self.options).encode(writer) } } @@ -29,6 +29,18 @@ pub(crate) struct TestPayload { pub header: TestSdHeader, } +impl Encode for TestPayload { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + self.header.encoded_size() + } + + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + self.header.encode(writer) + } +} + impl PayloadWireFormat for TestPayload { type SdHeader = TestSdHeader; fn message_id(&self) -> crate::protocol::MessageId { @@ -71,15 +83,6 @@ impl PayloadWireFormat for TestPayload { fn sd_flags(&self) -> Option { Some(self.header.flags) } - fn required_size(&self) -> usize { - self.header.required_size() - } - fn encode( - &self, - writer: &mut T, - ) -> Result { - self.header.encode(writer) - } fn new_subscription_sd_header( service_id: u16, instance_id: u16, @@ -219,3 +222,87 @@ mod tests { assert!(p.offered_endpoints().is_empty()); } } + +/// An `SdHeader` whose `encoded_size` fails, modelling a downstream +/// [`PayloadWireFormat`] impl. +/// +/// No in-tree `SdHeader` can fail — `sd::Header::encoded_size` is +/// unconditionally `Ok(size)` — so the only way to exercise the error path +/// that the trait bound permits is to write an impl that takes it. That is +/// precisely the case the PR #153 review flagged: the bound does not require +/// infallibility, and `PayloadWireFormat` is public. +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct FailingSdHeader; + +impl Encode for FailingSdHeader { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + Err(crate::protocol::Error::Sd( + sd::Error::ConfigurationStringTooLong(usize::MAX), + )) + } + + fn encode(&self, _writer: &mut impl embedded_io::Write) -> Result { + Err(crate::protocol::Error::Sd( + sd::Error::ConfigurationStringTooLong(usize::MAX), + )) + } +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub(crate) struct FailingPayload { + pub header: FailingSdHeader, +} + +impl Encode for FailingPayload { + type Error = crate::protocol::Error; + + fn encoded_size(&self) -> Result { + self.header.encoded_size() + } + + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + self.header.encode(writer) + } +} + +impl PayloadWireFormat for FailingPayload { + type SdHeader = FailingSdHeader; + fn message_id(&self) -> crate::protocol::MessageId { + crate::protocol::MessageId::SD + } + fn as_sd_header(&self) -> Option<&FailingSdHeader> { + Some(&self.header) + } + fn from_payload_bytes( + _message_id: crate::protocol::MessageId, + _payload: &[u8], + ) -> Result { + Ok(Self { + header: FailingSdHeader, + }) + } + fn new_sd_payload(header: &FailingSdHeader) -> Self { + Self { + header: header.clone(), + } + } + fn sd_flags(&self) -> Option { + None + } + #[allow(clippy::too_many_arguments)] + fn new_subscription_sd_header( + _service_id: u16, + _instance_id: u16, + _major_version: u8, + _ttl: u32, + _event_group_id: u16, + _client_ip: core::net::Ipv4Addr, + _protocol: sd::TransportProtocol, + _client_port: u16, + _reboot_flag: sd::RebootFlag, + ) -> FailingSdHeader { + FailingSdHeader + } +} diff --git a/src/raw_payload.rs b/src/raw_payload.rs index 9cea01f..9f60437 100644 --- a/src/raw_payload.rs +++ b/src/raw_payload.rs @@ -12,7 +12,8 @@ use std::vec::Vec; use embedded_io::Error as _; use crate::protocol::{self, MessageId, sd}; -use crate::traits::{PayloadWireFormat, WireFormat}; +use crate::traits::PayloadWireFormat; +use automotive_wire_codec::Encode; /// Owned SD header backed by heap-allocated vectors. #[derive(Clone, Debug, Eq, PartialEq)] @@ -25,12 +26,14 @@ pub struct VecSdHeader { pub options: Vec, } -impl WireFormat for VecSdHeader { - fn required_size(&self) -> usize { - sd::Header::new(self.flags, &self.entries, &self.options).required_size() +impl Encode for VecSdHeader { + type Error = protocol::Error; + + fn encoded_size(&self) -> Result { + sd::Header::new(self.flags, &self.entries, &self.options).encoded_size() } - fn encode(&self, writer: &mut T) -> Result { + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { sd::Header::new(self.flags, &self.entries, &self.options).encode(writer) } } @@ -67,6 +70,29 @@ impl RawPayload { } } +impl Encode for RawPayload { + type Error = protocol::Error; + + fn encoded_size(&self) -> Result { + match &self.kind { + RawPayloadKind::Sd(header) => header.encoded_size(), + RawPayloadKind::Raw(bytes) => Ok(bytes.len()), + } + } + + fn encode(&self, writer: &mut impl embedded_io::Write) -> Result { + match &self.kind { + RawPayloadKind::Sd(header) => header.encode(writer), + RawPayloadKind::Raw(bytes) => { + writer + .write_all(bytes) + .map_err(|e| protocol::Error::Io(e.kind()))?; + Ok(bytes.len()) + } + } + } +} + impl PayloadWireFormat for RawPayload { type SdHeader = VecSdHeader; @@ -122,25 +148,6 @@ impl PayloadWireFormat for RawPayload { } } - fn required_size(&self) -> usize { - match &self.kind { - RawPayloadKind::Sd(header) => header.required_size(), - RawPayloadKind::Raw(bytes) => bytes.len(), - } - } - - fn encode(&self, writer: &mut T) -> Result { - match &self.kind { - RawPayloadKind::Sd(header) => header.encode(writer), - RawPayloadKind::Raw(bytes) => { - writer - .write_all(bytes) - .map_err(|e| protocol::Error::Io(e.kind()))?; - Ok(bytes.len()) - } - } - } - fn new_subscription_sd_header( service_id: u16, instance_id: u16, @@ -227,7 +234,7 @@ impl PayloadWireFormat for RawPayload { #[cfg(test)] mod tests { use super::*; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; use std::net::Ipv4Addr; fn make_sd_payload() -> RawPayload { @@ -330,13 +337,13 @@ mod tests { #[test] fn required_size_raw() { let p = make_raw_payload(); - assert_eq!(p.required_size(), 2); + assert_eq!(p.encoded_size().unwrap(), 2); } #[test] fn encode_raw_payload() { let p = make_raw_payload(); - let mut buf = std::vec![0u8; p.required_size()]; + let mut buf = std::vec![0u8; p.encoded_size().unwrap()]; let n = p.encode(&mut buf.as_mut_slice()).unwrap(); assert_eq!(n, 2); assert_eq!(&buf, &[0xDE, 0xAD]); @@ -345,9 +352,9 @@ mod tests { #[test] fn encode_sd_payload() { let p = make_sd_payload(); - let mut buf = std::vec![0u8; p.required_size()]; + let mut buf = std::vec![0u8; p.encoded_size().unwrap()]; let n = p.encode(&mut buf.as_mut_slice()).unwrap(); - assert_eq!(n, p.required_size()); + assert_eq!(n, p.encoded_size().unwrap()); } #[test] @@ -360,7 +367,7 @@ mod tests { &entries, &[], ); - let mut buf = std::vec![0u8; header.required_size()]; + let mut buf = std::vec![0u8; header.encoded_size().unwrap()]; header.encode(&mut buf.as_mut_slice()).unwrap(); let p = RawPayload::from_payload_bytes(MessageId::SD, &buf).unwrap(); @@ -528,7 +535,7 @@ mod tests { entries: std::vec![], options: std::vec![], }; - let size = header.required_size(); + let size = header.encoded_size().unwrap(); assert!(size > 0); let mut buf = std::vec![0u8; size]; let n = header.encode(&mut buf.as_mut_slice()).unwrap(); diff --git a/src/sd_codec.rs b/src/sd_codec.rs index 71cbab5..9f757e0 100644 --- a/src/sd_codec.rs +++ b/src/sd_codec.rs @@ -11,7 +11,9 @@ use core::net::{IpAddr, Ipv4Addr}; use core::sync::atomic::{AtomicU16, Ordering}; -use crate::WireFormat; +use automotive_wire_codec::{EncodeToSliceError, InsufficientBuffer}; + +use crate::Encode; use crate::protocol::sd::{ Entry, EventGroupEntry, Flags, Header as SdHeader, Options as SdOptions, OptionsCount, RebootFlag, SdHeaderView, ServiceEntry, TransportProtocol, @@ -69,12 +71,56 @@ pub struct SubscribeAckRequest { /// Packet-construction errors. #[derive(Debug, Clone, Copy, Eq, PartialEq)] pub enum BuildError { - /// `buf` is shorter than the encoded datagram. - BufferTooSmall, + /// `buf` is shorter than the encoded datagram. Carries the codec's + /// [`InsufficientBuffer`] so callers see the needed / available byte + /// counts, matching the rest of the crate's buffer-too-small reporting. + BufferTooSmall(InsufficientBuffer), + /// A fixed-capacity entry or option list could not take another element. + /// + /// Distinct from [`BufferTooSmall`](Self::BufferTooSmall), whose + /// [`InsufficientBuffer`] fields are documented in *bytes*. Element counts + /// were previously reported through that variant, which read as a byte + /// count and would have been actively misleading if it ever fired. + ListFull { + /// Elements the caller tried to place. + needed: usize, + /// Elements the fixed-capacity list can hold. + capacity: usize, + }, /// SD or SOME/IP encoding failed mid-write. EncodeFailed, } +impl core::fmt::Display for BuildError { + fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result { + match self { + BuildError::BufferTooSmall(ib) => write!(f, "buffer too small: {ib}"), + BuildError::ListFull { needed, capacity } => write!( + f, + "fixed-capacity list full: needed {needed} elements, capacity {capacity}" + ), + BuildError::EncodeFailed => f.write_str("encoding failed mid-write"), + } + } +} + +impl core::error::Error for BuildError {} + +impl From for BuildError { + fn from(ib: InsufficientBuffer) -> Self { + BuildError::BufferTooSmall(ib) + } +} + +impl From> for BuildError { + fn from(e: EncodeToSliceError) -> Self { + match e { + EncodeToSliceError::InsufficientBuffer(ib) => BuildError::BufferTooSmall(ib), + EncodeToSliceError::Encode(_) => BuildError::EncodeFailed, + } + } +} + /// Encode a `SubscribeEventgroup` datagram into `buf`. Returns its /// length in bytes. `session` should come from [`next_sd_session`]. /// @@ -191,16 +237,24 @@ fn build_multi_service_entry_datagram( } else { Entry::OfferService(svc) }; - entries - .push(entry) - .map_err(|_| BuildError::BufferTooSmall)?; + // `take(N)` bounds the loop to the vecs' capacity, so these pushes + // cannot actually fail; the mapping keeps the fallible signature honest + // and reports capacity as needed/available if that invariant is ever + // broken. + entries.push(entry).map_err(|_| BuildError::ListFull { + needed: requests.len(), + capacity: N, + })?; options .push(SdOptions::IpV4Endpoint { ip: req.local_ip, port: req.unicast_port, protocol: TransportProtocol::Udp, }) - .map_err(|_| BuildError::BufferTooSmall)?; + .map_err(|_| BuildError::ListFull { + needed: requests.len(), + capacity: N, + })?; } encode_sd_datagram(buf, &entries, &options, session, RebootFlag::Continuous) } @@ -268,21 +322,26 @@ fn encode_sd_datagram( session: u16, reboot: RebootFlag, ) -> Result { - if buf.len() < SOMEIP_HEADER_LEN { - return Err(BuildError::BufferTooSmall); - } - + // Header-first, body-second: `sd::Header::encoded_size()` is exact and + // computable without writing, so the SOME/IP length field is known before + // either section is written. One linear forward pass — no backfill. let sd_payload = SdHeader::new(Flags::new_sd(reboot), entries, options); let sd_payload_len = sd_payload - .encode_to_slice(&mut buf[SOMEIP_HEADER_LEN..]) + .encoded_size() .map_err(|_| BuildError::EncodeFailed)?; + let total = SOMEIP_HEADER_LEN + sd_payload_len; + if buf.len() < total { + return Err(BuildError::BufferTooSmall(InsufficientBuffer { + needed: total, + available: buf.len(), + })); + } let header = Header::new_sd(u32::from(session), sd_payload_len); - header - .encode_to_slice(&mut buf[..SOMEIP_HEADER_LEN]) - .map_err(|_| BuildError::EncodeFailed)?; + header.encode_to_slice(&mut buf[..SOMEIP_HEADER_LEN])?; + let written = sd_payload.encode_to_slice(&mut buf[SOMEIP_HEADER_LEN..total])?; - Ok(SOMEIP_HEADER_LEN + sd_payload_len) + Ok(SOMEIP_HEADER_LEN + written) } /// Build a SOME/IP notification (event) datagram into `buf`: the @@ -301,7 +360,10 @@ pub fn build_notification_datagram( ) -> Result { let total = SOMEIP_HEADER_LEN + payload.len(); if buf.len() < total { - return Err(BuildError::BufferTooSmall); + return Err(BuildError::BufferTooSmall(InsufficientBuffer { + needed: total, + available: buf.len(), + })); } #[allow(clippy::cast_possible_truncation)] let length_field: u32 = 8 + payload.len() as u32; @@ -336,7 +398,10 @@ pub fn encode_response_header( payload_len: usize, ) -> Result<(), BuildError> { if buf.len() < SOMEIP_HEADER_LEN { - return Err(BuildError::BufferTooSmall); + return Err(BuildError::BufferTooSmall(InsufficientBuffer { + needed: SOMEIP_HEADER_LEN, + available: buf.len(), + })); } let header = Header::new( MessageId::new_from_service_and_method(service_id, method_id), @@ -376,13 +441,19 @@ pub struct ParsedDatagram<'a> { pub payload: &'a [u8], } -/// Parse `data` as a SOME/IP datagram. Returns `None` if shorter than -/// [`SOMEIP_HEADER_LEN`] or [`HeaderView::parse`] rejects the header. -#[must_use] -pub fn parse_someip_datagram(data: &[u8]) -> Option> { - let (view, payload) = HeaderView::parse(data).ok()?; +/// Parse `data` as a SOME/IP datagram. +/// +/// # Errors +/// Propagates the [`HeaderView::parse`] error: [`protocol::Error::Incomplete`] +/// if `data` is shorter than a header can consume, or a validation error if the +/// header fields are malformed. This is the same error type used throughout the +/// crate, so callers can distinguish "not enough bytes yet" from "malformed". +/// +/// [`protocol::Error::Incomplete`]: crate::protocol::Error::Incomplete +pub fn parse_someip_datagram(data: &[u8]) -> Result, crate::protocol::Error> { + let (view, payload) = HeaderView::parse(data)?; let message_id = view.message_id(); - Some(ParsedDatagram { + Ok(ParsedDatagram { service_id: message_id.service_id(), method_id: message_id.method_id(), upper_header: view.upper_header_bytes(), @@ -391,15 +462,27 @@ pub fn parse_someip_datagram(data: &[u8]) -> Option> { } /// Parse `data` as a SOME/IP-SD datagram, returning the inner -/// [`SdHeaderView`] for entry/option iteration. `None` if the wrapper -/// fails to parse, the message-ID is not SD, or the SD payload is bad. -#[must_use] -pub fn parse_someip_sd_datagram(data: &[u8]) -> Option> { - let (view, sd_payload) = HeaderView::parse(data).ok()?; +/// [`SdHeaderView`] for entry/option iteration. +/// +/// # Errors +/// The three failure modes are distinguishable by variant: +/// - [`protocol::Error::Incomplete`] — the SOME/IP wrapper needs more bytes. +/// - [`protocol::Error::UnsupportedMessageID`] — a well-formed SOME/IP message +/// whose message-ID is not [`MessageId::SD`] (i.e. not an SD message). +/// - [`protocol::Error::Sd`] (and other validation variants) — the wrapper or +/// the inner SD payload is malformed. +/// +/// [`protocol::Error::Incomplete`]: crate::protocol::Error::Incomplete +/// [`protocol::Error::UnsupportedMessageID`]: crate::protocol::Error::UnsupportedMessageID +/// [`protocol::Error::Sd`]: crate::protocol::Error::Sd +pub fn parse_someip_sd_datagram(data: &[u8]) -> Result, crate::protocol::Error> { + let (view, sd_payload) = HeaderView::parse(data)?; if !view.is_sd() { - return None; + return Err(crate::protocol::Error::UnsupportedMessageID( + view.message_id(), + )); } - SdHeaderView::parse(sd_payload).ok() + SdHeaderView::parse(sd_payload) } /// Run an E2E check for `parsed` against `e2e`, keyed by `source`. Returns @@ -489,6 +572,118 @@ mod tests { assert_eq!(parsed.payload, &payload); } + #[test] + fn build_too_small_buffer_reports_needed_and_available() { + let request = SubscribeEventgroupRequest { + service_id: 0x0042, + instance_id: 1, + major_version: 1, + event_group_id: 1, + ttl: 3, + local_ip: Ipv4Addr::new(192, 0, 2, 2), + local_rx_port: 30600, + }; + // Enough for the SOME/IP header but not the SD body. + let mut buf = [0u8; SOMEIP_HEADER_LEN]; + let err = + build_subscribe_eventgroup_datagram(&mut buf, &request, 3, RebootFlag::Continuous) + .unwrap_err(); + match err { + BuildError::BufferTooSmall(ib) => { + assert_eq!(ib.available, SOMEIP_HEADER_LEN); + assert!(ib.needed > ib.available); + } + other => panic!("expected BufferTooSmall, got {other:?}"), + } + } + + #[test] + fn parse_someip_datagram_incomplete_is_error() { + // Fewer than SOMEIP_HEADER_LEN bytes -> Incomplete, not a silent None. + let err = parse_someip_datagram(&[0u8; 4]).unwrap_err(); + assert!(matches!(err, crate::protocol::Error::Incomplete(_))); + } + + #[test] + fn parse_sd_datagram_incomplete_vs_not_sd_vs_malformed() { + // 1. Incomplete: too few bytes for even the SOME/IP header. + let err = parse_someip_sd_datagram(&[0u8; 4]).unwrap_err(); + assert!(matches!(err, crate::protocol::Error::Incomplete(_))); + + // 2. Not SD: a well-formed non-SD notification datagram. + let mut buf = [0u8; 64]; + let len = build_notification_datagram(&mut buf, 0x0003, 0x8001, 9, &[1, 2, 3]).unwrap(); + let err = parse_someip_sd_datagram(&buf[..len]).unwrap_err(); + assert!(matches!( + err, + crate::protocol::Error::UnsupportedMessageID(_) + )); + + // 3. Truncated SD datagram: the SD message-ID is present but the + // payload is chopped short. This is an error, and crucially it is + // NOT reported as the not-SD (`UnsupportedMessageID`) case — the + // three failure modes stay distinguishable. + let request = SubscribeEventgroupRequest { + service_id: 0x0042, + instance_id: 1, + major_version: 1, + event_group_id: 1, + ttl: 3, + local_ip: Ipv4Addr::new(192, 0, 2, 2), + local_rx_port: 30600, + }; + let mut sd_buf = [0u8; 128]; + let sd_len = + build_subscribe_eventgroup_datagram(&mut sd_buf, &request, 3, RebootFlag::Continuous) + .unwrap(); + let truncated = &sd_buf[..sd_len - 4]; + let err = parse_someip_sd_datagram(truncated).unwrap_err(); + assert!( + !matches!(err, crate::protocol::Error::UnsupportedMessageID(_)), + "truncated SD datagram must not surface as not-SD" + ); + } + + #[test] + fn parse_sd_datagram_structurally_invalid_entry_is_sd_error() { + // Length-consistent SD datagram (unlike the truncation case above, + // which short-circuits as `Incomplete` before `SdHeaderView::parse` + // ever reaches the entry walk): corrupt the first entry's type byte + // to an unrecognized value while leaving every length field intact, + // so parsing gets past the `Incomplete` checks and hits + // `EntryView::entry_type()`'s validation, surfacing + // `protocol::Error::Sd(sd::Error::InvalidEntryType(_))`. + let request = SubscribeEventgroupRequest { + service_id: 0x0042, + instance_id: 1, + major_version: 1, + event_group_id: 1, + ttl: 3, + local_ip: Ipv4Addr::new(192, 0, 2, 2), + local_rx_port: 30600, + }; + let mut sd_buf = [0u8; 128]; + let sd_len = + build_subscribe_eventgroup_datagram(&mut sd_buf, &request, 3, RebootFlag::Continuous) + .unwrap(); + + // Byte layout: 16-byte SOME/IP header, then flags/reserved(4) + + // entries_size(4), then the entries array (options_size + options + // follow after the entries). The first entry's type byte is + // therefore at offset 24. + let entry_type_offset: usize = 16 + 8; + sd_buf[entry_type_offset] = 0xFF; + + let err = parse_someip_sd_datagram(&sd_buf[..sd_len]).unwrap_err(); + assert!( + matches!( + err, + crate::protocol::Error::Sd(crate::protocol::sd::Error::InvalidEntryType(0xFF)) + ), + "expected Sd(InvalidEntryType(0xFF)), got {err:?}" + ); + } + #[test] fn subscribe_builder_honors_reboot_flag() { let request = SubscribeEventgroupRequest { diff --git a/src/server/event_publisher.rs b/src/server/event_publisher.rs index 1ce1ac2..98d8a62 100644 --- a/src/server/event_publisher.rs +++ b/src/server/event_publisher.rs @@ -6,10 +6,9 @@ use super::subscription_manager::{SUBSCRIBERS_PER_GROUP, SubscriptionHandle}; use crate::CapacityKind; use crate::e2e::E2EKey; use crate::protocol::{Header, Message}; -use crate::traits::{PayloadWireFormat, WireFormat}; +use crate::traits::PayloadWireFormat; use crate::transport::{E2ERegistryHandle, SharedHandle, TransportSocket}; -#[cfg(test)] -use alloc::sync::Arc; +use automotive_wire_codec::Encode; use core::marker::PhantomData; use core::net::SocketAddrV4; use heapless::Vec as HeaplessVec; @@ -194,7 +193,7 @@ where // `encode_to_slice` report a less-actionable protocol I/O error // when it runs out of buffer. Matches the raw-event path below // and the client socket_manager path. - let required_size = message.required_size(); + let required_size = message.encoded_size()?; if required_size > msg_buf.len() { crate::log::error!( "Message size ({} bytes) exceeds msg_buf.len() ({}); dropping publish", @@ -207,7 +206,9 @@ where // Serialize the message into the caller-provided buffer. // (PR-3 #125 change: no longer uses an in-future `[u8; UDP_BUFFER_SIZE]`; // the caller decides the buffer size and lifetime.) - let mut message_length = message.encode_to_slice(msg_buf)?; + let mut message_length = message + .encode_to_slice(msg_buf) + .map_err(crate::protocol::Error::from)?; // Apply E2E protect if configured. `protected_buf` is disjoint from // `msg_buf`, so we can read the unprotected payload directly out of @@ -225,6 +226,22 @@ where protected_buf, ); match result { + // Post-hoc length backfill (intentional, not an `Encode` gap): + // the SOME/IP length field at `msg_buf[4..8]` was already + // written by `encode_to_slice` above, but E2E protection + // changes the payload's size (header + CRC overhead), so + // the final on-wire length isn't known until *after* + // `protect` runs. A single-pass `Encode` impl cannot + // express "go back and rewrite bytes already emitted + // based on bytes written later" — that's exactly the + // size-changing, post-hoc transform the codec's README + // scopes out of `Encode`, recommending a two-phase + // consumer-owned API instead. That two-phase API is + // protect/check, which is why E2E stays off the + // `Encode`/`Decode` traits rather than being forced onto + // them. This rewrite does not change any on-wire bytes + // that `Encode` would have produced without E2E; it only + // patches the length field to reflect the protected size. Some(Ok(protected_len)) => { if 16 + protected_len > msg_buf.len() { crate::log::error!( @@ -439,7 +456,9 @@ where // Serialize header + payload into the caller-provided buffer. // (PR-3 #125 change: no longer uses an in-future `[u8; UDP_BUFFER_SIZE]`.) - let header_len = header.encode_to_slice(buf)?; + let header_len = header + .encode_to_slice(buf) + .map_err(crate::protocol::Error::from)?; let Some(total_len) = header_len.checked_add(payload.len()) else { crate::log::error!( "raw event length computation overflowed usize (header_len={}, payload.len()={}); dropping publish", @@ -742,7 +761,9 @@ where payload.len(), ); - let header_len = header.encode_to_slice(buf)?; + let header_len = header + .encode_to_slice(buf) + .map_err(crate::protocol::Error::from)?; let Some(total_len) = header_len.checked_add(payload.len()) else { crate::log::error!( "raw event length computation overflowed usize (header_len={}, payload.len()={}); dropping publish", @@ -868,7 +889,7 @@ mod tests { use crate::server::SubscriptionManager; use crate::tokio_transport::TokioSocket; use std::net::{Ipv4Addr, SocketAddrV4}; - use std::sync::Mutex; + use std::sync::{Arc, Mutex}; use std::vec; use std::vec::Vec; use tokio::net::UdpSocket; @@ -915,7 +936,7 @@ mod tests { } fn make_test_message() -> Message { - Message::new_sd(0x0001, &empty_sd_header()) + Message::new_sd(0x0001, &empty_sd_header()).expect("in-tree SdHeader sizing is infallible") } #[tokio::test] @@ -1182,7 +1203,7 @@ mod tests { ); let message = Message::new(header, payload); assert!( - message.required_size() > UDP_BUFFER_SIZE, + message.encoded_size().unwrap() > UDP_BUFFER_SIZE, "fixture must exceed cap", ); @@ -1244,7 +1265,7 @@ mod tests { ); let message = Message::new(header, payload); assert!( - message.required_size() <= UDP_BUFFER_SIZE, + message.encoded_size().unwrap() <= UDP_BUFFER_SIZE, "fixture's raw size must fit the cap so the pre-encode check passes and \ we actually exercise the post-protect guard", ); diff --git a/src/server/mod.rs b/src/server/mod.rs index eb33cb2..bba61a4 100644 --- a/src/server/mod.rs +++ b/src/server/mod.rs @@ -30,8 +30,6 @@ use crate::Timer; use crate::e2e::{E2EKey, E2EProfile}; #[cfg(feature = "_alloc")] use crate::protocol::sd; -#[cfg(test)] -use crate::protocol::sd::{Entry, Flags, ServiceEntry}; #[cfg(feature = "_alloc")] use crate::transport::SocketOptions; #[cfg(feature = "_alloc")] @@ -42,8 +40,6 @@ use alloc::sync::Arc; use core::net::Ipv4Addr; #[cfg(feature = "_alloc")] use core::net::SocketAddrV4; -#[cfg(test)] -use std::vec::Vec; #[cfg(feature = "server-tokio")] use crate::e2e::E2ERegistry; @@ -1708,14 +1704,16 @@ where #[cfg(all(test, feature = "server-tokio"))] mod tests { use super::*; + use crate::protocol::sd::{Entry, Flags, ServiceEntry}; use crate::protocol::{ Header as SomeIpHeader, MessageType, MessageTypeField, MessageView, ReturnCode, }; use crate::tokio_transport::{TokioTimer, TokioTransport}; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; use std::format; use std::net::IpAddr; use std::vec; + use std::vec::Vec; use tokio::net::UdpSocket; /// Type alias bringing the tokio-flavor concrete type parameters back @@ -3008,13 +3006,13 @@ mod tests { ) -> usize { let opt = sd::Options::IpV4Endpoint { ip, protocol, port }; let mut slot = buf; - opt.write(&mut slot).unwrap() + opt.encode(&mut slot).unwrap() } fn write_load_balancing_option(buf: &mut [u8], priority: u16, weight: u16) -> usize { let opt = sd::Options::LoadBalancing { priority, weight }; let mut slot = buf; - opt.write(&mut slot).unwrap() + opt.encode(&mut slot).unwrap() } /// Build a byte buffer holding `count` `IpV4Endpoint` options with diff --git a/src/server/runtime.rs b/src/server/runtime.rs index 4bda216..49c3151 100644 --- a/src/server/runtime.rs +++ b/src/server/runtime.rs @@ -42,7 +42,7 @@ where T: TransportSocket, { use crate::protocol::Header as SomeIpHeader; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; let entry = Entry::OfferService(ServiceEntry { index_first_options_run: 0, @@ -111,7 +111,7 @@ where T: TransportSocket, { use crate::protocol::Header as SomeIpHeader; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; let ack_entry = Entry::SubscribeAckEventGroup(sd::EventGroupEntry { index_first_options_run: 0, @@ -177,7 +177,7 @@ where T: TransportSocket, { use crate::protocol::Header as SomeIpHeader; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; let nack_entry = Entry::SubscribeAckEventGroup(sd::EventGroupEntry { index_first_options_run: 0, @@ -1024,7 +1024,7 @@ mod tests { /// Encode a minimal Subscribe SD payload and return `(wire_bytes, sd_len)` so /// callers can parse an `SdHeaderView` and extract an `EntryView`. fn subscribe_wire_bytes() -> ([u8; 512], usize) { - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; let entry = sd::Entry::SubscribeEventGroup(sd::EventGroupEntry { index_first_options_run: 0, diff --git a/src/server/sd_state.rs b/src/server/sd_state.rs index 14c5b46..17068ab 100644 --- a/src/server/sd_state.rs +++ b/src/server/sd_state.rs @@ -199,7 +199,7 @@ impl SdStateManager { socket: &T, ) -> Result<(), Error> { use crate::protocol::Header as SomeIpHeader; - use crate::traits::WireFormat; + use automotive_wire_codec::Encode; let entry = Entry::OfferService(ServiceEntry { index_first_options_run: 0, diff --git a/src/traits.rs b/src/traits.rs index b6293c8..bf3290b 100644 --- a/src/traits.rs +++ b/src/traits.rs @@ -18,54 +18,67 @@ pub struct OfferedEndpoint { pub is_offer: bool, } -/// A trait for types that can be serialized to a [`Writer`](embedded_io::Write). +/// Crate-local conveniences over any codec [`Encode`](automotive_wire_codec::Encode) type. /// -/// `WireFormat` acts as the base trait for all types that can be serialized -/// as part of the Simple SOME/IP ecosystem. Decoding is handled by zero-copy -/// view types (`HeaderView`, `MessageView`, etc.) instead of this trait. -pub trait WireFormat: Send + Sized + Sync { - /// Returns the number of bytes required to serialize this value. - fn required_size(&self) -> usize; - - /// Serialize a value to a byte stream. - /// Returns the number of bytes written. - /// # Errors - /// - If the data cannot be written to the stream - fn encode(&self, writer: &mut T) -> Result; - - /// Encode into a byte slice, returning the number of bytes written. - /// - /// # Errors - /// Returns an error if `buf` is too small (requires at least - /// [`required_size()`](Self::required_size) bytes). - fn encode_to_slice(&self, buf: &mut [u8]) -> Result { - // `embedded_io::Write` is implemented for `&mut [u8]` (the writer - // advances the slice), so the writer passed to `encode` is a - // reborrow of `buf` — named to avoid a `&mut &mut` expression. - let mut writer: &mut [u8] = buf; - self.encode(&mut writer) - } - +/// The codec's `Encode` trait provides `encode`, `encoded_size`, and +/// `encode_to_slice`; this extension adds the heap-allocating +/// `encode_to_vec` helper for `std` builds. It is blanket-implemented for +/// every `Encode` type, so bringing it into scope makes `encode_to_vec` +/// available anywhere. +pub trait EncodeExt: automotive_wire_codec::Encode { /// Encode into a newly allocated `Vec`. /// /// # Errors /// Returns an error if encoding fails. #[cfg(feature = "std")] - fn encode_to_vec(&self) -> Result, protocol::Error> { - let mut buf = std::vec![0u8; self.required_size()]; - self.encode_to_slice(&mut buf)?; + fn encode_to_vec(&self) -> Result, Self::Error> { + let mut buf = std::vec![0u8; self.encoded_size()?]; + let mut cursor: &mut [u8] = &mut buf; + self.encode(&mut cursor)?; Ok(buf) } } +impl EncodeExt for T {} + /// A trait for SOME/IP Payload types that can be serialized to a /// [`Writer`](embedded_io::Write) and constructed from raw payload bytes. /// +/// The encode side is provided by the [`Encode`](automotive_wire_codec::Encode) +/// supertrait (`encoded_size` + `encode`); implementors get `encode_to_slice` +/// and — under `std` — the crate's [`EncodeExt`]`::encode_to_vec` for free. +/// /// Note that SOME/IP payloads are not self identifying, so the [Message ID](protocol::MessageId) -/// must be provided by the caller. -pub trait PayloadWireFormat: core::fmt::Debug + Send + Sized + Sync { +/// must be provided by the caller: `Encode` alone cannot reconstruct a payload +/// from bytes, which is why [`from_payload_bytes`](Self::from_payload_bytes) +/// remains an inherent requirement. +pub trait PayloadWireFormat: + automotive_wire_codec::Encode + core::fmt::Debug + Send + Sized + Sync +{ /// The SD header type used by this payload implementation. - type SdHeader: WireFormat + Clone + core::fmt::Debug + Eq; + // `Send + Sync` used to come for free from this trait's predecessor's own + // `Send + Sync` supertrait (removed in the codec migration). The codec's + // `Encode` has no such supertrait, but the client's channel-carried types + // (`DiscoveryMessage`, + // `ClientUpdate`, `ControlMessage`) embed `SdHeader` and flow through + // `Send`-bounded channels, so the bound is pervasive rather than + // localized. Restate it here on the associated type (all concrete + // `SdHeader` types — `VecSdHeader`, `HeaplessSdHeader`, `sd::Header<'a>`, + // and the test header — are plain owned/borrowed structs that are auto + // `Send + Sync`), instead of threading a `where` clause through every + // client type definition and impl. + // `Error = protocol::Error` matches the bound this trait already puts on + // `Self`. Without it the associated error type is opaque, which is what + // pushed `Message::new_sd` into swallowing a failed `encoded_size` with + // `unwrap_or(0)` -- it could not name the error to propagate it. Every + // concrete `SdHeader` already uses `protocol::Error`, so this costs + // nothing in tree and closes the hole for downstream impls. + type SdHeader: automotive_wire_codec::Encode + + Clone + + core::fmt::Debug + + Eq + + Send + + Sync; /// Get the Message ID for the payload fn message_id(&self) -> MessageId; @@ -80,14 +93,6 @@ pub trait PayloadWireFormat: core::fmt::Debug + Send + Sized + Sync { fn new_sd_payload(header: &Self::SdHeader) -> Self; /// Return the SD flags if this payload is a service discovery message. fn sd_flags(&self) -> Option; - /// Number of bytes required to write the payload - fn required_size(&self) -> usize; - /// Serialize the payload to a [Writer](embedded_io::Write) - /// - /// # Errors - /// - /// Returns an error if the payload cannot be written to the writer. - fn encode(&self, writer: &mut T) -> Result; /// Construct an SD header for subscribing to an event group. #[allow(clippy::too_many_arguments)] diff --git a/tests/bare_metal_e2e.rs b/tests/bare_metal_e2e.rs index e7f3c09..15e0f26 100644 --- a/tests/bare_metal_e2e.rs +++ b/tests/bare_metal_e2e.rs @@ -26,9 +26,9 @@ use core::time::Duration; use std::collections::VecDeque; use std::sync::{Arc, Mutex, RwLock}; +use simple_someip::Encode; use simple_someip::PayloadWireFormat; use simple_someip::ServiceEndpointKey; -use simple_someip::WireFormat; use simple_someip::client::Error as ClientError; use simple_someip::client::{ClientUpdate, ControlMessage, ReceivedMessage, SendMessage}; use simple_someip::define_static_channels; @@ -859,7 +859,8 @@ async fn inbound_datagram_larger_than_claimed_buffer_is_dropped_not_fatal() { rx.push(vec![0xFFu8; BUF_LEN], 256, source); // Then a valid small SD message that fits the 64-byte buffer. - let sd_msg = Message::::new_sd(1, &empty_vec_sd_header()); + let sd_msg = Message::::new_sd(1, &empty_vec_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let mut wire = vec![0u8; BUF_LEN]; let len = sd_msg.encode(&mut wire.as_mut_slice()).expect("encode sd"); assert!( diff --git a/tests/client_server.rs b/tests/client_server.rs index a6930cd..d554d6d 100644 --- a/tests/client_server.rs +++ b/tests/client_server.rs @@ -205,7 +205,8 @@ async fn test_client_server_subscribe_and_receive_event() { let _ = tokio::time::timeout(std::time::Duration::from_millis(250), updates.recv()).await; // Publish an event from the server to the client's unicast port - let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); + let event_msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let sent = publisher .publish_event(service_id, 1, 0x01, &event_msg) .await @@ -349,7 +350,8 @@ async fn test_add_endpoint_and_send_to_service() { let _ = tokio::time::timeout(std::time::Duration::from_millis(250), updates.recv()).await; // Publish an event from the server - let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); + let event_msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let sent = publisher .publish_event(service_id, 1, 0x01, &event_msg) .await @@ -371,7 +373,8 @@ async fn test_add_endpoint_and_send_to_service() { )) .await .unwrap(); - let msg = Message::::new_sd(0x0001, &empty_sd_header()); + let msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let result = client .send_to_service( ServiceEndpointKey::udp(service_id, SocketAddr::V4(server_addr)), @@ -431,7 +434,8 @@ async fn test_subscribe_auto_binds_discovery() { let _ = tokio::time::timeout(std::time::Duration::from_millis(250), updates.recv()).await; // Publish an event and verify the client can receive it - let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); + let event_msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let sent = publisher .publish_event(service_id, 1, 0x01, &event_msg) .await @@ -489,7 +493,8 @@ async fn test_client_request_resolves_via_unicast_reply() { // send_to_service creates a PendingResponse; the server will send the event // which has a matching request_id, resolving it. - let msg = Message::::new_sd(0x0001, &empty_sd_header()); + let msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let pending = client .send_to_service( ServiceEndpointKey::udp(service_id, SocketAddr::V4(server_addr)), @@ -499,7 +504,8 @@ async fn test_client_request_resolves_via_unicast_reply() { .expect("send_to_service failed"); // Publish an event that the client unicast socket will receive - let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); + let event_msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); publisher .publish_event(service_id, 1, 0x01, &event_msg) .await @@ -690,7 +696,8 @@ async fn test_multiple_subscribers_receive_events() { let _ = tokio::time::timeout(std::time::Duration::from_millis(250), updates2.recv()).await; // Publish event - let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); + let event_msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let sent = publisher .publish_event(service_id, 1, 0x01, &event_msg) .await @@ -904,7 +911,8 @@ async fn test_two_devices_same_service_instance_addressed_independently() { ); // Publish from A: the client must receive an event sourced from A, not B. - let event_msg = Message::::new_sd(0x0001, &empty_sd_header()); + let event_msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let sent_a = publisher_a .publish_event(service_id, 1, 0x01, &event_msg) .await @@ -945,7 +953,8 @@ async fn test_two_devices_same_service_instance_addressed_independently() { .await .unwrap(); - let msg = Message::::new_sd(0x0001, &empty_sd_header()); + let msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let result_a = client .send_to_service( ServiceEndpointKey::udp(service_id, SocketAddr::V4(addr_a)), @@ -957,7 +966,8 @@ async fn test_two_devices_same_service_instance_addressed_independently() { "expected ServiceNotFound for removed device A, got {result_a:?}" ); - let msg = Message::::new_sd(0x0001, &empty_sd_header()); + let msg = Message::::new_sd(0x0001, &empty_sd_header()) + .expect("in-tree SdHeader sizing is infallible"); let result_b = client .send_to_service( ServiceEndpointKey::udp(service_id, SocketAddr::V4(addr_b)), diff --git a/tests/wire_golden.rs b/tests/wire_golden.rs new file mode 100644 index 0000000..0f6a7d0 --- /dev/null +++ b/tests/wire_golden.rs @@ -0,0 +1,673 @@ +//! Phase 0 of the automotive-wire-codec migration: byte-exact "golden" +//! wire snapshots. +//! +//! Every test in this file asserts encoder output against a **hard-coded +//! `&[u8]` literal** derived by hand from the SOME/IP / SOME/IP-SD wire +//! layout — never a re-encode of the same value compared to itself. The +//! literal is the oracle. Each literal is cross-checked against the +//! current encoder (that's what running the test does); if a hand +//! derivation ever disagrees with the encoder, that is a latent wire bug +//! to report, not something to "fix" by copying encoder output into the +//! literal. +//! +//! These tests must keep passing, byte-for-byte, through every later +//! phase of the automotive-wire-codec migration — that is their entire +//! purpose. +//! +//! This suite protected the Phase 2 fix of a historical entry-size bug +//! (see `src/protocol/sd/entry.rs`): the former `Entry::required_size()` +//! returned 17 and `ServiceEntry`/`EventGroupEntry::encode` returned +//! `Ok(16)` while only writing 15 bytes — i.e. the *returned counts* +//! overstated the true 16-byte wire size by one. That mismatch never +//! affected the bytes actually placed on the wire, so no golden literal +//! below ever needed to change (all literals reflect the true 16-byte +//! on-wire entry size); the bug was in the returned `usize`, which was +//! corrected in Phase 2 with these golden tests as the regression guard. + +use simple_someip::Encode; +use simple_someip::protocol::sd::{ + Entry, EventGroupEntry, Flags, Header as SdHeader, OptionType, Options, OptionsCount, + RebootFlag, ServiceEntry, TransportProtocol, +}; +use simple_someip::protocol::{Header, MessageId, MessageType, MessageTypeField, ReturnCode}; + +// =========================================================================== +// 1. SOME/IP `Header` +// =========================================================================== + +#[test] +fn header_request_golden_bytes() { + // service_id 0x1234, method_id 0x0001, no payload (length = 8), + // request_id 0xABCD0042, protocol_version 0x01, interface_version 0x03, + // message_type Request/no-TP (0x00), return_code Ok (0x00). + let header = Header::new( + MessageId::new_from_service_and_method(0x1234, 0x0001), + 0xABCD_0042, + 0x01, + 0x03, + MessageTypeField::new(MessageType::Request, false), + ReturnCode::Ok, + 0, + ); + let mut buf = [0u8; 16]; + let n = header.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 16); + + #[rustfmt::skip] + let expected: [u8; 16] = [ + 0x12, 0x34, 0x00, 0x01, // message_id: service 0x1234, method 0x0001 + 0x00, 0x00, 0x00, 0x08, // length = 8 (payload_len 0 + 8) + 0xAB, 0xCD, 0x00, 0x42, // request_id + 0x01, // protocol_version + 0x03, // interface_version + 0x00, // message_type: Request, no TP + 0x00, // return_code: Ok + ]; + assert_eq!(buf, expected); +} + +#[test] +fn header_response_tp_generic_error_golden_bytes() { + // A second header value exercising the Response wire byte (0x80, + // NOT the enum discriminant), the TP flag, a non-Ok return code, + // and a non-zero payload length feeding the length field. + let header = Header::new( + MessageId::new_from_service_and_method(0x005B, 0x8001), + 0x0000_0007, + 0x01, + 0x02, + MessageTypeField::new(MessageType::Response, true), + ReturnCode::GenericError(0x15), + 10, + ); + let mut buf = [0u8; 16]; + header.encode_to_slice(&mut buf).unwrap(); + + #[rustfmt::skip] + let expected: [u8; 16] = [ + 0x00, 0x5B, 0x80, 0x01, // message_id: service 0x005B, method 0x8001 + 0x00, 0x00, 0x00, 0x12, // length = 18 (payload_len 10 + 8) + 0x00, 0x00, 0x00, 0x07, // request_id + 0x01, // protocol_version + 0x02, // interface_version + 0xA0, // message_type: Response (0x80) | TP flag (0x20) + 0x15, // return_code: GenericError(0x15) + ]; + assert_eq!(buf, expected); +} + +// =========================================================================== +// 2. `Message

` with a raw/opaque payload +// =========================================================================== + +#[cfg(feature = "std")] +mod message_golden { + use simple_someip::PayloadWireFormat; + use simple_someip::RawPayload; + use simple_someip::protocol::Message; + + use super::{Encode, Header, MessageId, MessageType, MessageTypeField, ReturnCode}; + + #[test] + fn message_raw_payload_golden_bytes() { + // Non-SD message: service 0x005B, method 0x0001, 4-byte opaque + // payload. header(16) + payload(4) = 20 bytes total. + let payload_bytes = [0xDE, 0xAD, 0xBE, 0xEF]; + let message_id = MessageId::new_from_service_and_method(0x005B, 0x0001); + let header = Header::new( + message_id, + 0x0000_0001, + 0x01, + 0x01, + MessageTypeField::new(MessageType::Request, false), + ReturnCode::Ok, + payload_bytes.len(), + ); + let payload = RawPayload::from_payload_bytes(message_id, &payload_bytes).unwrap(); + let message = Message::new(header, payload); + + let mut buf = [0u8; 20]; + let n = message.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 20); + + #[rustfmt::skip] + let expected: [u8; 20] = [ + 0x00, 0x5B, 0x00, 0x01, // message_id: service 0x005B, method 0x0001 + 0x00, 0x00, 0x00, 0x0C, // length = 12 (payload_len 4 + 8) + 0x00, 0x00, 0x00, 0x01, // request_id + 0x01, // protocol_version + 0x01, // interface_version + 0x00, // message_type: Request, no TP + 0x00, // return_code: Ok + 0xDE, 0xAD, 0xBE, 0xEF, // opaque payload bytes + ]; + assert_eq!(buf, expected); + } +} + +// =========================================================================== +// 3. SD `Header` — one composite per `EntryType`, one test per `OptionType` +// =========================================================================== + +#[test] +fn sd_header_find_service_golden_bytes() { + // FindService entry with vsomeip-standard wildcard instance/version + // fields, no options. + let entries = [Entry::FindService(ServiceEntry::find(0x1234))]; + let sd_header = SdHeader::new(Flags::new_sd(RebootFlag::RecentlyRebooted), &entries, &[]); + + let mut buf = [0u8; 28]; + let n = sd_header.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 28); + + #[rustfmt::skip] + let expected: [u8; 28] = [ + 0xC0, 0x00, 0x00, 0x00, // flags (reboot|unicast) + 3 reserved bytes + 0x00, 0x00, 0x00, 0x10, // entries_size = 16 (1 entry) + // --- FindService entry (16 bytes) --- + 0x00, // entry type: FindService + 0x00, 0x00, // index_first/second_options_run + 0x10, // options_count: first=1, second=0 + 0x12, 0x34, // service_id + 0xFF, 0xFF, // instance_id: wildcard + 0xFF, // major_version: wildcard + 0xFF, 0xFF, 0xFF, // ttl: wildcard (0x00FFFFFF) + 0xFF, 0xFF, 0xFF, 0xFF, // minor_version: wildcard + 0x00, 0x00, 0x00, 0x00, // options_size = 0 + ]; + assert_eq!(buf, expected); +} + +#[test] +fn sd_header_offer_service_with_ipv4_endpoint_golden_bytes() { + // OfferService entry referencing one IPv4Endpoint option. + let entry = Entry::OfferService(ServiceEntry { + index_first_options_run: 0, + index_second_options_run: 0, + options_count: OptionsCount::new(1, 0), + service_id: 0x1234, + instance_id: 0x0001, + major_version: 1, + ttl: 0x00FF_FFFF, + minor_version: 0, + }); + let option = Options::IpV4Endpoint { + ip: core::net::Ipv4Addr::new(192, 168, 1, 10), + protocol: TransportProtocol::Udp, + port: 30509, + }; + let entries = [entry]; + let options = [option]; + let sd_header = SdHeader::new( + Flags::new_sd(RebootFlag::RecentlyRebooted), + &entries, + &options, + ); + + let mut buf = [0u8; 40]; + let n = sd_header.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 40); + + #[rustfmt::skip] + let expected: [u8; 40] = [ + 0xC0, 0x00, 0x00, 0x00, // flags + reserved + 0x00, 0x00, 0x00, 0x10, // entries_size = 16 + // --- OfferService entry (16 bytes) --- + 0x01, // entry type: OfferService + 0x00, 0x00, // index_first/second_options_run + 0x10, // options_count: first=1, second=0 + 0x12, 0x34, // service_id + 0x00, 0x01, // instance_id + 0x01, // major_version + 0xFF, 0xFF, 0xFF, // ttl = 0x00FFFFFF + 0x00, 0x00, 0x00, 0x00, // minor_version + 0x00, 0x00, 0x00, 0x0C, // options_size = 12 + // --- IPv4Endpoint option (12 bytes) --- + 0x00, 0x09, // length field = 9 + 0x04, // option type: IpV4Endpoint + 0x00, // discard flag + 192, 168, 1, 10, // ip + 0x00, // reserved + 0x11, // protocol: UDP + 0x77, 0x2D, // port = 30509 + ]; + assert_eq!(buf, expected); +} + +#[test] +fn sd_header_stop_offer_service_with_load_balancing_golden_bytes() { + // StopOfferService entry (TTL forced to 0 by convention) referencing + // one LoadBalancing option. + let entry = Entry::StopOfferService(ServiceEntry { + index_first_options_run: 0, + index_second_options_run: 0, + options_count: OptionsCount::new(1, 0), + service_id: 0xABCD, + instance_id: 0x0002, + major_version: 2, + ttl: 0, + minor_version: 1, + }); + let option = Options::LoadBalancing { + priority: 100, + weight: 200, + }; + let entries = [entry]; + let options = [option]; + let sd_header = SdHeader::new(Flags::new_sd(RebootFlag::Continuous), &entries, &options); + + let mut buf = [0u8; 36]; + let n = sd_header.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 36); + + #[rustfmt::skip] + let expected: [u8; 36] = [ + 0x40, 0x00, 0x00, 0x00, // flags: unicast only (Continuous reboot) + 0x00, 0x00, 0x00, 0x10, // entries_size = 16 + // --- StopOfferService entry (16 bytes) --- + 0x02, // entry type: StopOfferService + 0x00, 0x00, // index_first/second_options_run + 0x10, // options_count: first=1, second=0 + 0xAB, 0xCD, // service_id + 0x00, 0x02, // instance_id + 0x02, // major_version + 0x00, 0x00, 0x00, // ttl = 0 (stop-offer) + 0x00, 0x00, 0x00, 0x01, // minor_version + 0x00, 0x00, 0x00, 0x08, // options_size = 8 + // --- LoadBalancing option (8 bytes) --- + 0x00, 0x05, // length field = 5 + 0x02, // option type: LoadBalancing + 0x00, // discard flag + 0x00, 0x64, // priority = 100 + 0x00, 0xC8, // weight = 200 + ]; + assert_eq!(buf, expected); +} + +#[test] +fn sd_header_subscribe_eventgroup_with_ipv6_endpoint_golden_bytes() { + // Subscribe (eventgroup) entry referencing one IPv6Endpoint option. + let entry = Entry::SubscribeEventGroup(EventGroupEntry { + index_first_options_run: 0, + index_second_options_run: 0, + options_count: OptionsCount::new(1, 0), + service_id: 0x0042, + instance_id: 0x0001, + major_version: 1, + ttl: 3, + counter: 0, + event_group_id: 1, + }); + let option = Options::IpV6Endpoint { + ip: core::net::Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0, 1), + protocol: TransportProtocol::Tcp, + port: 8080, + }; + let entries = [entry]; + let options = [option]; + let sd_header = SdHeader::new( + Flags::new_sd(RebootFlag::RecentlyRebooted), + &entries, + &options, + ); + + let mut buf = [0u8; 52]; + let n = sd_header.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 52); + + #[rustfmt::skip] + let expected: [u8; 52] = [ + 0xC0, 0x00, 0x00, 0x00, // flags + reserved + 0x00, 0x00, 0x00, 0x10, // entries_size = 16 + // --- Subscribe entry (16 bytes) --- + 0x06, // entry type: Subscribe + 0x00, 0x00, // index_first/second_options_run + 0x10, // options_count: first=1, second=0 + 0x00, 0x42, // service_id + 0x00, 0x01, // instance_id + 0x01, // major_version + 0x00, 0x00, 0x03, // ttl = 3 + 0x00, 0x00, // counter = 0 + 0x00, 0x01, // event_group_id = 1 + 0x00, 0x00, 0x00, 0x18, // options_size = 24 + // --- IPv6Endpoint option (24 bytes) --- + 0x00, 0x15, // length field = 21 + 0x06, // option type: IpV6Endpoint + 0x00, // discard flag + 0xFE, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, // ip: fe80::1 + 0x00, // reserved + 0x06, // protocol: TCP + 0x1F, 0x90, // port = 8080 + ]; + assert_eq!(buf, expected); +} + +#[test] +fn sd_header_subscribe_ack_eventgroup_with_configuration_golden_bytes() { + // SubscribeAckEventGroup entry referencing one Configuration option. + let entry = Entry::SubscribeAckEventGroup(EventGroupEntry { + index_first_options_run: 0, + index_second_options_run: 0, + options_count: OptionsCount::new(1, 0), + service_id: 0xAAAA, + instance_id: 0x0001, + major_version: 1, + ttl: 0x00FF_FFFF, + counter: 0, + event_group_id: 0x0010, + }); + let mut configuration_string = + heapless::Vec::::new( + ); + configuration_string.extend_from_slice(b"abc").unwrap(); + let option = Options::Configuration { + configuration_string, + }; + let entries = [entry]; + let options = [option]; + let sd_header = SdHeader::new(Flags::new_sd(RebootFlag::Continuous), &entries, &options); + + let mut buf = [0u8; 35]; + let n = sd_header.encode_to_slice(&mut buf).unwrap(); + assert_eq!(n, 35); + + #[rustfmt::skip] + let expected: [u8; 35] = [ + 0x40, 0x00, 0x00, 0x00, // flags: unicast only + 0x00, 0x00, 0x00, 0x10, // entries_size = 16 + // --- SubscribeAck entry (16 bytes) --- + 0x07, // entry type: SubscribeAck + 0x00, 0x00, // index_first/second_options_run + 0x10, // options_count: first=1, second=0 + 0xAA, 0xAA, // service_id + 0x00, 0x01, // instance_id + 0x01, // major_version + 0xFF, 0xFF, 0xFF, // ttl = 0x00FFFFFF + 0x00, 0x00, // counter = 0 + 0x00, 0x10, // event_group_id = 0x0010 + 0x00, 0x00, 0x00, 0x07, // options_size = 7 + // --- Configuration option (7 bytes) --- + 0x00, 0x04, // length field = 4 (1 + string_len 3) + 0x01, // option type: Configuration + 0x00, // discard flag + 0x61, 0x62, 0x63, // "abc" + ]; + assert_eq!(buf, expected); +} + +// --- One standalone test per `OptionType` (8 variants) --- + +fn encode_option(option: &Options) -> heapless::Vec { + let size = option.size(); + let mut buf = [0u8; 32]; + let n = option.encode(&mut &mut buf[..size]).unwrap(); + assert_eq!(n, size); + let mut out = heapless::Vec::new(); + out.extend_from_slice(&buf[..size]).unwrap(); + out +} + +#[test] +fn option_configuration_golden_bytes() { + let mut configuration_string = + heapless::Vec::::new( + ); + configuration_string.extend_from_slice(b"ab").unwrap(); + let option = Options::Configuration { + configuration_string, + }; + assert_eq!(u8::from(OptionType::Configuration), 0x01); + let bytes = encode_option(&option); + // length=3 (1 + string_len 2), type=0x01, discard=0, "ab" + assert_eq!(bytes.as_slice(), &[0x00, 0x03, 0x01, 0x00, 0x61, 0x62]); +} + +#[test] +fn option_load_balancing_golden_bytes() { + let option = Options::LoadBalancing { + priority: 0x1234, + weight: 0x5678, + }; + assert_eq!(u8::from(OptionType::LoadBalancing), 0x02); + let bytes = encode_option(&option); + // length=5, type=0x02, discard=0, priority=0x1234, weight=0x5678 + assert_eq!( + bytes.as_slice(), + &[0x00, 0x05, 0x02, 0x00, 0x12, 0x34, 0x56, 0x78] + ); +} + +#[test] +fn option_ipv4_endpoint_golden_bytes() { + let option = Options::IpV4Endpoint { + ip: core::net::Ipv4Addr::new(10, 0, 0, 1), + protocol: TransportProtocol::Udp, + port: 30490, + }; + assert_eq!(u8::from(OptionType::IpV4Endpoint), 0x04); + let bytes = encode_option(&option); + // length=9, type=0x04, discard=0, ip=10.0.0.1, reserved=0, proto=UDP(0x11), port=30490 + assert_eq!( + bytes.as_slice(), + &[0x00, 0x09, 0x04, 0x00, 10, 0, 0, 1, 0x00, 0x11, 0x77, 0x1A] + ); +} + +#[test] +fn option_ipv6_endpoint_golden_bytes() { + let option = Options::IpV6Endpoint { + ip: core::net::Ipv6Addr::new(0x2001, 0x0db8, 0, 0, 0, 0, 0, 1), + protocol: TransportProtocol::Tcp, + port: 443, + }; + assert_eq!(u8::from(OptionType::IpV6Endpoint), 0x06); + let bytes = encode_option(&option); + // length=21, type=0x06, discard=0, ip=2001:db8::1, reserved=0, proto=TCP(0x06), port=443 + #[rustfmt::skip] + let expected: [u8; 24] = [ + 0x00, 0x15, 0x06, 0x00, + 0x20, 0x01, 0x0D, 0xB8, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, + 0x00, 0x06, 0x01, 0xBB, + ]; + assert_eq!(bytes.as_slice(), &expected); +} + +#[test] +fn option_ipv4_multicast_golden_bytes() { + let option = Options::IpV4Multicast { + ip: core::net::Ipv4Addr::new(239, 1, 2, 3), + protocol: TransportProtocol::Udp, + port: 30491, + }; + assert_eq!(u8::from(OptionType::IpV4Multicast), 0x14); + let bytes = encode_option(&option); + // length=9, type=0x14, discard=0, ip=239.1.2.3, reserved=0, proto=UDP(0x11), port=30491 + assert_eq!( + bytes.as_slice(), + &[0x00, 0x09, 0x14, 0x00, 239, 1, 2, 3, 0x00, 0x11, 0x77, 0x1B] + ); +} + +#[test] +fn option_ipv6_multicast_golden_bytes() { + let option = Options::IpV6Multicast { + ip: core::net::Ipv6Addr::new(0xff02, 0, 0, 0, 0, 0, 0, 0xabcd), + protocol: TransportProtocol::Udp, + port: 5353, + }; + assert_eq!(u8::from(OptionType::IpV6Multicast), 0x16); + let bytes = encode_option(&option); + // length=21, type=0x16, discard=0, ip=ff02::abcd, reserved=0, proto=UDP(0x11), port=5353 + #[rustfmt::skip] + let expected: [u8; 24] = [ + 0x00, 0x15, 0x16, 0x00, + 0xFF, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xAB, 0xCD, + 0x00, 0x11, 0x14, 0xE9, + ]; + assert_eq!(bytes.as_slice(), &expected); +} + +#[test] +fn option_ipv4_sd_golden_bytes() { + let option = Options::IpV4SD { + ip: core::net::Ipv4Addr::new(192, 168, 0, 100), + protocol: TransportProtocol::Udp, + port: 30490, + }; + assert_eq!(u8::from(OptionType::IpV4SD), 0x24); + let bytes = encode_option(&option); + // length=9, type=0x24, discard=0, ip=192.168.0.100, reserved=0, proto=UDP(0x11), port=30490 + assert_eq!( + bytes.as_slice(), + &[ + 0x00, 0x09, 0x24, 0x00, 192, 168, 0, 100, 0x00, 0x11, 0x77, 0x1A + ] + ); +} + +#[test] +fn option_ipv6_sd_golden_bytes() { + let option = Options::IpV6SD { + ip: core::net::Ipv6Addr::new(0xfe80, 0, 0, 0, 0, 0, 0xabcd, 0xef01), + protocol: TransportProtocol::Tcp, + port: 30490, + }; + assert_eq!(u8::from(OptionType::IpV6SD), 0x26); + let bytes = encode_option(&option); + // length=21, type=0x26, discard=0, ip=fe80::abcd:ef01, reserved=0, proto=TCP(0x06), port=30490 + #[rustfmt::skip] + let expected: [u8; 24] = [ + 0x00, 0x15, 0x26, 0x00, + 0xFE, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x00, 0xAB, 0xCD, 0xEF, 0x01, + 0x00, 0x06, 0x77, 0x1A, + ]; + assert_eq!(bytes.as_slice(), &expected); +} + +// =========================================================================== +// 4. `sd_codec` datagram builders +// =========================================================================== + +#[cfg(any(feature = "bare_metal", feature = "server"))] +mod sd_codec_golden { + use simple_someip::protocol::sd::RebootFlag; + use simple_someip::sd_codec::{ + OfferServiceRequest, SubscribeAckRequest, SubscribeEventgroupRequest, + build_offer_service_datagram, build_subscribe_ack_datagram, + build_subscribe_eventgroup_datagram, + }; + + // NOTE: `encode_sd_datagram` (src/sd_codec.rs ~line 264) is a private + // helper, not reachable from an external integration test. Every + // public `build_*` builder funnels through it (SOME/IP wrapper via + // `Header::new_sd` + SD payload via `sd::Header::encode`), so pinning + // the three builders below exercises exactly the same encode path + // `encode_sd_datagram` would. + + #[test] + fn build_subscribe_eventgroup_datagram_golden_bytes() { + let request = SubscribeEventgroupRequest { + service_id: 0x1234, + instance_id: 0x0001, + major_version: 1, + event_group_id: 0x0001, + ttl: 5, + local_ip: core::net::Ipv4Addr::new(10, 0, 0, 5), + local_rx_port: 30509, + }; + let mut buf = [0u8; 64]; + let n = build_subscribe_eventgroup_datagram(&mut buf, &request, 7, RebootFlag::Continuous) + .unwrap(); + assert_eq!(n, 56); + + #[rustfmt::skip] + let expected: [u8; 56] = [ + // --- SOME/IP header (16 bytes) --- + 0xFF, 0xFF, 0x81, 0x00, // message_id: SD (service 0xFFFF, method 0x8100) + 0x00, 0x00, 0x00, 0x30, // length = 48 (SD payload 40 + 8) + 0x00, 0x00, 0x00, 0x07, // request_id = session 7 + 0x01, 0x01, // protocol/interface version + 0x02, 0x00, // message_type Notification, return_code Ok + // --- SD payload (40 bytes) --- + 0x40, 0x00, 0x00, 0x00, // flags: unicast only (Continuous reboot) + 0x00, 0x00, 0x00, 0x10, // entries_size = 16 + // Subscribe entry (16 bytes) + 0x06, 0x00, 0x00, 0x10, 0x12, 0x34, 0x00, 0x01, + 0x01, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00, 0x01, + 0x00, 0x00, 0x00, 0x0C, // options_size = 12 + // IPv4Endpoint option (12 bytes): 10.0.0.5:30509/UDP + 0x00, 0x09, 0x04, 0x00, 10, 0, 0, 5, 0x00, 0x11, 0x77, 0x2D, + ]; + assert_eq!(buf[..n], expected); + } + + #[test] + fn build_offer_service_datagram_golden_bytes() { + let request = OfferServiceRequest { + service_id: 0x0042, + instance_id: 0x0001, + major_version: 1, + minor_version: 0, + ttl: 3, + local_ip: core::net::Ipv4Addr::new(192, 0, 2, 1), + unicast_port: 30501, + }; + let mut buf = [0u8; 64]; + let n = build_offer_service_datagram(&mut buf, &request, 1).unwrap(); + assert_eq!(n, 56); + + #[rustfmt::skip] + let expected: [u8; 56] = [ + // --- SOME/IP header (16 bytes) --- + 0xFF, 0xFF, 0x81, 0x00, + 0x00, 0x00, 0x00, 0x30, // length = 48 + 0x00, 0x00, 0x00, 0x01, // request_id = session 1 + 0x01, 0x01, 0x02, 0x00, + // --- SD payload (40 bytes) --- + 0x40, 0x00, 0x00, 0x00, // flags: unicast only (builder forces Continuous) + 0x00, 0x00, 0x00, 0x10, + // OfferService entry (16 bytes) + 0x01, 0x00, 0x00, 0x10, 0x00, 0x42, 0x00, 0x01, + 0x01, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x0C, // options_size = 12 + // IPv4Endpoint option (12 bytes): 192.0.2.1:30501/UDP + 0x00, 0x09, 0x04, 0x00, 192, 0, 2, 1, 0x00, 0x11, 0x77, 0x25, + ]; + assert_eq!(buf[..n], expected); + } + + #[test] + fn build_subscribe_ack_datagram_golden_bytes() { + let request = SubscribeAckRequest { + service_id: 0x0099, + instance_id: 2, + event_group_id: 0x0005, + major_version: 1, + ttl: 10, + }; + let mut buf = [0u8; 64]; + let n = build_subscribe_ack_datagram(&mut buf, &request, 2).unwrap(); + assert_eq!(n, 44); + + #[rustfmt::skip] + let expected: [u8; 44] = [ + // --- SOME/IP header (16 bytes) --- + 0xFF, 0xFF, 0x81, 0x00, + 0x00, 0x00, 0x00, 0x24, // length = 36 (SD payload 28 + 8) + 0x00, 0x00, 0x00, 0x02, // request_id = session 2 + 0x01, 0x01, 0x02, 0x00, + // --- SD payload (28 bytes), no options --- + 0x40, 0x00, 0x00, 0x00, + 0x00, 0x00, 0x00, 0x10, + // SubscribeAckEventGroup entry (16 bytes), options_count = (0, 0) + 0x07, 0x00, 0x00, 0x00, 0x00, 0x99, 0x00, 0x02, + 0x01, 0x00, 0x00, 0x0A, 0x00, 0x00, 0x00, 0x05, + 0x00, 0x00, 0x00, 0x00, // options_size = 0 + ]; + assert_eq!(buf[..n], expected); + } +}