Add pub unsafe accessors for object/session handles and ObjectHandle new

[jhshannon17]

As detailed in this issue, in order to allow for vendor extensions to the standard, raw session/object handles need to be accessible outside of the crate. Creating new methods marked unsafe was the preferred way to allow this extension.

• adds new_from_raw method to ObjectHandle to allow creation
• adds raw_handle to ObjectHandle to get the underlying handle
• adds raw_handle to Session to get the underlying handle

[hug-dev]

Looks nice! Thank you!

Was wondering if you could add a small test using those if that would make sense?

[jhshannon17]

Looks nice! Thank you!

Was wondering if you could add a small test using those if that would make sense?

Absolutely!

Added a test case that resembles what I'm doing and exercises the new functions. Made small changes to the common test mod.

[jhshannon17]

Happy to move/reformat anything if it aligns more with convention.

[hug-dev]

Thanks for the added test! It will be a very nice example to anyone wanting to do similar things 👍

[hug-dev]

If you want to make it easier for now and get the PR merged, you could just add a simple "smoke" test where we try to clone an existing ObjectHandle from its raw handle. Then we can think about how making your more complex and useful example in a separate one?

[wiktor-k]

Folks, is there a reason getting the raw handle is considered unsafe?

As far as I saw in Rust, usually it's the constructor functions that are unsafe (since the compiler cannot check the input for validity, e.g. CStr::from_bytes_with_nul_unchecked) but the accessor functions are not unsafe since it's assumed that the check for validity has already been done elsewhere (e.g. Cstr::as_ptr).

(As a side-note, quite a lot of structures already have inner accessors via AsRef<INNER_TYPE> and friends).

Thanks for your input!

[hug-dev]

is there a reason getting the raw handle is considered unsafe?

I thought about the same when reading as well! I also agree that it should be ok to make them safe as getting the value should not be a safety issue per se!

[wiktor-k]

is there a reason getting the raw handle is considered unsafe?

I thought about the same when reading as well! I also agree that it should be ok to make them safe as getting the value should not be a safety issue per se!

Great to hear. I think just extending the visibility of handle from pub(crate) to pub would be sufficient.

Just as a curiosity, Rust docs for CStr::as_ptr also contain a warning about pointer misuse:

WARNING
The returned pointer is read-only; writing to it (including passing it to C code that writes to it) causes undefined behavior.
It is your responsibility to make sure that the underlying memory is not freed too early. For example, the following code will cause undefined behavior when ptr is used inside the unsafe block:

and even though that function is not marked unsafe. I think we could add documentation similar to this as well and consider the job well done 😃

[jhshannon17]

I'm very happy to remove the new handle accessors and just change the existing ones to pub.

Are there any other feelings for the ObjectHandle new?

[wiktor-k]

Are there any other feelings for the ObjectHandle new?

new_from_raw is good 👌 I wonder if we could/should replace all calls to new within the library to new_from_raw because that's basically what it does... not sure how that change would propagate.

FWIW I'm okay with what's in here already, just wondering if there are more "low hanging fruit" to be harvested ;)

[jhshannon17]

Are there any other feelings for the ObjectHandle new?

new_from_raw is good 👌 I wonder if we could/should replace all calls to new within the library to new_from_raw because that's basically what it does... not sure how that change would propagate.

FWIW I'm okay with what's in here already, just wondering if there are more "low hanging fruit" to be harvested ;)

Happy to harvest whatever is helpful - there are a number of places where new is currently being used that would introduce new unsafe blocks (mechanism/hkdf.rs, mechanism/kbkdf.rs) is that fine?

The other places it's used are just outside unsafe blocks so small to change.

[jhshannon17]

If you want to make it easier for now and get the PR merged, you could just add a simple "smoke" test where we try to clone an existing ObjectHandle from its raw handle. Then we can think about how making your more complex and useful example in a separate one?

Did this for now just to exercise the new ObjectHandle::new_from_raw.

I wonder if we could/should replace all calls to new within the library to new_from_raw because that's basically what it does... not sure how that change would propagate.

And still happy to make these changes if the two new unsafe blocks are fine.

[hug-dev]

Thank you for going through the comments! For me it's good as is. We can change all calls from new_from_raw to new in a later PR for all handle types.

The CI is failing, maybe #318 should be merged before?

[jhshannon17]

The CI is failing, maybe #318 should be merged before?

Gotcha, will rebase when available.

[hug-dev]

You can try now!

[jhshannon17]

pushed!

[wiktor-k]

Happy to have this finally merged 🥳

Thanks for your work @jhshannon17 ! 🙇

[jhshannon17]

Excellent, thank you all!