* [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling
2023-10-03 9:39 [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Erik Schilling
@ 2023-10-03 9:39 ` Erik Schilling
2023-10-03 10:07 ` Viresh Kumar
2023-10-04 11:22 ` Bartosz Golaszewski
2023-10-03 9:39 ` [libgpiod][PATCH v3 2/3] bindings: rust: rename {event,settings}_clone to try_clone Erik Schilling
` (2 subsequent siblings)
3 siblings, 2 replies; 11+ messages in thread
From: Erik Schilling @ 2023-10-03 9:39 UTC (permalink / raw)
To: Linux-GPIO; +Cc: Viresh Kumar, Manos Pitsidianakis, Kent Gibson, Erik Schilling
While attention was provided to prevent freeing in non-owned use-cases,
the lifetime of these object was not properly modeled.
The line_info from an event may only be used for as long as the event
exists.
This allowed us to write unsafe-free Rust code that causes a
use-after-free:
let event = chip.read_info_event().unwrap();
let line_info = event.line_info().unwrap();
drop(event);
dbg!(line_info.name().unwrap());
Which makes the AddressSanitizer scream:
==90154==ERROR: AddressSanitizer: heap-use-after-free on address 0x50b000005dc4 at pc 0x55a4f883a009 bp 0x7f60ac8fbbc0 sp 0x7f60ac8fb388
READ of size 2 at 0x50b000005dc4 thread T2
[...]
#3 0x55a4f8c3d5f3 in libgpiod::line_info::Info::name::h5ba0bfd360ecb405 libgpiod/bindings/rust/libgpiod/src/line_info.rs:70:18
[...]
0x50b000005dc4 is located 4 bytes inside of 112-byte region [0x50b000005dc0,0x50b000005e30)
freed by thread T2 here:
[...]
#1 0x7f60b07f7e31 in gpiod_info_event_free libgpiod/lib/info-event.c:61:2
[...]
previously allocated by thread T2 here:
#0 0x55a4f88b04be in malloc /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
#1 0x7f60b07f8ff0 in gpiod_line_info_from_uapi libgpiod/lib/line-info.c:144:9
The fix is to distinguish between the owned and non-owned variants and
assigning lifetimes to non-owned variants.
For modeling the non-owned type there are a couple of options. The ideal
solution would be using extern_types [1]. But that is still unstable.
Instead, we are defining a #[repr(transparent)] wrapper around the opaque
gpiod_line_info struct and cast the pointer to a reference.
This was recommended on the Rust Discord server as good practise.
(Thanks to Kyuuhachi, shepmaster, pie_flavor and ilyvion! Also thanks to
@epilys for a brainstorming on this on #linaro-virtualization IRC).
Of course, determining the lifetimes and casting across the types
requires some care. So this adds a couple of SAFETY comments that would
probably also have helped the existing code.
[1] https://github.com/rust-lang/rfcs/blob/master/text/1861-extern-types.md
Fixes: 91f9373 ("bindings: rust: Add libgpiod crate")
Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
bindings/rust/libgpiod/src/chip.rs | 8 +-
bindings/rust/libgpiod/src/info_event.rs | 8 +-
bindings/rust/libgpiod/src/line_info.rs | 130 +++++++++++++++++++++----------
3 files changed, 100 insertions(+), 46 deletions(-)
diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
index ebc15dc..bbb962f 100644
--- a/bindings/rust/libgpiod/src/chip.rs
+++ b/bindings/rust/libgpiod/src/chip.rs
@@ -85,7 +85,9 @@ impl Chip {
));
}
- line::Info::new(info)
+ // SAFETY: We verified that the pointer is valid. We own the pointer and
+ // no longer use it after converting it into a Info instance.
+ Ok(unsafe { line::Info::from_raw(info) })
}
/// Get the current snapshot of information about the line at given offset and start watching
@@ -101,7 +103,9 @@ impl Chip {
));
}
- line::Info::new_watch(info)
+ // SAFETY: We verified that the pointer is valid. We own the instance and
+ // no longer use it after converting it into a Info instance.
+ Ok(unsafe { line::Info::from_raw(info) })
}
/// Stop watching a line
diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs
index db60600..6eec0db 100644
--- a/bindings/rust/libgpiod/src/info_event.rs
+++ b/bindings/rust/libgpiod/src/info_event.rs
@@ -44,7 +44,7 @@ impl Event {
}
/// Get the line-info object associated with the event.
- pub fn line_info(&self) -> Result<line::Info> {
+ pub fn line_info(&self) -> Result<&line::InfoRef> {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
let info = unsafe { gpiod::gpiod_info_event_get_line_info(self.event) };
@@ -55,7 +55,11 @@ impl Event {
));
}
- line::Info::new_from_event(info)
+ // SAFETY: The pointer is valid. The returned reference receives the
+ // lifetime '0 - the same as &self. &self also controls lifetime and
+ // ownership of the owning object. Therefore, the borrow prevents moving
+ // of the owning object to another thread.
+ Ok(unsafe { line::InfoRef::from_raw(info) })
}
}
diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
index c4f488c..2148789 100644
--- a/bindings/rust/libgpiod/src/line_info.rs
+++ b/bindings/rust/libgpiod/src/line_info.rs
@@ -2,9 +2,10 @@
// SPDX-FileCopyrightText: 2022 Linaro Ltd.
// SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
-use std::ffi::CStr;
+use std::ops::Deref;
use std::str;
use std::time::Duration;
+use std::{ffi::CStr, marker::PhantomData};
use super::{
gpiod,
@@ -12,7 +13,7 @@ use super::{
Error, Result,
};
-/// Line info
+/// Line info reference
///
/// Exposes functions for retrieving kernel information about both requested and
/// free lines. Line info object contains an immutable snapshot of a line's status.
@@ -20,48 +21,57 @@ use super::{
/// The line info contains all the publicly available information about a
/// line, which does not include the line value. The line must be requested
/// to access the line value.
-
-#[derive(Debug, Eq, PartialEq)]
-pub struct Info {
- info: *mut gpiod::gpiod_line_info,
- contained: bool,
+///
+/// [InfoRef] only abstracts a reference to a [gpiod::gpiod_line_info] instance whose lifetime is managed
+/// by a different object instance. The owned counter-part of this type is [Info].
+#[derive(Debug)]
+#[repr(transparent)]
+pub struct InfoRef {
+ _info: gpiod::gpiod_line_info,
+ // Avoid the automatic `Sync` implementation.
+ //
+ // The C lib does not allow parallel invocations of the API. We could model
+ // that by restricting all wrapper functions to `&mut Info` - which would
+ // ensure exclusive access. But that would make the API a bit weird...
+ // So instead we just suppress the `Sync` implementation, which suppresses
+ // the `Send` implementation on `&Info` - disallowing to send it to other
+ // threads, making concurrent use impossible.
+ _not_sync: PhantomData<*mut gpiod::gpiod_line_info>,
}
-impl Info {
- fn new_internal(info: *mut gpiod::gpiod_line_info, contained: bool) -> Result<Self> {
- Ok(Self { info, contained })
- }
-
- /// Get a snapshot of information about the line.
- pub(crate) fn new(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
- Info::new_internal(info, false)
- }
-
- /// Get a snapshot of information about the line and start watching it for changes.
- pub(crate) fn new_watch(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
- Info::new_internal(info, false)
+impl InfoRef {
+ /// Converts a non-owning pointer to a wrapper reference of a specific
+ /// lifetime
+ ///
+ /// No ownership will be assumed, the pointer must be free'd by the original
+ /// owner.
+ ///
+ /// SAFETY: The pointer must point to an instance that is valid for the
+ /// entire lifetime 'a. The instance must be owned by an object that is
+ /// owned by the thread invoking this method. The owning object may not be
+ /// moved to another thread for the entire lifetime 'a.
+ pub(crate) unsafe fn from_raw<'a>(info: *mut gpiod::gpiod_line_info) -> &'a InfoRef {
+ &*(info as *mut _)
}
- /// Get the Line info object associated with an event.
- pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
- Info::new_internal(info, true)
+ fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
+ self as *const _ as *mut _
}
/// Get the offset of the line within the GPIO chip.
///
/// The offset uniquely identifies the line on the chip. The combination of the chip and offset
/// uniquely identifies the line within the system.
-
pub fn offset(&self) -> Offset {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- unsafe { gpiod::gpiod_line_info_get_offset(self.info) }
+ unsafe { gpiod::gpiod_line_info_get_offset(self.as_raw_ptr()) }
}
/// Get GPIO line's name.
pub fn name(&self) -> Result<&str> {
// SAFETY: The string returned by libgpiod is guaranteed to live as long
// as the `struct Info`.
- let name = unsafe { gpiod::gpiod_line_info_get_name(self.info) };
+ let name = unsafe { gpiod::gpiod_line_info_get_name(self.as_raw_ptr()) };
if name.is_null() {
return Err(Error::NullString("GPIO line's name"));
}
@@ -79,14 +89,14 @@ impl Info {
/// the line is used and we can't request it.
pub fn is_used(&self) -> bool {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- unsafe { gpiod::gpiod_line_info_is_used(self.info) }
+ unsafe { gpiod::gpiod_line_info_is_used(self.as_raw_ptr()) }
}
/// Get the GPIO line's consumer name.
pub fn consumer(&self) -> Result<&str> {
// SAFETY: The string returned by libgpiod is guaranteed to live as long
// as the `struct Info`.
- let name = unsafe { gpiod::gpiod_line_info_get_consumer(self.info) };
+ let name = unsafe { gpiod::gpiod_line_info_get_consumer(self.as_raw_ptr()) };
if name.is_null() {
return Err(Error::NullString("GPIO line's consumer name"));
}
@@ -100,44 +110,44 @@ impl Info {
/// Get the GPIO line's direction.
pub fn direction(&self) -> Result<Direction> {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.info) })
+ Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.as_raw_ptr()) })
}
/// Returns true if the line is "active-low", false otherwise.
pub fn is_active_low(&self) -> bool {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- unsafe { gpiod::gpiod_line_info_is_active_low(self.info) }
+ unsafe { gpiod::gpiod_line_info_is_active_low(self.as_raw_ptr()) }
}
/// Get the GPIO line's bias setting.
pub fn bias(&self) -> Result<Option<Bias>> {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.info) })
+ Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.as_raw_ptr()) })
}
/// Get the GPIO line's drive setting.
pub fn drive(&self) -> Result<Drive> {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.info) })
+ Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.as_raw_ptr()) })
}
/// Get the current edge detection setting of the line.
pub fn edge_detection(&self) -> Result<Option<Edge>> {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.info) })
+ Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.as_raw_ptr()) })
}
/// Get the current event clock setting used for edge event timestamps.
pub fn event_clock(&self) -> Result<EventClock> {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.info) })
+ EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.as_raw_ptr()) })
}
/// Returns true if the line is debounced (either by hardware or by the
/// kernel software debouncer), false otherwise.
pub fn is_debounced(&self) -> bool {
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- unsafe { gpiod::gpiod_line_info_is_debounced(self.info) }
+ unsafe { gpiod::gpiod_line_info_is_debounced(self.as_raw_ptr()) }
}
/// Get the debounce period of the line.
@@ -147,18 +157,54 @@ impl Info {
#[allow(clippy::unnecessary_cast)]
// SAFETY: `gpiod_line_info` is guaranteed to be valid here.
Duration::from_micros(unsafe {
- gpiod::gpiod_line_info_get_debounce_period_us(self.info) as u64
+ gpiod::gpiod_line_info_get_debounce_period_us(self.as_raw_ptr()) as u64
})
}
}
+/// Line info
+///
+/// This is the owned counterpart to [InfoRef]. Due to a [Deref] implementation,
+/// all functions of [InfoRef] can also be called on this type.
+#[derive(Debug)]
+pub struct Info {
+ info: *mut gpiod::gpiod_line_info,
+}
+
+// SAFETY: Info models a owned instance whose ownership may be safely
+// transferred to other threads.
+unsafe impl Send for Info {}
+
+impl Info {
+ /// Converts a owned pointer into an owned instance
+ ///
+ /// Assumes sole ownership over a [gpiod::gpiod_line_info] instance.
+ ///
+ /// SAFETY: The pointer must point to an instance that is valid. After
+ /// constructing an [Info] the pointer MUST NOT be used for any other
+ /// purpose anymore. All interactions with the libgpiod API have to happen
+ /// through this object.
+ pub(crate) unsafe fn from_raw(info: *mut gpiod::gpiod_line_info) -> Info {
+ Info { info }
+ }
+}
+
+impl Deref for Info {
+ type Target = InfoRef;
+
+ fn deref(&self) -> &Self::Target {
+ // SAFETY: The pointer is valid for the entire lifetime '0. Info is not
+ // Sync. Therefore, no &Info may be held by a different thread. Hence,
+ // the current thread owns it. Since we borrow with the lifetime of '0,
+ // no move to a different thread can occur while a reference remains
+ // being hold.
+ unsafe { InfoRef::from_raw(self.info) }
+ }
+}
+
impl Drop for Info {
fn drop(&mut self) {
- // We must not free the Line info object created from `struct chip::Event` by calling
- // libgpiod API.
- if !self.contained {
- // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
- unsafe { gpiod::gpiod_line_info_free(self.info) }
- }
+ // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
+ unsafe { gpiod::gpiod_line_info_free(self.info) }
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling
2023-10-03 9:39 ` [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling Erik Schilling
@ 2023-10-03 10:07 ` Viresh Kumar
2023-10-04 11:22 ` Bartosz Golaszewski
1 sibling, 0 replies; 11+ messages in thread
From: Viresh Kumar @ 2023-10-03 10:07 UTC (permalink / raw)
To: Erik Schilling; +Cc: Linux-GPIO, Manos Pitsidianakis, Kent Gibson
On 03-10-23, 11:39, Erik Schilling wrote:
> While attention was provided to prevent freeing in non-owned use-cases,
> the lifetime of these object was not properly modeled.
>
> The line_info from an event may only be used for as long as the event
> exists.
>
> This allowed us to write unsafe-free Rust code that causes a
> use-after-free:
>
> let event = chip.read_info_event().unwrap();
> let line_info = event.line_info().unwrap();
> drop(event);
> dbg!(line_info.name().unwrap());
>
> Which makes the AddressSanitizer scream:
>
> ==90154==ERROR: AddressSanitizer: heap-use-after-free on address 0x50b000005dc4 at pc 0x55a4f883a009 bp 0x7f60ac8fbbc0 sp 0x7f60ac8fb388
> READ of size 2 at 0x50b000005dc4 thread T2
> [...]
> #3 0x55a4f8c3d5f3 in libgpiod::line_info::Info::name::h5ba0bfd360ecb405 libgpiod/bindings/rust/libgpiod/src/line_info.rs:70:18
> [...]
>
> 0x50b000005dc4 is located 4 bytes inside of 112-byte region [0x50b000005dc0,0x50b000005e30)
> freed by thread T2 here:
> [...]
> #1 0x7f60b07f7e31 in gpiod_info_event_free libgpiod/lib/info-event.c:61:2
> [...]
>
> previously allocated by thread T2 here:
> #0 0x55a4f88b04be in malloc /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
> #1 0x7f60b07f8ff0 in gpiod_line_info_from_uapi libgpiod/lib/line-info.c:144:9
>
> The fix is to distinguish between the owned and non-owned variants and
> assigning lifetimes to non-owned variants.
>
> For modeling the non-owned type there are a couple of options. The ideal
> solution would be using extern_types [1]. But that is still unstable.
> Instead, we are defining a #[repr(transparent)] wrapper around the opaque
> gpiod_line_info struct and cast the pointer to a reference.
>
> This was recommended on the Rust Discord server as good practise.
> (Thanks to Kyuuhachi, shepmaster, pie_flavor and ilyvion! Also thanks to
> @epilys for a brainstorming on this on #linaro-virtualization IRC).
>
> Of course, determining the lifetimes and casting across the types
> requires some care. So this adds a couple of SAFETY comments that would
> probably also have helped the existing code.
>
> [1] https://github.com/rust-lang/rfcs/blob/master/text/1861-extern-types.md
>
> Fixes: 91f9373 ("bindings: rust: Add libgpiod crate")
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
> bindings/rust/libgpiod/src/chip.rs | 8 +-
> bindings/rust/libgpiod/src/info_event.rs | 8 +-
> bindings/rust/libgpiod/src/line_info.rs | 130 +++++++++++++++++++++----------
> 3 files changed, 100 insertions(+), 46 deletions(-)
>
> diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
> index ebc15dc..bbb962f 100644
> --- a/bindings/rust/libgpiod/src/chip.rs
> +++ b/bindings/rust/libgpiod/src/chip.rs
> @@ -85,7 +85,9 @@ impl Chip {
> ));
> }
>
> - line::Info::new(info)
> + // SAFETY: We verified that the pointer is valid. We own the pointer and
> + // no longer use it after converting it into a Info instance.
> + Ok(unsafe { line::Info::from_raw(info) })
> }
>
> /// Get the current snapshot of information about the line at given offset and start watching
> @@ -101,7 +103,9 @@ impl Chip {
> ));
> }
>
> - line::Info::new_watch(info)
> + // SAFETY: We verified that the pointer is valid. We own the instance and
> + // no longer use it after converting it into a Info instance.
> + Ok(unsafe { line::Info::from_raw(info) })
> }
>
> /// Stop watching a line
> diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs
> index db60600..6eec0db 100644
> --- a/bindings/rust/libgpiod/src/info_event.rs
> +++ b/bindings/rust/libgpiod/src/info_event.rs
> @@ -44,7 +44,7 @@ impl Event {
> }
>
> /// Get the line-info object associated with the event.
> - pub fn line_info(&self) -> Result<line::Info> {
> + pub fn line_info(&self) -> Result<&line::InfoRef> {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> let info = unsafe { gpiod::gpiod_info_event_get_line_info(self.event) };
>
> @@ -55,7 +55,11 @@ impl Event {
> ));
> }
>
> - line::Info::new_from_event(info)
> + // SAFETY: The pointer is valid. The returned reference receives the
> + // lifetime '0 - the same as &self. &self also controls lifetime and
> + // ownership of the owning object. Therefore, the borrow prevents moving
> + // of the owning object to another thread.
> + Ok(unsafe { line::InfoRef::from_raw(info) })
> }
> }
>
> diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
> index c4f488c..2148789 100644
> --- a/bindings/rust/libgpiod/src/line_info.rs
> +++ b/bindings/rust/libgpiod/src/line_info.rs
> @@ -2,9 +2,10 @@
> // SPDX-FileCopyrightText: 2022 Linaro Ltd.
> // SPDX-FileCopyrightText: 2022 Viresh Kumar <viresh.kumar@linaro.org>
>
> -use std::ffi::CStr;
> +use std::ops::Deref;
> use std::str;
> use std::time::Duration;
> +use std::{ffi::CStr, marker::PhantomData};
>
> use super::{
> gpiod,
> @@ -12,7 +13,7 @@ use super::{
> Error, Result,
> };
>
> -/// Line info
> +/// Line info reference
> ///
> /// Exposes functions for retrieving kernel information about both requested and
> /// free lines. Line info object contains an immutable snapshot of a line's status.
> @@ -20,48 +21,57 @@ use super::{
> /// The line info contains all the publicly available information about a
> /// line, which does not include the line value. The line must be requested
> /// to access the line value.
> -
> -#[derive(Debug, Eq, PartialEq)]
> -pub struct Info {
> - info: *mut gpiod::gpiod_line_info,
> - contained: bool,
> +///
> +/// [InfoRef] only abstracts a reference to a [gpiod::gpiod_line_info] instance whose lifetime is managed
> +/// by a different object instance. The owned counter-part of this type is [Info].
> +#[derive(Debug)]
> +#[repr(transparent)]
> +pub struct InfoRef {
> + _info: gpiod::gpiod_line_info,
> + // Avoid the automatic `Sync` implementation.
> + //
> + // The C lib does not allow parallel invocations of the API. We could model
> + // that by restricting all wrapper functions to `&mut Info` - which would
> + // ensure exclusive access. But that would make the API a bit weird...
> + // So instead we just suppress the `Sync` implementation, which suppresses
> + // the `Send` implementation on `&Info` - disallowing to send it to other
> + // threads, making concurrent use impossible.
> + _not_sync: PhantomData<*mut gpiod::gpiod_line_info>,
> }
>
> -impl Info {
> - fn new_internal(info: *mut gpiod::gpiod_line_info, contained: bool) -> Result<Self> {
> - Ok(Self { info, contained })
> - }
> -
> - /// Get a snapshot of information about the line.
> - pub(crate) fn new(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> - Info::new_internal(info, false)
> - }
> -
> - /// Get a snapshot of information about the line and start watching it for changes.
> - pub(crate) fn new_watch(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> - Info::new_internal(info, false)
> +impl InfoRef {
> + /// Converts a non-owning pointer to a wrapper reference of a specific
> + /// lifetime
> + ///
> + /// No ownership will be assumed, the pointer must be free'd by the original
> + /// owner.
> + ///
> + /// SAFETY: The pointer must point to an instance that is valid for the
> + /// entire lifetime 'a. The instance must be owned by an object that is
> + /// owned by the thread invoking this method. The owning object may not be
> + /// moved to another thread for the entire lifetime 'a.
> + pub(crate) unsafe fn from_raw<'a>(info: *mut gpiod::gpiod_line_info) -> &'a InfoRef {
> + &*(info as *mut _)
> }
>
> - /// Get the Line info object associated with an event.
> - pub(crate) fn new_from_event(info: *mut gpiod::gpiod_line_info) -> Result<Self> {
> - Info::new_internal(info, true)
> + fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
> + self as *const _ as *mut _
> }
>
> /// Get the offset of the line within the GPIO chip.
> ///
> /// The offset uniquely identifies the line on the chip. The combination of the chip and offset
> /// uniquely identifies the line within the system.
> -
> pub fn offset(&self) -> Offset {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - unsafe { gpiod::gpiod_line_info_get_offset(self.info) }
> + unsafe { gpiod::gpiod_line_info_get_offset(self.as_raw_ptr()) }
> }
>
> /// Get GPIO line's name.
> pub fn name(&self) -> Result<&str> {
> // SAFETY: The string returned by libgpiod is guaranteed to live as long
> // as the `struct Info`.
> - let name = unsafe { gpiod::gpiod_line_info_get_name(self.info) };
> + let name = unsafe { gpiod::gpiod_line_info_get_name(self.as_raw_ptr()) };
> if name.is_null() {
> return Err(Error::NullString("GPIO line's name"));
> }
> @@ -79,14 +89,14 @@ impl Info {
> /// the line is used and we can't request it.
> pub fn is_used(&self) -> bool {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - unsafe { gpiod::gpiod_line_info_is_used(self.info) }
> + unsafe { gpiod::gpiod_line_info_is_used(self.as_raw_ptr()) }
> }
>
> /// Get the GPIO line's consumer name.
> pub fn consumer(&self) -> Result<&str> {
> // SAFETY: The string returned by libgpiod is guaranteed to live as long
> // as the `struct Info`.
> - let name = unsafe { gpiod::gpiod_line_info_get_consumer(self.info) };
> + let name = unsafe { gpiod::gpiod_line_info_get_consumer(self.as_raw_ptr()) };
> if name.is_null() {
> return Err(Error::NullString("GPIO line's consumer name"));
> }
> @@ -100,44 +110,44 @@ impl Info {
> /// Get the GPIO line's direction.
> pub fn direction(&self) -> Result<Direction> {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.info) })
> + Direction::new(unsafe { gpiod::gpiod_line_info_get_direction(self.as_raw_ptr()) })
> }
>
> /// Returns true if the line is "active-low", false otherwise.
> pub fn is_active_low(&self) -> bool {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - unsafe { gpiod::gpiod_line_info_is_active_low(self.info) }
> + unsafe { gpiod::gpiod_line_info_is_active_low(self.as_raw_ptr()) }
> }
>
> /// Get the GPIO line's bias setting.
> pub fn bias(&self) -> Result<Option<Bias>> {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.info) })
> + Bias::new(unsafe { gpiod::gpiod_line_info_get_bias(self.as_raw_ptr()) })
> }
>
> /// Get the GPIO line's drive setting.
> pub fn drive(&self) -> Result<Drive> {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.info) })
> + Drive::new(unsafe { gpiod::gpiod_line_info_get_drive(self.as_raw_ptr()) })
> }
>
> /// Get the current edge detection setting of the line.
> pub fn edge_detection(&self) -> Result<Option<Edge>> {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.info) })
> + Edge::new(unsafe { gpiod::gpiod_line_info_get_edge_detection(self.as_raw_ptr()) })
> }
>
> /// Get the current event clock setting used for edge event timestamps.
> pub fn event_clock(&self) -> Result<EventClock> {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.info) })
> + EventClock::new(unsafe { gpiod::gpiod_line_info_get_event_clock(self.as_raw_ptr()) })
> }
>
> /// Returns true if the line is debounced (either by hardware or by the
> /// kernel software debouncer), false otherwise.
> pub fn is_debounced(&self) -> bool {
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - unsafe { gpiod::gpiod_line_info_is_debounced(self.info) }
> + unsafe { gpiod::gpiod_line_info_is_debounced(self.as_raw_ptr()) }
> }
>
> /// Get the debounce period of the line.
> @@ -147,18 +157,54 @@ impl Info {
> #[allow(clippy::unnecessary_cast)]
> // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> Duration::from_micros(unsafe {
> - gpiod::gpiod_line_info_get_debounce_period_us(self.info) as u64
> + gpiod::gpiod_line_info_get_debounce_period_us(self.as_raw_ptr()) as u64
> })
> }
> }
>
> +/// Line info
> +///
> +/// This is the owned counterpart to [InfoRef]. Due to a [Deref] implementation,
> +/// all functions of [InfoRef] can also be called on this type.
> +#[derive(Debug)]
> +pub struct Info {
> + info: *mut gpiod::gpiod_line_info,
> +}
> +
> +// SAFETY: Info models a owned instance whose ownership may be safely
> +// transferred to other threads.
> +unsafe impl Send for Info {}
> +
> +impl Info {
> + /// Converts a owned pointer into an owned instance
> + ///
> + /// Assumes sole ownership over a [gpiod::gpiod_line_info] instance.
> + ///
> + /// SAFETY: The pointer must point to an instance that is valid. After
> + /// constructing an [Info] the pointer MUST NOT be used for any other
> + /// purpose anymore. All interactions with the libgpiod API have to happen
> + /// through this object.
> + pub(crate) unsafe fn from_raw(info: *mut gpiod::gpiod_line_info) -> Info {
> + Info { info }
> + }
> +}
> +
> +impl Deref for Info {
> + type Target = InfoRef;
> +
> + fn deref(&self) -> &Self::Target {
> + // SAFETY: The pointer is valid for the entire lifetime '0. Info is not
> + // Sync. Therefore, no &Info may be held by a different thread. Hence,
> + // the current thread owns it. Since we borrow with the lifetime of '0,
> + // no move to a different thread can occur while a reference remains
> + // being hold.
> + unsafe { InfoRef::from_raw(self.info) }
> + }
> +}
> +
> impl Drop for Info {
> fn drop(&mut self) {
> - // We must not free the Line info object created from `struct chip::Event` by calling
> - // libgpiod API.
> - if !self.contained {
> - // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> - unsafe { gpiod::gpiod_line_info_free(self.info) }
> - }
> + // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
> + unsafe { gpiod::gpiod_line_info_free(self.info) }
> }
> }
>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
--
viresh
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling
2023-10-03 9:39 ` [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling Erik Schilling
2023-10-03 10:07 ` Viresh Kumar
@ 2023-10-04 11:22 ` Bartosz Golaszewski
1 sibling, 0 replies; 11+ messages in thread
From: Bartosz Golaszewski @ 2023-10-04 11:22 UTC (permalink / raw)
To: Erik Schilling; +Cc: Linux-GPIO, Viresh Kumar, Manos Pitsidianakis, Kent Gibson
On Tue, Oct 3, 2023 at 11:40 AM Erik Schilling
<erik.schilling@linaro.org> wrote:
>
> While attention was provided to prevent freeing in non-owned use-cases,
> the lifetime of these object was not properly modeled.
>
> The line_info from an event may only be used for as long as the event
> exists.
>
> This allowed us to write unsafe-free Rust code that causes a
> use-after-free:
>
> let event = chip.read_info_event().unwrap();
> let line_info = event.line_info().unwrap();
> drop(event);
> dbg!(line_info.name().unwrap());
>
> Which makes the AddressSanitizer scream:
>
> ==90154==ERROR: AddressSanitizer: heap-use-after-free on address 0x50b000005dc4 at pc 0x55a4f883a009 bp 0x7f60ac8fbbc0 sp 0x7f60ac8fb388
> READ of size 2 at 0x50b000005dc4 thread T2
> [...]
> #3 0x55a4f8c3d5f3 in libgpiod::line_info::Info::name::h5ba0bfd360ecb405 libgpiod/bindings/rust/libgpiod/src/line_info.rs:70:18
> [...]
>
> 0x50b000005dc4 is located 4 bytes inside of 112-byte region [0x50b000005dc0,0x50b000005e30)
> freed by thread T2 here:
> [...]
> #1 0x7f60b07f7e31 in gpiod_info_event_free libgpiod/lib/info-event.c:61:2
> [...]
>
> previously allocated by thread T2 here:
> #0 0x55a4f88b04be in malloc /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:69:3
> #1 0x7f60b07f8ff0 in gpiod_line_info_from_uapi libgpiod/lib/line-info.c:144:9
>
> The fix is to distinguish between the owned and non-owned variants and
> assigning lifetimes to non-owned variants.
>
> For modeling the non-owned type there are a couple of options. The ideal
> solution would be using extern_types [1]. But that is still unstable.
> Instead, we are defining a #[repr(transparent)] wrapper around the opaque
> gpiod_line_info struct and cast the pointer to a reference.
>
> This was recommended on the Rust Discord server as good practise.
> (Thanks to Kyuuhachi, shepmaster, pie_flavor and ilyvion! Also thanks to
> @epilys for a brainstorming on this on #linaro-virtualization IRC).
>
> Of course, determining the lifetimes and casting across the types
> requires some care. So this adds a couple of SAFETY comments that would
> probably also have helped the existing code.
>
> [1] https://github.com/rust-lang/rfcs/blob/master/text/1861-extern-types.md
>
> Fixes: 91f9373 ("bindings: rust: Add libgpiod crate")
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
Applied, thanks!
Bart
^ permalink raw reply [flat|nested] 11+ messages in thread
* [libgpiod][PATCH v3 2/3] bindings: rust: rename {event,settings}_clone to try_clone
2023-10-03 9:39 [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Erik Schilling
2023-10-03 9:39 ` [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling Erik Schilling
@ 2023-10-03 9:39 ` Erik Schilling
2023-10-04 11:22 ` Bartosz Golaszewski
2023-10-03 9:39 ` [libgpiod][PATCH v3 3/3] bindings: rust: allow cloning line::InfoRef -> line::Info Erik Schilling
2023-10-04 11:26 ` [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Bartosz Golaszewski
3 siblings, 1 reply; 11+ messages in thread
From: Erik Schilling @ 2023-10-03 9:39 UTC (permalink / raw)
To: Linux-GPIO; +Cc: Viresh Kumar, Manos Pitsidianakis, Kent Gibson, Erik Schilling
What is getting cloned is already clear from the type. This also aligns
a bit better with similar methods from the `std` crate [1].
[1] https://doc.rust-lang.org/std/index.html?search=try_clone
Link: https://lore.kernel.org/r/CVUKC1HXG1P8.13XIUCCXN95F0@ablu-work
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs | 2 +-
bindings/rust/libgpiod/src/edge_event.rs | 3 ++-
bindings/rust/libgpiod/src/line_settings.rs | 4 ++--
bindings/rust/libgpiod/tests/line_request.rs | 2 +-
4 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
index ad90d7b..8dbb496 100644
--- a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
+++ b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
@@ -34,7 +34,7 @@ fn main() -> libgpiod::Result<()> {
let event = events.next().unwrap()?;
// This will out live `event` and the next read_edge_events().
- let cloned_event = libgpiod::request::Event::event_clone(event)?;
+ let cloned_event = libgpiod::request::Event::try_clone(event)?;
let events = request.read_edge_events(&mut buffer)?;
for event in events {
diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs
index 0c0cfbc..4c940ba 100644
--- a/bindings/rust/libgpiod/src/edge_event.rs
+++ b/bindings/rust/libgpiod/src/edge_event.rs
@@ -25,7 +25,8 @@ use super::{
pub struct Event(*mut gpiod::gpiod_edge_event);
impl Event {
- pub fn event_clone(event: &Event) -> Result<Event> {
+ /// Makes a copy of the event object.
+ pub fn try_clone(event: &Event) -> Result<Event> {
// SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
let event = unsafe { gpiod::gpiod_edge_event_copy(event.0) };
if event.is_null() {
diff --git a/bindings/rust/libgpiod/src/line_settings.rs b/bindings/rust/libgpiod/src/line_settings.rs
index f0b3e9c..41b27e2 100644
--- a/bindings/rust/libgpiod/src/line_settings.rs
+++ b/bindings/rust/libgpiod/src/line_settings.rs
@@ -52,8 +52,8 @@ impl Settings {
unsafe { gpiod::gpiod_line_settings_reset(self.settings) }
}
- /// Makes copy of the settings object.
- pub fn settings_clone(&self) -> Result<Self> {
+ /// Makes a copy of the settings object.
+ pub fn try_clone(&self) -> Result<Self> {
// SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
let settings = unsafe { gpiod::gpiod_line_settings_copy(self.settings) };
if settings.is_null() {
diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
index da22bea..e0ae200 100644
--- a/bindings/rust/libgpiod/tests/line_request.rs
+++ b/bindings/rust/libgpiod/tests/line_request.rs
@@ -272,7 +272,7 @@ mod line_request {
for offset in offsets {
lsettings.set_debounce_period(Duration::from_millis((100 + offset).into()));
lconfig
- .add_line_settings(&[offset as Offset], lsettings.settings_clone().unwrap())
+ .add_line_settings(&[offset as Offset], lsettings.try_clone().unwrap())
.unwrap();
}
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [libgpiod][PATCH v3 2/3] bindings: rust: rename {event,settings}_clone to try_clone
2023-10-03 9:39 ` [libgpiod][PATCH v3 2/3] bindings: rust: rename {event,settings}_clone to try_clone Erik Schilling
@ 2023-10-04 11:22 ` Bartosz Golaszewski
2023-10-04 13:01 ` Erik Schilling
0 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2023-10-04 11:22 UTC (permalink / raw)
To: Erik Schilling; +Cc: Linux-GPIO, Viresh Kumar, Manos Pitsidianakis, Kent Gibson
On Tue, Oct 3, 2023 at 11:40 AM Erik Schilling
<erik.schilling@linaro.org> wrote:
>
> What is getting cloned is already clear from the type. This also aligns
> a bit better with similar methods from the `std` crate [1].
>
> [1] https://doc.rust-lang.org/std/index.html?search=try_clone
>
> Link: https://lore.kernel.org/r/CVUKC1HXG1P8.13XIUCCXN95F0@ablu-work
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
> bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs | 2 +-
> bindings/rust/libgpiod/src/edge_event.rs | 3 ++-
> bindings/rust/libgpiod/src/line_settings.rs | 4 ++--
> bindings/rust/libgpiod/tests/line_request.rs | 2 +-
> 4 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
> index ad90d7b..8dbb496 100644
> --- a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
> +++ b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
> @@ -34,7 +34,7 @@ fn main() -> libgpiod::Result<()> {
> let event = events.next().unwrap()?;
>
> // This will out live `event` and the next read_edge_events().
> - let cloned_event = libgpiod::request::Event::event_clone(event)?;
> + let cloned_event = libgpiod::request::Event::try_clone(event)?;
>
> let events = request.read_edge_events(&mut buffer)?;
> for event in events {
> diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs
> index 0c0cfbc..4c940ba 100644
> --- a/bindings/rust/libgpiod/src/edge_event.rs
> +++ b/bindings/rust/libgpiod/src/edge_event.rs
> @@ -25,7 +25,8 @@ use super::{
> pub struct Event(*mut gpiod::gpiod_edge_event);
>
> impl Event {
> - pub fn event_clone(event: &Event) -> Result<Event> {
> + /// Makes a copy of the event object.
> + pub fn try_clone(event: &Event) -> Result<Event> {
Hi Erik,
This fails to apply on top of current master of libgpiod. Could you verify?
Bart
> // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
> let event = unsafe { gpiod::gpiod_edge_event_copy(event.0) };
> if event.is_null() {
> diff --git a/bindings/rust/libgpiod/src/line_settings.rs b/bindings/rust/libgpiod/src/line_settings.rs
> index f0b3e9c..41b27e2 100644
> --- a/bindings/rust/libgpiod/src/line_settings.rs
> +++ b/bindings/rust/libgpiod/src/line_settings.rs
> @@ -52,8 +52,8 @@ impl Settings {
> unsafe { gpiod::gpiod_line_settings_reset(self.settings) }
> }
>
> - /// Makes copy of the settings object.
> - pub fn settings_clone(&self) -> Result<Self> {
> + /// Makes a copy of the settings object.
> + pub fn try_clone(&self) -> Result<Self> {
> // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
> let settings = unsafe { gpiod::gpiod_line_settings_copy(self.settings) };
> if settings.is_null() {
> diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
> index da22bea..e0ae200 100644
> --- a/bindings/rust/libgpiod/tests/line_request.rs
> +++ b/bindings/rust/libgpiod/tests/line_request.rs
> @@ -272,7 +272,7 @@ mod line_request {
> for offset in offsets {
> lsettings.set_debounce_period(Duration::from_millis((100 + offset).into()));
> lconfig
> - .add_line_settings(&[offset as Offset], lsettings.settings_clone().unwrap())
> + .add_line_settings(&[offset as Offset], lsettings.try_clone().unwrap())
> .unwrap();
> }
>
>
> --
> 2.41.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [libgpiod][PATCH v3 2/3] bindings: rust: rename {event,settings}_clone to try_clone
2023-10-04 11:22 ` Bartosz Golaszewski
@ 2023-10-04 13:01 ` Erik Schilling
0 siblings, 0 replies; 11+ messages in thread
From: Erik Schilling @ 2023-10-04 13:01 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linux-GPIO, Viresh Kumar, Manos Pitsidianakis, Kent Gibson
On Wed Oct 4, 2023 at 1:22 PM CEST, Bartosz Golaszewski wrote:
> On Tue, Oct 3, 2023 at 11:40 AM Erik Schilling
> <erik.schilling@linaro.org> wrote:
> >
> > What is getting cloned is already clear from the type. This also aligns
> > a bit better with similar methods from the `std` crate [1].
> >
> > [1] https://doc.rust-lang.org/std/index.html?search=try_clone
> >
> > Link: https://lore.kernel.org/r/CVUKC1HXG1P8.13XIUCCXN95F0@ablu-work
> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> > ---
> > bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs | 2 +-
> > bindings/rust/libgpiod/src/edge_event.rs | 3 ++-
> > bindings/rust/libgpiod/src/line_settings.rs | 4 ++--
> > bindings/rust/libgpiod/tests/line_request.rs | 2 +-
> > 4 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
> > index ad90d7b..8dbb496 100644
> > --- a/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
> > +++ b/bindings/rust/libgpiod/examples/buffered_event_lifetimes.rs
> > @@ -34,7 +34,7 @@ fn main() -> libgpiod::Result<()> {
> > let event = events.next().unwrap()?;
> >
> > // This will out live `event` and the next read_edge_events().
> > - let cloned_event = libgpiod::request::Event::event_clone(event)?;
> > + let cloned_event = libgpiod::request::Event::try_clone(event)?;
> >
> > let events = request.read_edge_events(&mut buffer)?;
> > for event in events {
> > diff --git a/bindings/rust/libgpiod/src/edge_event.rs b/bindings/rust/libgpiod/src/edge_event.rs
> > index 0c0cfbc..4c940ba 100644
> > --- a/bindings/rust/libgpiod/src/edge_event.rs
> > +++ b/bindings/rust/libgpiod/src/edge_event.rs
> > @@ -25,7 +25,8 @@ use super::{
> > pub struct Event(*mut gpiod::gpiod_edge_event);
> >
> > impl Event {
> > - pub fn event_clone(event: &Event) -> Result<Event> {
> > + /// Makes a copy of the event object.
> > + pub fn try_clone(event: &Event) -> Result<Event> {
>
> Hi Erik,
>
> This fails to apply on top of current master of libgpiod. Could you verify?
Hm. Not sure what went wrong. It rebased and tests cleanly for me.
Resent them.
- Erik
>
> Bart
>
> > // SAFETY: `gpiod_edge_event` is guaranteed to be valid here.
> > let event = unsafe { gpiod::gpiod_edge_event_copy(event.0) };
> > if event.is_null() {
> > diff --git a/bindings/rust/libgpiod/src/line_settings.rs b/bindings/rust/libgpiod/src/line_settings.rs
> > index f0b3e9c..41b27e2 100644
> > --- a/bindings/rust/libgpiod/src/line_settings.rs
> > +++ b/bindings/rust/libgpiod/src/line_settings.rs
> > @@ -52,8 +52,8 @@ impl Settings {
> > unsafe { gpiod::gpiod_line_settings_reset(self.settings) }
> > }
> >
> > - /// Makes copy of the settings object.
> > - pub fn settings_clone(&self) -> Result<Self> {
> > + /// Makes a copy of the settings object.
> > + pub fn try_clone(&self) -> Result<Self> {
> > // SAFETY: `gpiod_line_settings` is guaranteed to be valid here.
> > let settings = unsafe { gpiod::gpiod_line_settings_copy(self.settings) };
> > if settings.is_null() {
> > diff --git a/bindings/rust/libgpiod/tests/line_request.rs b/bindings/rust/libgpiod/tests/line_request.rs
> > index da22bea..e0ae200 100644
> > --- a/bindings/rust/libgpiod/tests/line_request.rs
> > +++ b/bindings/rust/libgpiod/tests/line_request.rs
> > @@ -272,7 +272,7 @@ mod line_request {
> > for offset in offsets {
> > lsettings.set_debounce_period(Duration::from_millis((100 + offset).into()));
> > lconfig
> > - .add_line_settings(&[offset as Offset], lsettings.settings_clone().unwrap())
> > + .add_line_settings(&[offset as Offset], lsettings.try_clone().unwrap())
> > .unwrap();
> > }
> >
> >
> > --
> > 2.41.0
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* [libgpiod][PATCH v3 3/3] bindings: rust: allow cloning line::InfoRef -> line::Info
2023-10-03 9:39 [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Erik Schilling
2023-10-03 9:39 ` [libgpiod][PATCH v3 1/3] bindings: rust: fix soundness of line_info modeling Erik Schilling
2023-10-03 9:39 ` [libgpiod][PATCH v3 2/3] bindings: rust: rename {event,settings}_clone to try_clone Erik Schilling
@ 2023-10-03 9:39 ` Erik Schilling
2023-10-04 11:26 ` Bartosz Golaszewski
2023-10-04 11:26 ` [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Bartosz Golaszewski
3 siblings, 1 reply; 11+ messages in thread
From: Erik Schilling @ 2023-10-03 9:39 UTC (permalink / raw)
To: Linux-GPIO; +Cc: Viresh Kumar, Manos Pitsidianakis, Kent Gibson, Erik Schilling
While one would usually use the ToOwned [1] contract in rust, libgpipd's
API only allows copying that may fail.
Thus, we cannot implement the existing trait and roll our own method. I
went with `try_clone` since that seems to be used in similar cases across
the `std` crate [2].
It also closes the gap of not having any way to clone owned instances.
Though - again - not through the Clone trait which may not fail [3].
[1] https://doc.rust-lang.org/std/borrow/trait.ToOwned.html
[2] https://doc.rust-lang.org/std/index.html?search=try_clone
[3] https://doc.rust-lang.org/std/clone/trait.Clone.html
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
---
bindings/rust/libgpiod/src/lib.rs | 1 +
bindings/rust/libgpiod/src/line_info.rs | 16 ++++++++++
bindings/rust/libgpiod/tests/line_info.rs | 53 +++++++++++++++++++++++++++++++
3 files changed, 70 insertions(+)
diff --git a/bindings/rust/libgpiod/src/lib.rs b/bindings/rust/libgpiod/src/lib.rs
index 3acc98c..fd95ed2 100644
--- a/bindings/rust/libgpiod/src/lib.rs
+++ b/bindings/rust/libgpiod/src/lib.rs
@@ -74,6 +74,7 @@ pub enum OperationType {
LineConfigSetOutputValues,
LineConfigGetOffsets,
LineConfigGetSettings,
+ LineInfoCopy,
LineRequestReconfigLines,
LineRequestGetVal,
LineRequestGetValSubset,
diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
index 2148789..bd290f6 100644
--- a/bindings/rust/libgpiod/src/line_info.rs
+++ b/bindings/rust/libgpiod/src/line_info.rs
@@ -58,6 +58,22 @@ impl InfoRef {
self as *const _ as *mut _
}
+ /// Clones the line info object.
+ pub fn try_clone(&self) -> Result<Info> {
+ // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
+ let copy = unsafe { gpiod::gpiod_line_info_copy(self.as_raw_ptr()) };
+ if copy.is_null() {
+ return Err(Error::OperationFailed(
+ crate::OperationType::LineInfoCopy,
+ errno::errno(),
+ ));
+ }
+
+ // SAFETY: The copy succeeded, we are the owner and stop using the
+ // pointer after this.
+ Ok(unsafe { Info::from_raw(copy) })
+ }
+
/// Get the offset of the line within the GPIO chip.
///
/// The offset uniquely identifies the line on the chip. The combination of the chip and offset
diff --git a/bindings/rust/libgpiod/tests/line_info.rs b/bindings/rust/libgpiod/tests/line_info.rs
index ce66a60..d02c9ea 100644
--- a/bindings/rust/libgpiod/tests/line_info.rs
+++ b/bindings/rust/libgpiod/tests/line_info.rs
@@ -19,6 +19,10 @@ mod line_info {
const NGPIO: usize = 8;
mod properties {
+ use std::thread;
+
+ use libgpiod::{line, request};
+
use super::*;
#[test]
@@ -271,5 +275,54 @@ mod line_info {
assert!(info.is_debounced());
assert_eq!(info.debounce_period(), Duration::from_millis(100));
}
+
+ fn generate_line_event(chip: &Chip) {
+ let mut line_config = line::Config::new().unwrap();
+ line_config
+ .add_line_settings(&[0], line::Settings::new().unwrap())
+ .unwrap();
+
+ let mut request = chip
+ .request_lines(Some(&request::Config::new().unwrap()), &line_config)
+ .unwrap();
+
+ let mut new_line_config = line::Config::new().unwrap();
+ let mut settings = line::Settings::new().unwrap();
+ settings.set_direction(Direction::Output).unwrap();
+ new_line_config.add_line_settings(&[0], settings).unwrap();
+ request.reconfigure_lines(&new_line_config).unwrap();
+ }
+
+ #[test]
+ fn ownership() {
+ let sim = Sim::new(Some(1), None, false).unwrap();
+ sim.set_line_name(0, "Test line").unwrap();
+ sim.enable().unwrap();
+
+ let chip = Chip::open(&sim.dev_path()).unwrap();
+
+ // start watching line
+ chip.watch_line_info(0).unwrap();
+
+ generate_line_event(&chip);
+
+ // read generated event
+ let event = chip.read_info_event().unwrap();
+ let info = event.line_info().unwrap();
+ assert_eq!(info.name().unwrap(), "Test line");
+
+ // clone info and move to separate thread
+ let info = info.try_clone().unwrap();
+
+ // drop the original event with the associated line_info
+ drop(event);
+
+ // assert that we can still read the name
+ thread::scope(|s| {
+ s.spawn(move || {
+ assert_eq!(info.name().unwrap(), "Test line");
+ });
+ });
+ }
}
}
--
2.41.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes
2023-10-03 9:39 [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Erik Schilling
` (2 preceding siblings ...)
2023-10-03 9:39 ` [libgpiod][PATCH v3 3/3] bindings: rust: allow cloning line::InfoRef -> line::Info Erik Schilling
@ 2023-10-04 11:26 ` Bartosz Golaszewski
2023-10-04 13:05 ` Erik Schilling
3 siblings, 1 reply; 11+ messages in thread
From: Bartosz Golaszewski @ 2023-10-04 11:26 UTC (permalink / raw)
To: Erik Schilling; +Cc: Linux-GPIO, Viresh Kumar, Manos Pitsidianakis, Kent Gibson
On Tue, Oct 3, 2023 at 11:40 AM Erik Schilling
<erik.schilling@linaro.org> wrote:
>
> While reviewing the bindings for thread-safety, I realized that the
> bindings did not properly model the lifetimes of non-owned line_info
> instances.
>
> This fixes that. It might be a bit mind bending. I tried to provide
> lengthy comments to clarify what happens.
>
> To: Linux-GPIO <linux-gpio@vger.kernel.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> Cc: Kent Gibson <warthog618@gmail.com>
>
> Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> ---
Please make sure to include my brgl@bgdev.pl address too in the future
when submitting libgpiod patches, thanks!
Bart
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes
2023-10-04 11:26 ` [libgpiod][PATCH v3 0/3] bindings: rust: fix modeling of line_info lifetimes Bartosz Golaszewski
@ 2023-10-04 13:05 ` Erik Schilling
0 siblings, 0 replies; 11+ messages in thread
From: Erik Schilling @ 2023-10-04 13:05 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Linux-GPIO, Viresh Kumar, Manos Pitsidianakis, Kent Gibson
On Wed Oct 4, 2023 at 1:26 PM CEST, Bartosz Golaszewski wrote:
> On Tue, Oct 3, 2023 at 11:40 AM Erik Schilling
> <erik.schilling@linaro.org> wrote:
> >
> > While reviewing the bindings for thread-safety, I realized that the
> > bindings did not properly model the lifetimes of non-owned line_info
> > instances.
> >
> > This fixes that. It might be a bit mind bending. I tried to provide
> > lengthy comments to clarify what happens.
> >
> > To: Linux-GPIO <linux-gpio@vger.kernel.org>
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org>
> > Cc: Kent Gibson <warthog618@gmail.com>
> >
> > Signed-off-by: Erik Schilling <erik.schilling@linaro.org>
> > ---
>
> Please make sure to include my brgl@bgdev.pl address too in the future
> when submitting libgpiod patches, thanks!
Thanks for merging!
Will do! Though, you should probably update the CONTRIBUTING section in
the README with that and clarify whether you want to receive it on CC or
TO :->.
I will update my b4 config accordingly.
- Erik
^ permalink raw reply [flat|nested] 11+ messages in thread