ivila commented on PR #209:
URL:
https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/209#issuecomment-3061572542
> > Hi @ivila,
> > Thanks for the implementation! I’ve reviewed the `ObjectHandle`
abstraction and would like to confirm my understanding — please correct me if
anything is off:
> >
> > * `raw::TEE_ObjectHandle` is the low-level FFI type used at the Rust-C
boundary.
> > * `ObjectHandle` is the high-level Rust wrapper that encapsulates the
FFI handle, offering a safer and more idiomatic interface.
> > * The conversion currently always succeed: `ObjectHandle` can be
constructed from `raw::TEE_ObjectHandle`, and raw access is only available via
`handle()` or `handle_mut()`.
> >
> > Given this design, I’d like to suggest revisiting the decision to
support a "null" handle (`ObjectHandle::new_null()`) in safe Rust code.
> > The main use case, as I understand, is that a handle may initially be
`null` when obtained from FFI (e.g., via out-params in C APIs). That makes
sense at the raw layer. However, `ObjectHandle` is a Rust abstraction, and
ideally, it should enforce the invariant that **only valid (non-null) handles**
are held inside. This aligns with Rust’s convention: if a value exists, it
should be valid by construction.
> > ### Suggestion:
> > To better leverage Rust’s type system, could we refine the design as
follows?
> >
> > * **Remove** the `ObjectHandle::new_null()` constructor.
> > * Introduce `TryFrom<raw::TEE_ObjectHandle>` implementation for
`ObjectHandle`, where we check for null and return a `Result<Self, Error>`.
> > * Ensure that all entry points from the FFI boundary perform this
validation before returning an `ObjectHandle`.
> >
> > This would make `ObjectHandle` strictly non-null and eliminate the need
for an `is_null()` method entirely in most cases. It also communicates clearly
to developers that once they have an `ObjectHandle`, it’s safe to use without
extra checks.
> > The code pattern might be like this after the refinement.
> > ```
> > let mut raw_handle: raw::TEE_ObjectHandle = core::ptr::null_mut();
> >
> > let status = unsafe {
> > raw::TEE_OpenPersistentObject(
> > storage_id as u32,
> > object_id.as_ptr() as _,
> > object_id.len(),
> > flags.bits(),
> > &mut raw_handle,
> > )
> > };
> >
> > if status == raw::TEE_SUCCESS {
> > // Convert to safe Rust wrapper
> > return ObjectHandle::try_from(raw_handle);
> > } else {
> > return Err(Error::from_raw_error(status));
> > }
> > ```
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > Of course, I’m not entirely sure whether this would cover **all** use
cases — feel free to share your thoughts or if there are edge cases we should
consider.
>
> Agreed. The same issues are occurring with TransientObject as well—and
since TransientObject relies heavily on ObjectHandle::new_null(), I’ll
investigate further and determine what changes are needed for TransientObject.
@m4sterchain After further consideration, I just make `new_null`, `is_null`
and `handle_mut` function `pub(crate)` to prevent further significant changes
to TransientObject (which should be done eventually, but isn’t the focus of
this PR as it heavily related to `crypto_op`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@teaclave.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@teaclave.apache.org
For additional commands, e-mail: dev-h...@teaclave.apache.org
