Fix various issues in ockam_ffi.
#2131
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
thomcc
commented
Oct 30, 2021
| where | ||
| F: FnOnce() -> Result<(), FfiOckamError>, | ||
| { | ||
| let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); |
There was a problem hiding this comment.
AssertUnwindSafe here doesn't really matter — UnwindSafe is dubious at best, is unrelated to safety, and we aren't in a position to properly represent it anyway (as propagating the unwinding would be UB).
| struct AbortOnDrop; | ||
| impl Drop for AbortOnDrop { | ||
| fn drop(&mut self) { | ||
| eprintln!("Panic from error drop, aborting!"); |
There was a problem hiding this comment.
In principal, this eprintln! could panic (if stderr is closed, for example), but this Drop can only be called in response to a panic, and Rust calls core::intrinsics::abort if it runs a Drop that panics while it's already panicking.
That is to say, we could implement this Drop as panic!("Panic from error drop, aborting!"), but this has notably better user experience, as core::intrinsics::abort is a SIGILL (on x86), not a SIGABRT.
thomcc
force-pushed
the
thomcc/ffi-cleanup
branch
from
c623001 to
cd332f9
Compare
|
Merged as #2138 - thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I had some computer issues this morning that I fixed by updating the OS, which meant that I wasn't on my primary dev machine for the day (I forgot to switch back after the update completed).
As a result of this, I decided to do my audit of the FFI code, which I had been meaning to do for a while (since at Mozilla, this kind of FFI code was my responsibility, so I know some non-obvious pitfalls to watch out for, and the solutions to them).
Fixes #1562, as well as the fact that we unwind from
extern "C"fns, which is UB (fixing this robustly is a bit tricky, but not too bad — doing this correctly is a bit involved, but the implementation here should be robust.I also cleaned up a use of
#[cfg(feature = "bls")]when there's noblsfeature, which is not that important, but caused a false positive error in my editor.