soundness for capsule reference and name

[davidhewitt]

Extension of #5229

I could not push to that PR.

I'm too tired to continue tonight, will most likely revisit this tomorrow evening.

[davidhewitt]

I changed this because the &CStr had the same problematic lifetime as .reference(). Also CStr::from_ptr is linear complexity so this is potentially more efficient depending on what the user is doing with it.

[Icxolu]

Maybe we can use Option<NonNull<c_char>> here, to enforce the check for the null pointer?

[davidhewitt]

I played around with this just now but the NonNull doesn't carry the const-ness of the value. It's also a fairly nested type, overall it just didn't feel that good.

One option is we could have a type CapsuleName which has an unsafe .as_cstr() method. (unsafe because it's legal for callers to change the capsule name later.)

[davidhewitt]

(So -> PyResult<Option<CapsuleName>>)

[davidhewitt]

I wonder if the new names should be introduced as get_pointer, get_reference etc, which are verbose but not overly so.

[honzasp]

Thanks for taking over and extending the PR :) As a user of PyO3 I'd prefer the xxx_checked() names rather than get_xxx(), because using the get_ prefix in Rust is frowned upon, and there is a strong precedent of the checked/unchecked dichotomy in the standard library.

I think it would be best to eventually rename pointer_checked() back to pointer(), after a sufficiently long period of the old unchecked pointer() being deprecated.

By the way, thanks for all the work that you put into this excellent crate!

[davidhewitt]

I took another pass at this file.

I added SAFETY notes on the whole file as per #5487, and attempted to use CapsuleName to make the name consumption slightly more ergonomic / type-safe.

Possibly in the future it might be correct to use PyResult<Option<NonNull<CStr>> as the return type for .name(), but at the moment NonNull<CStr> is a wide pointer and creating it incurs a length check (which may change in future).

[Tpt]

Looks great, just a comment to make sure I understood the design properly

[Tpt]

Just to make sure: I guess the use case of the CapsuleName type is because it's lifetime is unknown and so it's nicer to make .as_cstr() the unsafe dereference-like function that assume the pointed value is still allocated, just like pointers?

[davidhewitt]

Yep precisely. I wasn't convinced that NonNull<c_char> was easy enough to use correctly in isolation, and NonNull<CStr> was better but currently costs a length check to create (because it's a fat pointer). So this abstraction will exist for now, maybe if CStr changes to be thin in the future we can simplify.

[Tpt]

Thanks! Makes perfect sense!