Improve use of unsafe and fix clippy complaints
#35
Conversation
This modifies our use of `unsafe` to prefer unsafe blocks to fully unsafe function declarations, to make it easier to see what operations are actually unsafe within a function, and then adds safety documentation as appropriate.
nuttycom
changed the title
unsafe and fix clippy complaints
| @@ -206,15 +264,25 @@ pub extern "C" fn zcashlc_init_accounts_table( | |||
| unsafe { *capacity_ret.as_mut().unwrap() = v.capacity() }; | |||
| let p = v.as_mut_ptr(); | |||
| std::mem::forget(v); | |||
There was a problem hiding this comment.
I don't think this is a correct pattern. The doc for std::mem::forget says
Any resources the value manages, such as heap memory or a file handle, will linger forever in an unreachable state. However, it does not guarantee that pointers to this memory will remain valid.
As far as I know, the Rust allocator is free to reuse the memory that was associated with v as soon as it goes out of scope, i.e. at the the end of this block. std::mem::forget only prevents the destructor from being run.
There was a problem hiding this comment.
See https://github.com/zcash/zcash/blob/f84a57e71bded0774bcb63e8c205c6db6ce442e1/src/rust/src/zip339_ffi.rs#L39-L72 for an example of how to robustly return a string to C/C++ and let it manage the memory.
There was a problem hiding this comment.
As @nuttycom pointed out privately, this isn't a simple string so we probably need Box::into_raw.
| @@ -297,12 +373,34 @@ pub unsafe extern "C" fn zcashlc_derive_extended_spending_keys( | |||
| unwrap_exc_or_null(res) | |||
There was a problem hiding this comment.
The same incorrect "std::mem::forget then return" pattern is used just above here.
| /// of `1`. | ||
| /// - The memory referenced by `buf` must not be mutated for the duration of the function call. | ||
| /// - The total size `length` must be no larger than `isize::MAX`. See the safety documentation of | ||
| /// pointer::offset. |
There was a problem hiding this comment.
This should be a separate refactor, but I'm really not a fan of stateful error reporting using a thread-local variable to hold the error state (as implemented by the ffi_helpers::error_handling module). Once we have zcash/zcash#4816 (Define a "result" type for use in C++ error handling) we should replace this error reporting with that.
There was a problem hiding this comment.
I'm afraid this won't really work, because the other side of this FFI is not C++; it's Swift.
No description provided.