Impl drop for HandleData

[urschrei]

This a draft PR to investigate the possibility of improving the safety of the network functionality.

ATM, this is just a move of the pointer-reconstitution logic into a Drop impl (This is safer than assuming a function will be called, although of course drop() isn't called on panic), but there will hopefully be more to come when I've done some more digging.

This also fixes some unnecessary mutability and borrows, and makes the struct we pass to libproj repr c.

[urschrei]

I'm still trying to figure out whether there's anything Drop-related I've failed to take account of when returning early / bubbling an error (this is a potential problem with the existing code too)

[urschrei]

Hmmm. I've just checked (added a print statement to the drop() function) and…it never gets called, irrespective of whether hptr is Some or None. I would expect it to be called sometime after https://github.com/georust/proj/blob/master/src/network.rs#L208 but it never is (and close_network is definitely being called).

[urschrei]

Sooo I think we have been leaking a little bit of memory for…a while

It turns out that let _ = *hd; doesn't actually create a value. You have to re-constitute the boxed value you leaked and cast in the exact reverse order in which you leaked and cast it in the first place.

On the way out: HandleData -> Box<HandleData> -> *mut HandleData -> *mut c_void -> *mut PROJ_NETWORK_HANDLE

With this PR, on the way in (_close_network): *mut PROJ_NETWORK_HANDLE -> *mut c_void -> *mut HandleData -> Box<HandleData>

And this triggers the new manual Drop impl.

[urschrei]

You can also apparently transmute handle directly to a Box<HandleData> using

let _: Box<HandleData> = std::mem::transmute(handle as *const c_void as *mut HandleData);

but everything I've read says you should really avoid transmute unless you're absolutely sure you know what you're doing, which…well, we know that isn't the case.

[urschrei]

More investigation reveals that Option<*mut c_char> is not FFI-safe, and a simplified example run with Miri shows that it does indeed leak memory. However, if we alter the signature to Option<NonNull<c_char>> which can be built from *mut c_char, i.e. the result of a CString::into_raw call, nothing leaks.

[urschrei]

Sorry for the slow review. It's very much outside of what I'm familiar with.

I did spend some time reading about opaque pointers and boxing/ownership stuff and this all seems correct.

I also built and verified that the drop implementation is at least getting called sometimes.

So I guess: looks good to me in as as far as I'm good at looking.

And thanks for the explanations — I learned some things.

I was in the same situation til I started asking, and only found out that there are special FFI rules and exceptions for Option by accident, so this is also future reference material.

I'm having a last niggling uncertainty about passing around RequestBuilder (HeaderMap is OK), because it can't be verified with Miri – even though we use the Blocking version, it's still Tokio under the hood, which currently prevents analysis. I'm going to just ask on URLO tomorrow, and merge if the consensus is that it's safe.

Thanks for looking at it!

[urschrei]

bors try

[bors[bot]]

try

Build succeeded:

• ci result

[urschrei]

bors r+

[bors[bot]]

Build succeeded:

• ci result

[michaelkirk]

I'm having a last niggling uncertainty about passing around RequestBuilder (HeaderMap is OK), because it can't be verified with Miri – even though we use the Blocking version, it's still Tokio under the hood, which currently prevents analysis. I'm going to just ask on URLO tomorrow, and merge if the consensus is that it's safe.

I see that you ended up removcing the opaque pointer and are rebuilding the request builder from a url. I would have guessed it was safe. Did you learn something that made this seem necessary, or was it more-so an abundance of caution in the face of uncertainty?

To be clear, I'm fine with the change! Just hoping to learn more.

[urschrei]

Did you learn something that made this seem necessary

It potentially contains Option fields, which we now know not to be FFI-safe unless they're wrapping NonNull (as well as Arc and threads, which may or may not be safe), so at that point it didn't seem wise to keep it.

I'm somewhat torn, but the resource-creation overhead is dwarfed by the data retrieval time.

[michaelkirk]

So to make sure I'm following your rationale:

1. We're passing a pointer to C.
2. The thing that's being pointed to has a field (request) which transitively has a field that is not FFI safe.
3. conclusion: Since that field isn't FFI safe, the pointer (1.) is not safe to pass to C.

That seems too scary to be true! Wouldn't that preclude sending almost any pointer to C? And how could anyone expect to do that analysis?

My understanding is that whether a type is FFI-safe refers only to the type in the API - in this case the pointer. Since pointers are FFI safe, end of story. The internal structure of whatever is being pointed to is completely opaque and untouched by C.

So specifically, the concern about Options might be relevant if the api was something like:

struct HandleData {
    request: reqwest::blocking::RequestBuilder,
}
pub(crate) unsafe extern "C" fn foo(
    handle: HandleData, // <-- passing a type (by value) that contains an option
) {
  ...

And indeed if you try to compile that, you'll see:

warning: `extern` fn uses type `HandleData`, which is not FFI-safe
  --> src/network.rs:39:13
   |
39 |     handle: HandleData
   |             ^^^^^^^^^^ not FFI-safe
   |
   = note: `#[warn(improper_ctypes_definitions)]` on by default
   = help: consider adding a `#[repr(C)]` or `#[repr(transparent)]` attribute to this struct
   = note: this struct has unspecified layout

But if we only pass a pointer, since pointers are FFI safe, that should be fine:

pub(crate) unsafe extern "C" fn foo(
    handle: *mut HandleData
) {
    println!("{:?}", (* handle).hptr)
}

And indeed this produces no FFI warning.

I could be way off though. I'm still just figuring this out though...

[michaelkirk]

I'm somewhat torn, but the resource-creation overhead is dwarfed by the data retrieval time.

And to reiterate, I'm not worried at all about the perf repercussions of your change. I think it's totally fine. I'm just trying to get better at rust FFI stuff. Thanks for talking it through with me!