FFI Best Practices Examples Contain Unsoundness and Other Not Great Things

[jhwgh1968]

Per a comment by @simonsan, here is an issue to discuss some changes to #106.

EDIT of maintainer: Feedback of @Michael-F-Bryan can be found here in the PR https://github.com/rust-unofficial/patterns/pull/106#pullrequestreview-561849641

It seems while I was away, the PR was merged with additional fixes. Only after that happened, did @Michael-F-Bryan come along and do a deeper code review than the initial reviewers did.

It is definitely better late than never. I am happy to make changes, but it will take some time. I do not always get time for this hobby of mine, and the original work was done when I had a lot more than I will for a while.

Anyone who wants to beat me to it can open a PR for anything they think is sufficiently glaring. I would be happy to look at anything, and will probably approve unless it messes up the example.

---

For my own part, here is a brief explanation of where all my work came from.

The lessons I wrote -- and the structure of the code examples -- came from a stand-alone hobby project I did to learn Rust several years ago. It was quite an ambitious one, and I am still surprised how far I got.

I converted most of a 15,000 line abandon-ware C library into Rust, while keeping it perfectly ABI compatible with the original. I ran it through a C-to-Rust translation tool, got it barely working in 100% unsafe Rust, and started working my way toward safety.

In so doing, I learned a lot -- but also had some special challenges, and almost certainly missed some things.

I ended up relying on Rust UB in a couple of places specifically because the ABI made me. Even though it is UB, it was often predictable, easy to debug, and paper over. I stopped worrying about things that didn't fall into certain traps in C. In the course of creating these examples, those flaws in the original sneak through sometimes.

The code was also very old, around the Rust 1.15 timeframe. In the process of writing the code examples, I had to do simplifications and modernizations from pre-edition 2018 Rust. It does not surprise me that, when I was unable to test the code I wrote "by eye", it has issues.

---

With that explanation out of the way, I will start with the original comments @Michael-F-Bryan left.

• Unwinding across the FFI boundary is UB, so all calls to non-trivial Rust code in FFI functions must be wrapped in std::panic::catch_unwind()

This is true, and a good point. Perhaps it should be an additional best practice?

I also wonder if it wouldn't clutter some of the examples up more, but perhaps that "realism" is worth it.

• Almost all of your FFI bindings should unsafe fn's because their memory safety depends on the caller doing the right thing, so not marking them as unsafe is technically unsound

Also a good point. I was less rigorous about this during that project due to the way that project developed, where unsafe fn became a marker for "you don't know where all your unsafe blocks are," a habit which transferred over to this text.

• No null pointer checks

I am actually somewhat ambivalent about this.

Among C programmers, there is no simple answer to whether you should write "do not pass NULL pointers" into the documented contract of a function, or put explicit checks everywhere. It really depends on how much you trust your users.

Because the actual behavior that results from null pointer access (despite it being UB) is commonly handled cleanly, and because it was less clutter, I chose the former option.

• You can tell some of the code hasn't been compiled and run, for example the get_error_from_ffi() function will always panic because you call CString::new() with a buffer that contains a null (the terminator)

It seems that I used an unstable function, which does not simply stop at the first null byte as I would have expected. I agree that is important to fix.

• Functions not documenting their safety invariants and whether they borrow/own the values passed in from C

This was also caused by an accident of how this document came about. "Object-based APIs" was going to be a big thing that contained many other things as bullet points. Outside of that, it probably would be a good idea to document.

[simonsan]

I asked for a review in the Rust community discord under # review and no one replied for a few hours, so I thought nobody will review it there, so I deleted my comment. After merging @Michael-F-Bryan sent a comment there that he reviewed it but it got already merged. So it's rather based on a small miscommunication overall. I was assuming people would say: "Hey there, for sure, I can review it." So I would have waited for their review. I've let two other Rust programmers of another community read over it to also get more feedback in, and it was fine.

Nevertheless I think it's fine that it got merged, as we are now able to iterate over it, with more eyes on it and people giving feedback. Improvement by iteration so to say. Thank you for the work you did! :-)

[simonsan]

We should also link to the Unsafe Code Guidelines

[simonsan]

Update: Talked to @Michael-F-Bryan and he said he'll try to make a PR tomorrow which should fix (some or all of) these issues.