[libgpiod][PATCH v2 0/3] bindings: rust: fix modeling of line_info lifetimes

[libgpiod][PATCH v2 0/3] bindings: rust: fix modeling of line_info lifetimes

[Erik Schilling]

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>
---
Changes in v2:
- Removed unneeded temporary variables
- Added missing SAFETY comment
- Renamed owning wrapper to `Event`, non-owning to `EventRef`
- Renamed existing clone methods to try_clone()
- Slightly tweaked try_clone() documentation
- Dropped version bump commit
- Added Fixes tag
- CC'd Kent - suggested by vireshk since he reviewed his commits
- Link to v1: https://lore.kernel.org/r/20230927-rust-line-info-soundness-v1-0-990dce6f18ab@linaro.org

---
Erik Schilling (3):
      bindings: rust: fix soundness of line_info modeling
      bindings: rust: rename {event,settings}_clone to try_clone
      bindings: rust: allow cloning line::InfoRef -> line::Info

 .../libgpiod/examples/buffered_event_lifetimes.rs  |   2 +-
 bindings/rust/libgpiod/src/chip.rs                 |   8 +-
 bindings/rust/libgpiod/src/edge_event.rs           |   3 +-
 bindings/rust/libgpiod/src/info_event.rs           |   8 +-
 bindings/rust/libgpiod/src/lib.rs                  |   1 +
 bindings/rust/libgpiod/src/line_info.rs            | 140 +++++++++++++++------
 bindings/rust/libgpiod/src/line_settings.rs        |   4 +-
 bindings/rust/libgpiod/tests/line_info.rs          |  53 ++++++++
 bindings/rust/libgpiod/tests/line_request.rs       |   2 +-
 9 files changed, 173 insertions(+), 48 deletions(-)
---
base-commit: ced90e79217793957b11414f47f8aa8a77c7a2d5
change-id: 20230927-rust-line-info-soundness-14c08e0d26e9

Best regards,
-- 
Erik Schilling <erik.schilling@linaro.org>

[libgpiod][PATCH v2 1/3] bindings: rust: fix soundness of line_info modeling

[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 81e1be6..9ef8f22 100644
--- a/bindings/rust/libgpiod/src/chip.rs
+++ b/bindings/rust/libgpiod/src/chip.rs
@@ -107,7 +107,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_owned(info) })
     }
 
     /// Get the current snapshot of information about the line at given offset and start watching
@@ -123,7 +125,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_owned(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..843f3f7 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_non_owning(info) })
     }
 }
 
diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
index c4f488c..e3ea5e1 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_non_owning<'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_owned(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_non_owning(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

[libgpiod][PATCH v2 2/3] bindings: rust: rename {event,settings}_clone to try_clone

[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
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 9af5226..8731719 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

[libgpiod][PATCH v2 3/3] bindings: rust: allow cloning line::InfoRef -> line::Info

[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

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 e3ea5e1..c9dd379 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_owned(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

Re: [libgpiod][PATCH v2 1/3] bindings: rust: fix soundness of line_info modeling

[Viresh Kumar]

On 29-09-23, 15:18, Erik Schilling wrote:
> diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
> index 81e1be6..9ef8f22 100644
> --- a/bindings/rust/libgpiod/src/chip.rs
> +++ b/bindings/rust/libgpiod/src/chip.rs
> @@ -107,7 +107,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_owned(info) })

Hmm, I was expecting the naming to be simplified in this version here.

Now:

Info::from_raw_owned()
InfoRef::from_raw_non_owning()

What I am suggesting:

Info::from_raw()
InfoRef::from_raw()

Or maybe just `new()` routines for both ?

I think structure names tell us enough about ownership here and we don't need to
add it to functions.

-- 
viresh

Re: [libgpiod][PATCH v2 2/3] bindings: rust: rename {event,settings}_clone to try_clone

[Viresh Kumar]

On 29-09-23, 15:18, Erik Schilling 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
> 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 9af5226..8731719 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();
>              }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [libgpiod][PATCH v2 3/3] bindings: rust: allow cloning line::InfoRef -> line::Info

[Viresh Kumar]

On 29-09-23, 15:18, Erik Schilling wrote:
> 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
> 
> 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 e3ea5e1..c9dd379 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_owned(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");
> +                });
> +            });
> +        }
>      }
>  }

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [libgpiod][PATCH v2 1/3] bindings: rust: fix soundness of line_info modeling

[Erik Schilling]

On Tue Oct 3, 2023 at 10:58 AM CEST, Viresh Kumar wrote:
> On 29-09-23, 15:18, Erik Schilling wrote:
> > diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
> > index 81e1be6..9ef8f22 100644
> > --- a/bindings/rust/libgpiod/src/chip.rs
> > +++ b/bindings/rust/libgpiod/src/chip.rs
> > @@ -107,7 +107,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_owned(info) })
>
> Hmm, I was expecting the naming to be simplified in this version here.
>
> Now:
>
> Info::from_raw_owned()
> InfoRef::from_raw_non_owning()
>
> What I am suggesting:
>
> Info::from_raw()
> InfoRef::from_raw()
>
> Or maybe just `new()` routines for both ?
>
> I think structure names tell us enough about ownership here and we don't need to
> add it to functions.

Ah, I posted some weak opposition against that in [1], but failed to
mention that again for v2. I mostly disliked `Info::from_raw()` being
not very expressive without peeking at the type definition. But if you
feel strongly about this, I am happy to change it.

I would strongly vote for `from_raw()` since `new()` sounds like it
would create a new instance, but really, we are just wrapping an already
existing instance here.

So... Shall I move to `from_raw()` or keep
`from_raw_owned/non_owning()`?

- Erik

[1] https://lore.kernel.org/r/CVUJTBQZYN6B.17WXH28G8MKZ2@ablu-work

Re: [libgpiod][PATCH v2 1/3] bindings: rust: fix soundness of line_info modeling

[Viresh Kumar]

On 03-10-23, 11:17, Erik Schilling wrote:
> Ah, I posted some weak opposition against that in [1], but failed to
> mention that again for v2. I mostly disliked `Info::from_raw()` being
> not very expressive without peeking at the type definition. But if you
> feel strongly about this, I am happy to change it.

I feel `Info` and `InfoRef` speak enough for themselves and what they contain
and I don't see a need for the routines to be named specially anymore.

> I would strongly vote for `from_raw()` since `new()` sounds like it
> would create a new instance, but really, we are just wrapping an already
> existing instance here.
> 
> So... Shall I move to `from_raw()` or keep
> `from_raw_owned/non_owning()`?

I like the simplified version, i.e. `from_raw()` for sure. Unless someone else
have an objection to that.

-- 
viresh