Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Py::from_owned_ptr_or_panic (and or_err) is marked safe, but sounds unsafe #95

Closed
ExpHP opened this issue Dec 13, 2017 · 7 comments
Closed

Comments

@ExpHP
Copy link

ExpHP commented Dec 13, 2017

https://pyo3.github.io/pyo3/pyo3/struct.Py.html#method.from_owned_ptr_or_panic
https://pyo3.github.io/pyo3/pyo3/struct.Py.html#method.from_owned_ptr_or_err

Are these missing unsafe fn, or is the documentation wrong?

@fafhrd91
Copy link
Contributor

I made them safe because of panic, developer needs to make sure ptr is valid. Unconditional fns are unsafe.

I don’t have strong preferences, we can change

@fafhrd91
Copy link
Contributor

fafhrd91 commented Dec 13, 2017

@ExpHP would you like to join organizationand work on improvements?

@ExpHP
Copy link
Author

ExpHP commented Dec 13, 2017

I made them safe because of panic, developer needs to make sure ptr is valid. Unconditional fns are unsafe.

The Rust philosophy of unsafe in general is that it must not be possible to invoke undefined behavior without writing unsafe. So if a safe code example can generate UB:

// You can generate an invalid pointer in safe code
Py::<()>::from_owned_ptr_or_panic(&3i32 as *const _ as *mut _)

then one of the functions called in it should be marked unsafe.

@ExpHP do you want to join organization ion and work on improvements?

Possibly. At this point though I'm still trying to consider whether or not it's even a good idea for me to be trying to use Rust and python together right now, so it's possible I might just contribute for one day and then disappear. :P

I did recently work on a project in C++ that had its own CPython bindings, so it's possible I might be able to contribute ideas from that experience. (though Rust adds a whole new dimension of design with its lifetimes; it's interesting to see some of the ideas that this library has!)

@fafhrd91
Copy link
Contributor

fafhrd91 commented Dec 13, 2017 via email

@ExpHP
Copy link
Author

ExpHP commented Dec 13, 2017

On the contrary. Normally, just about the only thing you can possibly do with an expression like &3i32 as *const as *mut _ in safe code is to debug print the address or convert it into an integer. Dereferencing it or calling any ffi function are both unsafe.

@fafhrd91
Copy link
Contributor

I agree with your reasoning, I am fine with adding unsafe.

@fafhrd91
Copy link
Contributor

fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants