Harden Win32 FFI boundary and fix safety/correctness issues (0.7.0) - #39
Merged
Conversation
Addresses a batch of safety, correctness, and robustness findings around the Win32/driver boundary, and bumps the crate to 0.7.0. Breaking changes: - Ndisapi no longer implements Clone. It owns the driver HANDLE and closes it on Drop, so cloning risked a double-close / use of a reused OS handle. Use Arc<Ndisapi> for shared ownership (as the async API and examples do). - ndis_get_request / ndis_set_request are now `unsafe fn`: the driver reads/writes the generic T as raw bytes, so T must be plain old data. The safe u32 wrappers (get/set_hw_packet_filter) are unchanged. Critical / High: - SockAddrStorage: add length-aware from_raw_sockaddr and use it during adapter enumeration so IPv6 addresses are no longer truncated to 16 bytes; zero-initialize storage in all constructors (no more reads of uninitialized memory). - win32_event_stream: register the waker before re-checking readiness (fixes a lost-wakeup race); UnregisterWaitEx now waits for in-flight callbacks (INVALID_HANDLE_VALUE) and, if that fails, leaks rather than freeing the callback / closing the event (avoids use-after-free and use-after-close). - AsyncNdisapiAdapter::new no longer leaks the event handle on partial construction. - static_api registry getters wrote through a *mut derived from a shared reference (UB) and leaked the HKEY; now use a real &mut, validate REG_DWORD/size, and close the key via an RAII guard. - set_friendly_name now writes a UTF-16 REG_SZ value (was UTF-8) and only updates the cached name after the registry write succeeds. Medium: - IntermediateBuffer::set_length clamps to MAX_ETHER_FRAME and the data accessors clamp defensively, so a bad length can no longer panic. - Centralize IPv4 -> IN_ADDR conversion to network byte order (the from_ip_string path stored host order, reversing addresses on little-endian Windows); fix add_ndp_entry_ipv4 to match; add round-trip tests. - delete_routes / reset_* / delete_unicast_address_* accumulate per-entry failures instead of masking them. - get_tcpip_bound_adapters_info clamps the driver-reported adapter count to ADAPTER_LIST_SIZE and uses lossy UTF-8 for adapter names. - Async batch send (send_packets_to_adapter/mstcp) returns the number of packets submitted instead of the always-zero packet_success counter. Also runs cargo fmt and fixes doc-comment lints so `cargo clippy -- -D warnings` is clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several critical safety, correctness, and resource management improvements across the ndisapi crate. Key changes include introducing RAII guards for registry keys to prevent leaks, fixing socket address parsing to prevent IPv6 truncation, ensuring consistent network byte order for IPv4 addresses, and replacing unsafe MaybeUninit patterns with zero-initialization to avoid undefined behavior. Additionally, API safety is improved by marking OID request methods as unsafe, removing the Clone implementation from Ndisapi to prevent double-closes, and clamping buffer lengths to prevent out-of-bounds panics. The review feedback correctly points out a potential issue in get_tcpip_bound_adapters_info where using String::from_utf8_lossy on the entire fixed-size adapter name array could retain garbage bytes after the first NUL terminator, suggesting converting only up to the first NUL byte instead.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR hardens the Win32/driver FFI boundary in the ndisapi crate and ships a 0.7.0 release with breaking API changes intended to eliminate UB and resource-lifetime hazards.
Changes:
- Fixes socket-address handling (IPv6 length-awareness, zero-initialized storage) and centralizes IPv4 byte-order conversion with added round-trip tests.
- Hardens async Win32 event-stream teardown and construction to prevent lost wakeups, leaks on partial initialization, and potential use-after-free/use-after-close.
- Tightens unsafe driver OID APIs (
ndis_get_request/ndis_set_requestnowunsafe), removesNdisapi: Clone, and improves robustness of several IP Helper / registry helpers.
Reviewed changes
Copilot reviewed 15 out of 20 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/netlib/ip_helper/sockaddr_storage.rs | Adds length-aware raw sockaddr conversion, zero-inits storage, centralizes IPv4 IN_ADDR encoding, and adds endianness/round-trip tests. |
| src/netlib/ip_helper/network_adapter_info/routing.rs | Accumulates per-route deletion failures instead of masking earlier failures. |
| src/netlib/ip_helper/network_adapter_info/ndp.rs | Fixes IPv4 address byte-order storage when creating NDP entries. |
| src/netlib/ip_helper/network_adapter_info/address.rs | Accumulates per-entry deletion failures for unicast address removal. |
| src/netlib/ip_helper/network_adapter_info.rs | Uses SOCKET_ADDRESS length-aware conversion for adapter address enumeration; fixes registry friendly-name write encoding semantics. |
| src/netlib/ip_helper/if_luid.rs | Doc comment formatting cleanup. |
| src/netlib.rs | Doc comment formatting cleanup. |
| src/ndisapi/static_api.rs | Adds HKEY RAII guard and safe REG_DWORD query/set helpers; removes prior UB/leak patterns. |
| src/ndisapi/base_api.rs | Marks generic OID APIs as unsafe, clamps driver-reported adapter counts, and avoids UTF-8 panics via lossy conversion. |
| src/ndisapi.rs | Removes Clone from Ndisapi and documents shared-ownership via Arc. |
| src/lib.rs | Re-formats re-exports for readability. |
| src/driver/filters.rs | Doc comment formatting cleanup. |
| src/driver/base.rs | Clamps IntermediateBuffer length and defensively clamps slice access to prevent panics. |
| src/driver.rs | Doc comment formatting cleanup. |
| src/async_api/win32_event_stream.rs | Fixes lost-wakeup race, and hardens unregister/teardown to avoid UAF/UAC by waiting for callbacks. |
| src/async_api.rs | Prevents Win32 event handle leaks on partial construction; adjusts batch-send return values to reflect submissions. |
| README.md | Bumps documented dependency version to 0.7.0. |
| examples/listadapters.rs | Updates call sites for newly-unsafe OID getter API. |
| examples/async-packthru.rs | Whitespace cleanup. |
| Cargo.toml | Bumps crate version to 0.7.0. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- get_tcpip_bound_adapters_info: convert the adapter name only up to the first NUL, so trailing garbage in the fixed-size buffer is ignored rather than embedded in the name (gemini-code-assist). - socket_address_to_ip: reject a non-positive iSockaddrLength via usize::try_from so a signed/negative length cannot wrap to a huge usize and over-read the source sockaddr (Copilot). - set_registry_dword: use to_le_bytes to document the little-endian REG_DWORD encoding instead of relying on host endianness (Copilot). - sockaddr_storage: update from_sockaddr_in/from_sockaddr_in6 docs to match the zero-initialized implementation (drop the stale MaybeUninit/assume_init "# Safety" text) and reword the ipv4_to_in_addr comment to focus on the to_be/to_le inconsistency (Copilot). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reword the doc comment to describe the in-memory octet layout ([a, b, c, d], first octet at the lowest address) explicitly instead of calling it "network byte order", which could be misread as a claim about the u32's numeric endianness. Addresses PR review feedback. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Addresses PR review feedback. Co-Authored-By: Claude Opus 4.8 <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
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
Addresses safety, correctness, and robustness findings from a review of the Win32/driver boundary, and bumps the crate to 0.7.0. Every finding was reviewed and validated before fixing; the change set builds clean and passes
fmt/clippy -D warnings/test.Ndisapino longer implementsClone. It owns the driverHANDLEand closes it onDrop, so cloning risked a double-close / use of a reused OS handle. UseArc<Ndisapi>for shared ownership (the async API and examples already do).ndis_get_request/ndis_set_requestare nowunsafe fn. The driver reads/writes the genericTas raw bytes, soTmust be plain-old-data. The safeu32wrappers (get_hw_packet_filter/set_hw_packet_filter) are unchanged.Fixes
Critical
sockaddr_storage.rs,network_adapter_info.rs): added length-awarefrom_raw_sockaddr(*const SOCKADDR, len)and switched adapter enumeration to it so IPv6 addresses are no longer truncated to a 16-byteSOCKADDR; allfrom_sockaddr*constructors now zero-initialize the storage.Ndisapi: Clonedouble-close: see Breaking changes.High
win32_event_stream.rs): the waker is registered before the readiness re-check (lost-wakeup fix);UnregisterWaitEx(.., INVALID_HANDLE_VALUE)waits for in-flight callbacks, and on failure deliberately leaks instead of freeing the callback / closing the event (avoids use-after-free / use-after-close).async_api.rs):AsyncNdisapiAdapter::newno longer leaks the event on partial construction.HKEYleak (static_api.rs): getters now write through a real&mut, validateREG_DWORD/size, and always close the key via an RAII guard.set_friendly_namewrote UTF-8 asREG_SZ(network_adapter_info.rs): now writes UTF-16 and updates the cached name only after the write succeeds.Medium
IntermediateBuffer::set_lengthclamps toMAX_ETHER_FRAME; data accessors clamp defensively (no panics).IN_ADDRconversion to network byte order (thefrom_ip_stringpath stored host order, reversing addresses on little-endian Windows); fixedadd_ndp_entry_ipv4; added round-trip tests.delete_routes/reset_*/delete_unicast_address_*accumulate per-entry failures instead of masking them.get_tcpip_bound_adapters_infoclamps the driver-reported count toADAPTER_LIST_SIZEand uses lossy UTF-8 for names.send_packets_to_adapter/mstcp) returns the number of packets submitted instead of the always-zeropacket_successcounter.Version
0.6.6->0.7.0(minor bump signals the breaking changes per 0.x semver) inCargo.tomlandREADME.md.Verification
cargo build --all-targets- passcargo test- pass (31 unit + 8 doctests; driver-dependent tests are#[ignore]d)cargo fmt --check- passcargo clippy -- -D warnings- pass (also fixed pre-existing doc-comment lints)