From 6b7aa05aaa204cf9467f03db9f2ff423ef8597cb Mon Sep 17 00:00:00 2001 From: Nicholas Bishop Date: Thu, 23 Jan 2025 21:09:29 -0500 Subject: [PATCH] Fix memory leaks in DevicePathFromText The methods of both DevicePathToText and DevicePathFromText return memory that is allocated internally by UEFI, so it needs to be freed on drop. DevicePathToText already does that correctly by returning a `PoolString`, but this was missed in DevicePathFromText, which just returns a reference. Fix by adding PoolDevicePath and PoolDevicePathNode structs, and return them from the corresponding methods of DevicePathFromText. --- uefi-test-runner/src/proto/device_path.rs | 13 ++++++- uefi/CHANGELOG.md | 7 ++++ uefi/src/mem/mod.rs | 7 ++-- uefi/src/proto/device_path/text.rs | 44 ++++++++++++++++------- 4 files changed, 55 insertions(+), 16 deletions(-) diff --git a/uefi-test-runner/src/proto/device_path.rs b/uefi-test-runner/src/proto/device_path.rs index 3a08d482f..573db8d27 100644 --- a/uefi-test-runner/src/proto/device_path.rs +++ b/uefi-test-runner/src/proto/device_path.rs @@ -30,6 +30,17 @@ pub fn test() { ) .expect("Failed to open DevicePathFromText protocol"); + // Test round-trip conversion from path to text and back. + let device_path_string = device_path_to_text + .convert_device_path_to_text(&device_path, DisplayOnly(false), AllowShortcuts(false)) + .unwrap(); + assert_eq!( + *device_path_from_text + .convert_text_to_device_path(&device_path_string) + .unwrap(), + *device_path + ); + for path in device_path.node_iter() { info!( "path: type={:?}, subtype={:?}, length={}", @@ -47,7 +58,7 @@ pub fn test() { let convert = device_path_from_text .convert_text_to_device_node(text) .expect("Failed to convert text to device path"); - assert_eq!(path, convert); + assert_eq!(*path, *convert); } // Get the `LoadedImageDevicePath`. Verify it start with the same nodes as diff --git a/uefi/CHANGELOG.md b/uefi/CHANGELOG.md index 0a297080c..4416d0a56 100644 --- a/uefi/CHANGELOG.md +++ b/uefi/CHANGELOG.md @@ -1,11 +1,18 @@ # uefi - [Unreleased] +## Added +- Added `proto::device_path::PoolDevicePath` and + `proto::device_path::PoolDevicePathNode`. + ## Changed - MSRV increased to 1.81. - `core::error::Error` impls are no longer gated by the `unstable` feature. - Fixed missing checks in the `TryFrom` conversion from `&DevicePathNode` to specific node types. The node type and subtype are now checked, and `NodeConversionError::DifferentType` is returned if they do not match. +- **Breaking:** Fixed memory leaks in `DevicePathFromText` protocol. The methods + now return wrapper objects that free the device path / device path node on + drop. # uefi - 0.33.0 (2024-10-23) diff --git a/uefi/src/mem/mod.rs b/uefi/src/mem/mod.rs index 7992bebdc..220dc9b5b 100644 --- a/uefi/src/mem/mod.rs +++ b/uefi/src/mem/mod.rs @@ -12,16 +12,17 @@ pub(crate) mod util; #[cfg(feature = "alloc")] pub(crate) use util::*; -/// TODO +/// Wrapper for memory allocated with UEFI's pool allocator. The memory is freed +/// on drop. #[derive(Debug)] pub(crate) struct PoolAllocation(NonNull); impl PoolAllocation { - pub(crate) fn new(ptr: NonNull) -> Self { + pub(crate) const fn new(ptr: NonNull) -> Self { Self(ptr) } - pub(crate) fn as_ptr(&self) -> NonNull { + pub(crate) const fn as_ptr(&self) -> NonNull { self.0 } } diff --git a/uefi/src/proto/device_path/text.rs b/uefi/src/proto/device_path/text.rs index 79db28950..4e6bbad8a 100644 --- a/uefi/src/proto/device_path/text.rs +++ b/uefi/src/proto/device_path/text.rs @@ -61,6 +61,30 @@ impl Deref for PoolString { } } +/// Device path allocated from UEFI pool memory. +#[derive(Debug)] +pub struct PoolDevicePath(PoolAllocation); + +impl Deref for PoolDevicePath { + type Target = DevicePath; + + fn deref(&self) -> &Self::Target { + unsafe { DevicePath::from_ffi_ptr(self.0.as_ptr().as_ptr().cast()) } + } +} + +/// Device path node allocated from UEFI pool memory. +#[derive(Debug)] +pub struct PoolDevicePathNode(PoolAllocation); + +impl Deref for PoolDevicePathNode { + type Target = DevicePathNode; + + fn deref(&self) -> &Self::Target { + unsafe { DevicePathNode::from_ffi_ptr(self.0.as_ptr().as_ptr().cast()) } + } +} + /// Device Path to Text protocol. /// /// This protocol provides common utility functions for converting device @@ -139,14 +163,12 @@ impl DevicePathFromText { pub fn convert_text_to_device_node( &self, text_device_node: &CStr16, - ) -> Result<&DevicePathNode> { + ) -> Result { unsafe { let ptr = (self.0.convert_text_to_device_node)(text_device_node.as_ptr().cast()); - if ptr.is_null() { - Err(Status::OUT_OF_RESOURCES.into()) - } else { - Ok(DevicePathNode::from_ffi_ptr(ptr.cast())) - } + NonNull::new(ptr.cast_mut()) + .map(|p| PoolDevicePathNode(PoolAllocation::new(p.cast()))) + .ok_or(Status::OUT_OF_RESOURCES.into()) } } @@ -160,14 +182,12 @@ impl DevicePathFromText { /// memory for the conversion. /// /// [`OUT_OF_RESOURCES`]: Status::OUT_OF_RESOURCES - pub fn convert_text_to_device_path(&self, text_device_path: &CStr16) -> Result<&DevicePath> { + pub fn convert_text_to_device_path(&self, text_device_path: &CStr16) -> Result { unsafe { let ptr = (self.0.convert_text_to_device_path)(text_device_path.as_ptr().cast()); - if ptr.is_null() { - Err(Status::OUT_OF_RESOURCES.into()) - } else { - Ok(DevicePath::from_ffi_ptr(ptr.cast())) - } + NonNull::new(ptr.cast_mut()) + .map(|p| PoolDevicePath(PoolAllocation::new(p.cast()))) + .ok_or(Status::OUT_OF_RESOURCES.into()) } } }