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

[Viresh Kumar <viresh.kumar@xxxxxxxxxx>]

On 27-09-23, 18:29, Erik Schilling wrote:
> diff --git a/bindings/rust/libgpiod/src/chip.rs b/bindings/rust/libgpiod/src/chip.rs
> index 81e1be6..02265fc 100644
> --- a/bindings/rust/libgpiod/src/chip.rs
> +++ b/bindings/rust/libgpiod/src/chip.rs
> @@ -95,7 +95,7 @@ impl Chip {
>      }
>  
>      /// Get a snapshot of information about the line.
> -    pub fn line_info(&self, offset: Offset) -> Result<line::Info> {
> +    pub fn line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
>          // SAFETY: The `gpiod_line_info` returned by libgpiod is guaranteed to live as long
>          // as the `struct Info`.
>          let info = unsafe { gpiod::gpiod_chip_get_line_info(self.ichip.chip, offset) };
> @@ -107,12 +107,16 @@ 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 InfoOwned instance.
> +        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
> +
> +        Ok(line_info)

Maybe get rid of the extra `line_info` variable and return directly ?

>      }
>  
>      /// Get the current snapshot of information about the line at given offset and start watching
>      /// it for future changes.
> -    pub fn watch_line_info(&self, offset: Offset) -> Result<line::Info> {
> +    pub fn watch_line_info(&self, offset: Offset) -> Result<line::InfoOwned> {
>          // SAFETY: `gpiod_line_info` is guaranteed to be valid here.
>          let info = unsafe { gpiod::gpiod_chip_watch_line_info(self.ichip.chip, offset) };
>  
> @@ -123,7 +127,11 @@ 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 InfoOwned instance.
> +        let line_info = unsafe { line::InfoOwned::from_raw_owned(info) };
> +
> +        Ok(line_info)

Same here ?

> diff --git a/bindings/rust/libgpiod/src/info_event.rs b/bindings/rust/libgpiod/src/info_event.rs
> index db60600..e88dd72 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::Info> {
>          // 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,9 @@ impl Event {
>              ));
>          }
>  
> -        line::Info::new_from_event(info)
> +        let line_info = unsafe { line::Info::from_raw_non_owning(info) };

SAFETY comment ?

> +
> +        Ok(line_info)
>      }
>  }
>  
> diff --git a/bindings/rust/libgpiod/src/line_info.rs b/bindings/rust/libgpiod/src/line_info.rs
>  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)
> +    /// 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 Info {

I think we can get rid of _non_owning, and _owned later on, from functions since
the parent structure already says so.

Info::from_raw()
InfoOwned::from_raw()

should be good enough ?

> -    /// 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 _

What's wrong with keeping `_info` as `info` in the structure and using it
directly instead of this, since this is private anyway ?

-- 
viresh