no_std / no_alloc support: zero-copy protocol core (0.2.0)#3
Open
JustinKovacich wants to merge 17 commits into
Open
no_std / no_alloc support: zero-copy protocol core (0.2.0)#3JustinKovacich wants to merge 17 commits into
JustinKovacich wants to merge 17 commits into
Conversation
Convert the DoIP protocol core to compile on bare-metal targets (no_std, no alloc) with tokio client/server behind opt-in features. - Cargo.toml: lean `default = []`; std/alloc/codec/client/server are opt-in. Drop byteorder; embedded-io replaces std::io on the core path. Version 0.1.0 -> 0.2.0 (breaking). - lib.rs: #![no_std], gate the tokio modules, core::time::Duration. - messages/: new Encode (into embedded_io::Write) and zero-copy Decode<'a> (borrows from the RX buffer) traits replace read/write. DiagnosticMessage/Ack and Message/Payload are now generic over the data container (MessageRef<'a> = borrowed, OwnedMessage = alloc). - framer.rs: sans-io try_frame() extracts one message from a buffer. Two intentional wire-behavior changes vs 0.1.0 (Gate A verified these are the ONLY divergences; 34/34 golden vectors otherwise byte-identical): - routing_activation_request now decodes the optional 4-byte manufacturer tail (previously hardcoded to None). - try_frame errors on a corrupt/short header instead of stalling (returning Ok(None) forever). Checkpoints pass: cargo check --no-default-features (+alloc, +thumbv7em target) and cargo test --no-default-features. Gate A adversarial review: PASS, no confirmed correctness findings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The work-package plan (WP0-WP6) with adversarial review gates that drove the 0.2.0 no_std conversion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Rewrite MessageCodec::decode to wrap the sans-io crate::try_frame() and convert the borrowed MessageRef into an OwnedMessage; rewrite MessageCodec::encode to buffer through a Vec<u8> using the new Encode trait. Behavior change: a corrupt/invalid header now surfaces as a MessageError from the codec instead of returning Ok(None) forever (the old code silently stalled the connection on bad framing). Mechanically update client.rs, client_inner.rs, connection.rs, server.rs, and socket_manager.rs to use OwnedMessage (the owned Message<Vec<u8>> alias) in place of the old non-generic Message type, and add the explicit alloc/std imports (Vec, Box, String, ToString, format) that the crate no longer pulls in implicitly now that it is #![no_std]. Fix socket_manager.rs's connection-reset check to compare against embedded_io::ErrorKind directly instead of calling a std::io::Error-shaped .kind() that no longer exists on the new MessageError::Io(embedded_io::ErrorKind) variant. Also apply the same generic-type fixes to the echo_server example (DiagnosticMessage<Vec<u8>>, OwnedMessage) and drop two pre-existing clippy warnings in simple_client so `cargo clippy --all-targets --features client,server -- -D warnings` is clean, since both examples are required to compile for the checkpoint. No behavioral changes intended beyond the documented codec error surfacing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Verifies no compile fallout remains in the integration test or examples after the WP4 no_std port, and adds a Feature flags section to the README covering the alloc/std/codec/client/server matrix, bare-metal usage with default-features = false, and cargo test --features client,server for development. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add examples/bare_metal_codec.rs: builds a RoutingActivationRequest and a DiagnosticMessage from stack data, encodes each into a fixed-size buffer via the core-only Encode trait, and exercises the sans-io try_frame() framer incrementally (partial slice -> Ok(None), then full slice -> decoded message). Compiles with `--no-default-features` and runs on host for reporting only. Add a `feature-matrix` CI job running the full verification matrix from the migration plan: no_std/alloc/std/client+server checks, the thumbv7em-none-eabihf cross-check, clippy -D warnings, both test configurations, and building/running the new example. Also apply `cargo fmt` to src/lib.rs (pre-existing formatting drift unrelated to this change) so `cargo fmt --check` passes as part of the matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds backticks to doc items referencing DoIP/DoIPInt/control_sender/ update_receiver/SendDiagnosticMessage; inlines format! args in Header's Debug impls; uses Duration::from_mins for the 5- and 2-minute timeouts instead of from_secs; allows needless_pass_by_value on MessageError::io (used as a bare fn pointer across ~30 map_err call sites, so taking a reference would force closures everywhere for no benefit); and allows too_many_lines on the match-heavy handle_control_message dispatcher rather than force an artificial split. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
framer::try_frame computed `Header::SIZE + payload_length as usize` directly, which overflows on 32-bit targets (e.g. thumbv7em) when a hostile header sets payload_length to u32::MAX, panicking in debug builds and wrapping in release. Instead compare available payload bytes (`buf.len() - Header::SIZE < payload_len`) before computing the total, which is now provably non-overflowing. Added a regression test with payload_length = 0xFFFFFFFF. `cargo check --no-default-features --features server` failed to build (E0432) because socket_manager unconditionally imports `client::ClientOptions` and `connection`, both gated on the `client` feature, while socket_manager itself was gated on `any(client, server)`. socket_manager is only ever used by client_inner.rs (there is no server usage), so the correct fix is to gate the `socket_manager` module on `client` alone rather than sprinkling cfg attributes through its internals. Also added the missing single-feature `server`/`client`/`codec` cargo check steps to the CI feature-matrix job so this class of bug is caught going forward. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Group 4 cleanup, no wire-format changes: - Add read_optional_array::<N> to decode_util and use it in both routing_activation_request and routing_activation_response, replacing the copy-pasted `if rest.len() >= 4` blocks. - Add encode_util with write_u8 / write_u16_be / write_u32_be and convert the scalar big-endian / single-byte write_all call sites across src/messages/ to use them. Raw slice writes (arrays, user data) stay as plain write_all. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Groups 2 and 4c. Group 2 (bug): routing_activation_response hardcoded the header payload length to 9, but RoutingActivationResponse::encode writes 13 bytes when oem_specific is Some. Framing such a message truncated it and broke round-trip. Build the payload struct first and use response.encoded_size() as the header length, matching routing_activation_request. Removes the stale "TODO: Check the payload length" comment. Wire-format change: only the previously-broken oem_specific=Some path (header length 9 -> 13). All other paths are byte-identical. Group 4c: diagnostic_message and diagnostic_message_ack constructors now build the payload struct first and use payload.encoded_size() instead of the hand-computed `len + 4` / `5 + len` (same bytes as before). Adds a regression test round-tripping a routing activation response with oem_specific set. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Group 1. A peer could crash the decoder or a server connection task with crafted input; these paths now return errors or handle a real case. - Payload::decode: replace the todo!() arms. DoIPEntityStatusRequest, DoIPEntityStatusResponse and DiagnosticPowerModeInfoResponse are now decoded properly (the first is an empty payload, the others delegate to their existing Decode impls). DiagnosticPowerModeInfoRequest (no payload variant) and the Reserved / ReservedVehicleManufacturer ranges return the new MessageError::UnsupportedPayloadType instead of panicking. - Payload::VehicleAnnouncement now wraps VehicleIdentificationResponse (its actual 0x0004 wire format) instead of being a unit variant, so encoded_size/encode delegate to it (a real implementation) rather than todo!(). This fixes the decode -> re-encode panic. Debug and to_owned_message updated accordingly. - server: a codec/decoding error on a client stream now logs and closes that connection gracefully instead of panic!(). Wire-format: no change to working paths. VehicleAnnouncement encode was previously unreachable (panicked); it now emits the standard 33-byte vehicle identification response. Adds tests: unsupported payload_type decodes to Err (no panic), entity status request decodes empty, and VehicleAnnouncement round-trips. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Group 3. Flattening socket errors to embedded_io::ErrorKind lost the OS error code/message in socket_manager logs. - Add a std-gated MessageError::Std(std::io::Error) variant that keeps the full error, and make From<std::io::Error> produce it. The no_std core is untouched and still uses Io(ErrorKind). - socket_manager: handle the new Std variant equivalently to Io, including the ConnectionReset check (matched via io_err.kind()), and log the full error so the OS detail is preserved. No wire-format change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
RoutingActivationResponse and VehicleIdentificationResponse are server->client messages; a peer sending either is a protocol-role violation. handle_client_message previously hit todo!() for both, panicking the connection task on network-reachable input (same class fixed for decoder/server paths in bdab87c). Now warn! and return Error::UnexpectedMessageType, matching the existing catch-all arm and the graceful-close handling already in place. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
EntityStatusRequest (0x4001) and VehicleIdentificationRequest (0x0001-0x0003) decode successfully since bdab87c, so a peer sending either reached handle_client_message's todo!() and panicked the connection task -- same remote-DoS class as the previous fixes. These are legitimate client->server requests the server just doesn't implement yet, so warn that they are not yet supported and ignore them (no response) instead of panicking. handle_client_message now returns Result<Option<OwnedMessage>, Error>; the connection loop only writes a response when one is produced and stays open otherwise. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
handle_client_connection incremented active_connections on entry but only decremented on two of its exit paths. Any `?` early return from handle_client_message (e.g. UnexpectedMessageType) or from write_sink.send skipped the decrement, permanently inflating the counter. A peer could repeatedly trigger these paths to leak all connection slots. Replace the manual fetch_add/fetch_sub pairing with an ActiveConnectionGuard RAII type whose Drop always decrements the counter, so every exit path (return, `?`, or panic) is covered. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Replace the empty integration_test.rs stub with real end-to-end tests that drive a Server and Client over an actual localhost TCP socket: routing activation, a diagnostic message round trip, and two hardening regressions (an unsupported payload type and a malformed header must close the offending connection without taking the server down). Servers bind to 127.0.0.1:0 and tests read back the OS-assigned port via Server::handle_client_connection (already public) instead of the fixed-port run_server(), and clients use a small test-only Connector (the documented extension point in connection.rs) since ConnectorSocket only accepts TCP_PORT. No src/ changes were needed. Also add net/io-util/time to the dev-dependency tokio features so the tests can drive raw TcpStream I/O; this is dev-only and does not affect the published feature set or no_std build. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The test harness's accept loop previously spawned a task per accepted connection, which would contain a panic escaping handle_client_connection and let the server-survival tests pass even if a future regression reintroduced a panic on malformed/unsupported input - the exact class of regression those tests exist to catch. Server::run_server awaits the handler inline in its accept loop, so a handler panic kills it in production. Restructure the harness to await handle_client_connection inline and sequentially, mirroring run_server's real control flow (the tests only ever need one connection at a time), and fix the comments that claimed spawn-based parity. Also replace the bare accept_loop.abort() at test end with a shutdown() helper that first asserts the accept loop is still alive - turning a dead accept loop into an explicit failure - and then aborts and awaits the task. Verified by temporarily reinserting a panic on the codec-error path in handle_client_connection: both unsupported_payload_type_does_not_kill_server and malformed_header_does_not_kill_server now fail under that simulated regression, and pass again once it is removed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Converts the DoIP protocol core to compile on bare-metal targets (
#![no_std], no alloc) while keeping the tokio client/server behind opt-in features. Version 0.1.0 → 0.2.0 (breaking).src/messages/,src/framer.rs): newEncode(intoembedded_io::Write) and zero-copyDecode<'a>traits replace thestd::io+byteorderread/write;Message<D>is generic over its data container (MessageRef<'a>borrowed /OwnedMessagealloc); sans-iotry_frame()extracts one message from an RX buffer.default = [];alloc/std/codec/client/serveropt-in (documented in README). Bare metal usesdefault-features = false.MessageCodecwrapstry_frame; client/server ported toOwnedMessagewith no behavioral changes beyond those listed below.thumbv7em-none-eabihfand per-feature checks; pedantic clippy now green.examples/bare_metal_codec.rsexercises encode + incremental framing with stack buffers only.Intentional behavior changes
routing_activation_requestnow decodes the optional 4-byte manufacturer tail (was hardcodedNone).todo!()panics reachable from remote input replaced with errors or real implementations (VehicleAnnouncementnow carries itsVehicleIdentificationResponsepayload);routing_activation_responseheader length derived from the payload (was hardcoded 9, wrong whenoem_specificisSome);try_framelength arithmetic is overflow-safe on 32-bit targets; server-only builds compile; socket error logs keep full OS error detail via a std-gatedMessageError::Stdvariant;active_connectionsuses an RAII guard so early returns can't leak slots.Verification
docs/planning/no_std_migration_plan.md): Gate A (wire-format equivalence, 34/34 golden vectors) and Gate B (std-layer behavior preservation) passed; final whole-branch review findings fixed as above.alloc,std,codec,client,server,client,server, thumbv7em target; clippy-D warnings+ pedantic; fmt.Known follow-ups (out of scope)
server.rsaccept-looppanic!on TCP accept failure (pre-existing TODO).tests/integration_test.rsis an empty stub (pre-existing).Vec(simplest-correct version per plan; aBufMutwriter adapter would remove a copy).🤖 Generated with Claude Code