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

["Erik Schilling" <erik.schilling@linaro.org>]

On Thu Sep 28, 2023 at 1:27 PM CEST, Viresh Kumar wrote:
> 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 ?

Will fix in v2

>
> >      }
> >  
> >      /// 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 ?

Good catch. Forgot that the lint is not enabled by default... Will fix
in v2.

>
> > +
> > +        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 ?

I got no strong feelings here. I first started with `from_raw`, but switched to
the added suffix since `Info::from_raw` sounded ambigous to me.

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

We would still need to cast it the same way. One _could_ write:

    fn as_raw_ptr(&self) -> *mut gpiod::gpiod_line_info {
        &self.info as *const _ as *mut _
    }

But the cast dance is still required since we need a *mut, but start
with a readonly reference.

This is required since libgpiod's C lib keeps the struct internals
opaque and does not make guarantees about immutable datastructures for
any API calls.

Technically, the 1:1 mapping of this to Rust would be to restrict the
entire API to `&mut self`. One could do that - it would probably allow
us to advertise the structs as `Sync` - but it would require consumers
to declare all libgpiod-related variables as `mut`.

- Erik