Impl drop for HandleData #94

urschrei · 2021-11-17T15:37:45Z

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.

This is safer than assuming a function will be called. Also fixes some unnecessary mutability warnings

michaelkirk

This seems like an improvement! Is there a reason it's only a draft?

michaelkirk · 2021-11-29T20:24:36Z

src/network.rs

@@ -311,7 +318,7 @@ fn _network_read_range(
    // - 1 is used because the HTTP convention is to use inclusive start and end offsets
    let end = offset as usize + size_to_read - 1;
    let hvalue = format!("bytes={}-{}", offset, end);
-    let mut hd = unsafe { &mut *(handle as *const c_void as *mut HandleData) };
+    let hd = unsafe { &mut *(handle as *const c_void as *mut HandleData) };


The current logic is so simple it's probably not worth changing but I wanted to make sure you were aware of @pka's https://github.com/pka/http-range-client - which was pulled out of some of our work on the flatgeobuf client

I was not aware! This looks very interesting…

src/network.rs

urschrei · 2021-11-29T20:37:44Z

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)

As per https://stackoverflow.com/questions/30742004/correct-idiom-for-freeing-reprc-structs-using-drop-trait/30742180#30742180

urschrei · 2021-11-30T15:59:19Z

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 · 2021-11-30T17:46:26Z

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 · 2021-11-30T17:52:17Z

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.

Option<*const c_char> isn't FFI-safe as it's doubly-nullable, and isn't handled under Option's current guaranteed special cases. Using Option<NonNull<c_char>> is allowed, however.

urschrei · 2021-11-30T21:45:52Z

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.

michaelkirk

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.

urschrei · 2021-12-07T23:40:29Z

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!

The RequestBuilder struct is complex and uses Tokio under the hood. Instead of passing it around, we pass the URL around and create a new Client for data retrieval. This is a bit less efficient since we don't use the connection pool created by the initial Client, but it's also safer.

urschrei · 2021-12-08T12:59:31Z

bors try

bors · 2021-12-08T13:07:54Z

try

Build succeeded:

ci result

urschrei · 2021-12-08T13:47:58Z

bors r+

bors · 2021-12-08T13:57:13Z

Build succeeded:

ci result

michaelkirk · 2021-12-08T15:29:08Z

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 · 2021-12-08T15:40:52Z

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 · 2021-12-08T17:02:30Z

So to make sure I'm following your rationale:

We're passing a pointer to C.
The thing that's being pointed to has a field (request) which transitively has a field that is not FFI safe.
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 · 2021-12-08T17:06:02Z

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!

Impl drop for HandleData

e38b545

This is safer than assuming a function will be called. Also fixes some unnecessary mutability warnings

michaelkirk approved these changes Nov 29, 2021

View reviewed changes

urschrei added 2 commits November 30, 2021 13:12

Fix up comment

2f126d9

Fix missing ?

f3d765a

urschrei force-pushed the drop_network branch from 933f331 to f3d765a Compare November 30, 2021 13:46

urschrei added 2 commits November 30, 2021 15:05

No need for repr(C) as struct isn't modified by FFI

c97c9a7

As per https://stackoverflow.com/questions/30742004/correct-idiom-for-freeing-reprc-structs-using-drop-trait/30742180#30742180

Better comments

30dde80

Fix reconstitution of leaked HandleData

7f9e04a

Avoid leaking memory through doubly-nullable pointer

bd92226

Option<*const c_char> isn't FFI-safe as it's doubly-nullable, and isn't handled under Option's current guaranteed special cases. Using Option<NonNull<c_char>> is allowed, however.

urschrei marked this pull request as ready for review December 1, 2021 00:54

urschrei requested a review from michaelkirk December 3, 2021 18:42

michaelkirk approved these changes Dec 7, 2021

View reviewed changes

bors bot added a commit that referenced this pull request Dec 8, 2021

Try #94:

c62b267

urschrei added 2 commits December 8, 2021 13:22

Better error-handler comment, remove unneeded lifetime annotation

0985c5a

Update changelog

76d93bb

bors bot merged commit 787df36 into georust:master Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Impl drop for HandleData #94

Impl drop for HandleData #94

urschrei commented Nov 17, 2021 •

edited

Loading

michaelkirk left a comment

michaelkirk Nov 29, 2021

urschrei Nov 29, 2021

urschrei commented Nov 29, 2021

urschrei commented Nov 30, 2021 •

edited

Loading

urschrei commented Nov 30, 2021

urschrei commented Nov 30, 2021

urschrei commented Nov 30, 2021

michaelkirk left a comment

urschrei commented Dec 7, 2021

urschrei commented Dec 8, 2021

bors bot commented Dec 8, 2021

urschrei commented Dec 8, 2021

bors bot commented Dec 8, 2021

michaelkirk commented Dec 8, 2021

urschrei commented Dec 8, 2021

michaelkirk commented Dec 8, 2021 •

edited

Loading

michaelkirk commented Dec 8, 2021

Impl drop for HandleData #94

Impl drop for HandleData #94

Conversation

urschrei commented Nov 17, 2021 • edited Loading

michaelkirk left a comment

Choose a reason for hiding this comment

michaelkirk Nov 29, 2021

Choose a reason for hiding this comment

urschrei Nov 29, 2021

Choose a reason for hiding this comment

urschrei commented Nov 29, 2021

urschrei commented Nov 30, 2021 • edited Loading

urschrei commented Nov 30, 2021

urschrei commented Nov 30, 2021

urschrei commented Nov 30, 2021

michaelkirk left a comment

Choose a reason for hiding this comment

urschrei commented Dec 7, 2021

urschrei commented Dec 8, 2021

bors bot commented Dec 8, 2021

try

urschrei commented Dec 8, 2021

bors bot commented Dec 8, 2021

michaelkirk commented Dec 8, 2021

urschrei commented Dec 8, 2021

michaelkirk commented Dec 8, 2021 • edited Loading

michaelkirk commented Dec 8, 2021

urschrei commented Nov 17, 2021 •

edited

Loading

urschrei commented Nov 30, 2021 •

edited

Loading

michaelkirk commented Dec 8, 2021 •

edited

Loading