Harden Win32 FFI boundary and fix safety/correctness issues (0.7.0) (#39)

* feat!: harden Win32 FFI boundary for 0.7.0

Harden the Win32/driver boundary and bump the crate to 0.7.0.

Remove `Clone` from `Ndisapi` to prevent double-closing the owned driver handle, and mark generic `ndis_get_request` / `ndis_set_request` as `unsafe` because the driver reads and writes raw `T` bytes.

Fix IPv6 sockaddr truncation, uninitialized storage reads, async event lost-wakeup and teardown races, partial-construction handle leaks, registry UB/leaks, UTF-16 friendly-name writes, frame-length clamping, IPv4 byte-layout conversion, masked per-entry errors, adapter-count/name handling, and async batch-send return counts.

Also clean up docs, comments, formatting, and clippy warnings.

Author: wiresock

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "ndisapi"
-version = "0.6.6"
+version = "0.7.0"
 edition = "2021"
 authors = ["Vadim Smirnov <vadim@ntkernel.com>"]
 description = "Rust crate for interacting with the Windows Packet Filter driver (NDISAPI)"

README.md

@@ -28,7 +28,7 @@ Add the following to your `Cargo.toml` file:
 
 ```toml
 [dependencies]
-ndisapi = "0.6.6"
+ndisapi = "0.7.0"
 ```
 
 ## Usage

examples/async-packthru.rs

@@ -285,7 +285,7 @@ impl PacketInfo {
     /// All other fields are set to `None` because they are not applicable to ARP packets.
     fn handle_arp_packet(eth_hdr: &EthernetFrame<&[u8]>) -> PacketInfo {
         let arp_packet = ArpPacket::new_unchecked(eth_hdr.payload());
-        
+
         // Convert slices to fixed-size arrays
         let src_bytes: [u8; 4] = arp_packet
             .source_protocol_addr()
@@ -295,7 +295,7 @@ impl PacketInfo {
             .target_protocol_addr()
             .try_into()
             .unwrap_or([0u8; 4]);
-        
+
         PacketInfo {
             ethertype: EthernetProtocol::Arp,
             src_addr: Some(IpAddress::Ipv4(Ipv4Address::from_octets(src_bytes))),

examples/listadapters.rs

@@ -82,7 +82,9 @@ fn main() -> Result<()> {
             OID_802_3_CURRENT_ADDRESS,
             MacAddress::default(),
         );
-        if let Err(err) = driver.ndis_get_request::<_>(&mut current_address_request) {
+        // SAFETY: `MacAddress` wraps `[u8; 6]`, a plain-old-data type for which every byte
+        // pattern returned by the driver is a valid value.
+        if let Err(err) = unsafe { driver.ndis_get_request::<_>(&mut current_address_request) } {
             println!("Getting OID_802_3_CURRENT_ADDRESS Error: {}", err.message(),)
         } else {
             println!(

src/async_api.rs

@@ -20,7 +20,7 @@ use std::sync::Arc;
 use windows::{
     core::Result,
     Win32::{
-        Foundation::{HANDLE, WIN32_ERROR},
+        Foundation::{CloseHandle, HANDLE, WIN32_ERROR},
         System::Threading::CreateEventW,
     },
 };
@@ -78,13 +78,26 @@ impl AsyncNdisapiAdapter {
             CreateEventW(None, true, false, None)?
         };
 
-        // Setting the event for packet capture for the specified adapter.
+        // Hand the event to the `Win32EventStream` first. On success the stream owns the handle
+        // (and its wait registration) and closes it in its own `Drop`. If the stream cannot be
+        // created the handle is not yet owned by anything, so close it here to avoid leaking it.
+        let notif = match Win32EventStream::new(event_handle) {
+            Ok(notif) => notif,
+            Err(e) => {
+                let _ = unsafe { CloseHandle(event_handle) };
+                return Err(e);
+            }
+        };
+
+        // Register the event with the driver for packet-capture notifications. If this fails,
+        // `notif` is dropped, which unregisters the wait and closes the event handle — so the
+        // handle does not leak on this path either.
         driver.set_packet_event(adapter_handle, event_handle)?;
 
         Ok(Self {
             adapter_handle,
             driver,
-            notif: Win32EventStream::new(event_handle)?, // Creating a new Win32EventStream with the event handle.
+            notif,
         })
     }
 
@@ -227,7 +240,7 @@ impl AsyncNdisapiAdapter {
     /// # Arguments
     ///
     /// * `packet` - An `IntermediateBuffer` that will be encapsulated in an `EthPacket`
-    /// representing the Ethernet packet to be sent.
+    ///   representing the Ethernet packet to be sent.
     ///
     /// # Safety
     ///
@@ -277,7 +290,7 @@ impl AsyncNdisapiAdapter {
     ///
     /// # Returns
     ///
-    /// On successful operation, this function returns an `Ok(usize)` that represents the number of packets successfully sent to the network adapter. If the operation fails, an error is returned.
+    /// On successful operation, this function returns an `Ok(usize)` with the number of packets that were submitted to the driver for sending. If the operation fails, an error is returned.
     pub fn send_packets_to_adapter<'a, const N: usize>(
         &mut self,
         packets: impl IntoIterator<Item = &'a IntermediateBuffer>,
@@ -287,10 +300,14 @@ impl AsyncNdisapiAdapter {
             ndisapi::EthMRequest::<N>::from_iter(self.adapter_handle, packets.into_iter());
 
         // Try to send packets to the network adapter.
-        match self.driver.send_packets_to_adapter(&request) {
-            Ok(_) => Ok(request.get_packet_success() as usize),
-            Err(err) => Err(err),
-        }
+        //
+        // The send IOCTL takes no output buffer, so the driver never writes back the
+        // `packet_success` counter (unlike the read path); it would always read as 0 here.
+        // Report the number of packets submitted in the request instead, which is the
+        // meaningful value on the success path.
+        self.driver
+            .send_packets_to_adapter(&request)
+            .map(|_| request.get_packet_number() as usize)
     }
 
     /// Sends an Ethernet packet upwards through the network stack to the Microsoft TCP/IP protocol driver.
@@ -346,7 +363,7 @@ impl AsyncNdisapiAdapter {
     ///
     /// # Returns
     ///
-    /// On successful operation, this function returns `Ok(usize)`, where `usize` is the number of packets sent. If the operation fails, an error is returned.
+    /// On successful operation, this function returns `Ok(usize)`, where `usize` is the number of packets submitted to the driver for sending. If the operation fails, an error is returned.
     pub fn send_packets_to_mstcp<'a, const N: usize>(
         &mut self,
         packets: impl IntoIterator<Item = &'a IntermediateBuffer>,
@@ -356,10 +373,13 @@ impl AsyncNdisapiAdapter {
             ndisapi::EthMRequest::<N>::from_iter(self.adapter_handle, packets.into_iter());
 
         // Try to send packets upwards the network stack.
-        match self.driver.send_packets_to_mstcp(&request) {
-            Ok(_) => Ok(request.get_packet_success() as usize),
-            Err(err) => Err(err),
-        }
+        //
+        // As with `send_packets_to_adapter`, the send IOCTL has no output buffer, so
+        // `packet_success` is never populated by the driver. Return the number of packets
+        // submitted in the request rather than the always-zero success counter.
+        self.driver
+            .send_packets_to_mstcp(&request)
+            .map(|_| request.get_packet_number() as usize)
     }
 }
 
@@ -377,4 +397,4 @@ impl Drop for AsyncNdisapiAdapter {
             .driver
             .set_packet_event(self.adapter_handle, HANDLE(std::ptr::null_mut()));
     }
-}
+}

src/async_api/win32_event_stream.rs

@@ -36,7 +36,7 @@ use std::{
 use windows::{
     core::Result,
     Win32::{
-        Foundation::{CloseHandle, HANDLE},
+        Foundation::{CloseHandle, HANDLE, INVALID_HANDLE_VALUE},
         System::Threading::{
             RegisterWaitForSingleObject, ResetEvent, UnregisterWaitEx, INFINITE,
             WT_EXECUTEINWAITTHREAD,
@@ -89,14 +89,18 @@ impl Stream for Win32EventStream {
     fn poll_next(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Option<Self::Item>> {
         let this = Pin::into_inner(self);
 
-        if this.ready.swap(false, Ordering::Relaxed) {
-            // The Win32 event is ready, so we clear the ready flag and wake the waker, if present.
-            // Then we reset the event to non-signaled state.
-            // We signal readiness by returning `Poll::Ready`.
+        // Register the waker *before* checking readiness. The callback always stores `true` into
+        // `ready` and only then calls `waker.wake()`. By registering first and re-checking, a
+        // signal that fires between the check and the registration cannot be lost: either we
+        // observe `ready == true` here, or the callback's `wake()` targets the waker we just
+        // registered. (The previous order — check then register — could sleep forever if the
+        // event was signaled in the gap between the two.)
+        this.waker.register(cx.waker());
+
+        if this.ready.swap(false, Ordering::SeqCst) {
+            // The Win32 event was signaled; clear the flag and report readiness.
             Poll::Ready(Some(Ok(())))
         } else {
-            // The Win32 event is not ready, so we register the waker and return `Poll::Pending`.
-            this.waker.register(cx.waker());
             Poll::Pending
         }
     }
@@ -165,18 +169,33 @@ impl Win32EventNotification {
 impl Drop for Win32EventNotification {
     /// Implementing the Drop trait for the Win32EventNotification struct.
     fn drop(&mut self) {
-        unsafe {
-            // Deregistering the wait object.
-            if UnregisterWaitEx(self.wait_object, Some(self.win32_event)).is_err() {
-                //log::error!("error deregistering notification: {}", GetLastError);
+        // Deregister the wait and *block until any in-flight callback has finished*. Passing
+        // `INVALID_HANDLE_VALUE` as the completion event makes `UnregisterWaitEx` wait for
+        // outstanding callbacks to complete before returning. Only once that succeeds is it
+        // safe to free the callback box and close the event.
+        //
+        // Passing the event handle itself (as the original code did) merely asks the OS to
+        // signal that event on completion *without* waiting; tearing down a registered wait
+        // while it is still pending is explicitly undefined per the
+        // `RegisterWaitForSingleObject` documentation.
+        match unsafe { UnregisterWaitEx(self.wait_object, Some(INVALID_HANDLE_VALUE)) } {
+            Ok(()) => {
+                // The wait is gone and no callback can be running, so the boxed callback can be
+                // dropped and the event handle closed safely.
+                drop(unsafe { Box::from_raw(self.callback) });
+                let _ = unsafe { CloseHandle(self.win32_event) };
+            }
+            Err(_e) => {
+                // Deregistration failed, so we cannot prove that the wait is gone or that no
+                // callback is (or will later be) running. Freeing the callback box or closing
+                // the event here would risk a use-after-free of the callback and a
+                // use-after-close of the event handle (the callback also touches the event via
+                // `ResetEvent`). A leak is strictly preferable to undefined behavior, so we
+                // deliberately leak both the callback allocation and the event handle on this
+                // (expected-to-be-unreachable) path.
+                //log::error!("error deregistering notification: {_e}");
             }
-            drop(Box::from_raw(self.callback)); // Dropping the callback function.
         }
-
-        let _ = unsafe {
-            // Closing the handle to the event.
-            CloseHandle(self.win32_event)
-        };
     }
 }
 
@@ -190,4 +209,4 @@ impl Drop for Win32EventNotification {
 /// Windows API documentation. Our struct only contains raw pointers and handles
 /// that are essentially IDs which can be freely copied and are not tied to a
 /// specific thread. As such, it's safe to implement Send for this type.
-unsafe impl Send for Win32EventNotification {}
+unsafe impl Send for Win32EventNotification {}

src/driver.rs

@@ -10,25 +10,25 @@
 //! # Submodules
 //!
 //! * [`constants`] - Provides various constants and bitflag structures used to configure the
-//! packet filtering mechanism, specify filtering options for different protocols, and define
-//! the conditions for filtering at specific layers.
+//!   packet filtering mechanism, specify filtering options for different protocols, and define
+//!   the conditions for filtering at specific layers.
 //!
 //! * [`base`] - Provides Rust equivalents of several structures used in the NDISAPI Rust library
-//! for communicating with the Windows Packet Filter driver. he structures in this submodule are related
-//! to network adapters, Ethernet packets, adapter events, and Remote Access Service (RAS) links.
+//!   for communicating with the Windows Packet Filter driver. The structures in this submodule are related
+//!   to network adapters, Ethernet packets, adapter events, and Remote Access Service (RAS) links.
 //!
 //! * [`ioctl`] - Provides a collection of constants for IOCTL (Input/Output Control) codes and
-//! the `ctl_code` function used to generate these codes. IOCTL codes are used to communicate
-//! with the Windows Packet Filter driver to perform various operations.
+//!   the `ctl_code` function used to generate these codes. IOCTL codes are used to communicate
+//!   with the Windows Packet Filter driver to perform various operations.
 //!
 //! * [`filters`] - Provides structures for specifying filter conditions and actions for various protocols,
-//! including Ethernet 802.3, IPv4, IPv6, TCP, UDP, and ICMP. These structures allow users to define complex
-//! filtering rules based on multiple packet fields and layers.
+//!   including Ethernet 802.3, IPv4, IPv6, TCP, UDP, and ICMP. These structures allow users to define complex
+//!   filtering rules based on multiple packet fields and layers.
 //!
 //! * [`fastio`] - Provides Rust equivalents of several structures related to Fast I/O operations
-//! for the NDISAPI Rust library used in communicating with the Windows Packet Filter driver.
-//! The structures in this submodule are related to Fast I/O sections, which include headers and packet data,
-//! and are involved in read and write operations.
+//!   for the NDISAPI Rust library used in communicating with the Windows Packet Filter driver.
+//!   The structures in this submodule are related to Fast I/O sections, which include headers and packet data,
+//!   and are involved in read and write operations.
 //!
 
 // Submodules

src/driver/base.rs

@@ -161,26 +161,36 @@ impl IntermediateBuffer {
 
     /// Sets the length of the packet data stored in the `IntermediateBuffer`.
     ///
+    /// The value is clamped to `MAX_ETHER_FRAME` (the capacity of the underlying buffer) so that
+    /// a subsequent call to [`get_data`](Self::get_data) or [`get_data_mut`](Self::get_data_mut)
+    /// can never panic with an out-of-bounds slice.
+    ///
     /// # Arguments
     /// * `length`: A `u32` value representing the new length of the packet data.
     pub fn set_length(&mut self, length: u32) {
-        self.length = length
+        self.length = length.min(MAX_ETHER_FRAME as u32);
     }
 
     /// Returns a reference to the data stored in the buffer.
     ///
     /// This method returns a reference to the data stored in the buffer as a slice of bytes.
     /// The length of the slice is determined by the `length` field of the `buffer` struct.
+    ///
+    /// The length is clamped to the buffer capacity, so even a corrupt or version-mismatched
+    /// driver response that reports an oversized length cannot cause a panic here.
     pub fn get_data(&self) -> &[u8] {
-        &self.buffer.0[..self.length as usize]
+        &self.buffer.0[..(self.length as usize).min(MAX_ETHER_FRAME)]
     }
 
     /// Returns a mutable reference to the data stored in the buffer.
     ///
     /// This method returns a mutable reference to the data stored in the buffer as a slice of bytes.
     /// The length of the slice is determined by the `length` field of the `buffer` struct.
+    ///
+    /// The length is clamped to the buffer capacity, so even a corrupt or version-mismatched
+    /// driver response that reports an oversized length cannot cause a panic here.
     pub fn get_data_mut(&mut self) -> &mut [u8] {
-        &mut self.buffer.0[..self.length as usize]
+        &mut self.buffer.0[..(self.length as usize).min(MAX_ETHER_FRAME)]
     }
 }
 

src/driver/filters.rs

@@ -15,7 +15,7 @@
 //! * [`TcpUdpFilter`] - Represents a static filter for TCP and UDP packets.
 //! * [`IcmpFilter`] - Represents a static filter for ICMP packets.
 //! * [`StaticFilter`] - Represents a single static filter entry that combines filter conditions for various
-//! layers and the filter action to be taken.
+//!   layers and the filter action to be taken.
 //! * [`StaticFilterTable`] - Represents a table of static filters, used for managing multiple static filter entries.
 
 // Import required external crates and types

src/lib.rs

@@ -32,11 +32,12 @@ pub use crate::ndisapi::{
     IpAddressV4, IpAddressV4Union, IpAddressV6, IpAddressV6Union, IpRangeV4, IpRangeV6, IpSubnetV4,
     IpSubnetV6, IpV4Filter, IpV4FilterFlags, IpV6Filter, IpV6FilterFlags, Ndisapi,
     NetworkAdapterInfo, NetworkLayerFilter, NetworkLayerFilterUnion, PacketOidData, PortRange,
-    RasLinks, StaticFilter, StaticFilterTable, StaticFilterWithPosition, TcpUdpFilter, TcpUdpFilterFlags,
-    TransportLayerFilter, TransportLayerFilterUnion, UnsortedReadRequest, UnsortedSendRequest,
-    Version, ETHER_ADDR_LENGTH, ETH_802_3, FILTER_PACKET_DROP, FILTER_PACKET_DROP_RDR,
-    FILTER_PACKET_PASS, FILTER_PACKET_PASS_RDR, FILTER_PACKET_REDIRECT, ICMP, IPV4, IPV6,
-    IP_RANGE_V4_TYPE, IP_RANGE_V6_TYPE, IP_SUBNET_V4_TYPE, IP_SUBNET_V6_TYPE, TCPUDP,
+    RasLinks, StaticFilter, StaticFilterTable, StaticFilterWithPosition, TcpUdpFilter,
+    TcpUdpFilterFlags, TransportLayerFilter, TransportLayerFilterUnion, UnsortedReadRequest,
+    UnsortedSendRequest, Version, ETHER_ADDR_LENGTH, ETH_802_3, FILTER_PACKET_DROP,
+    FILTER_PACKET_DROP_RDR, FILTER_PACKET_PASS, FILTER_PACKET_PASS_RDR, FILTER_PACKET_REDIRECT,
+    ICMP, IPV4, IPV6, IP_RANGE_V4_TYPE, IP_RANGE_V6_TYPE, IP_SUBNET_V4_TYPE, IP_SUBNET_V6_TYPE,
+    TCPUDP,
 };
 
 pub use crate::async_api::AsyncNdisapiAdapter;