m4sterchain commented on PR #209:
URL:
https://github.com/apache/incubator-teaclave-trustzone-sdk/pull/209#issuecomment-3059642074
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.
--
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
