Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark free_str function as unsafe #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kitcatier
Copy link

@kitcatier kitcatier commented Mar 19, 2023

fastrank/src/lib.rs

Lines 77 to 79 in 4d3b57f

pub extern "C" fn free_str(originally_from_rust: *mut c_void) {
let _will_drop: CString = unsafe { CString::from_raw(originally_from_rust as *mut c_char) };
}

Hello, this function needs the unsafe keyword, because when we use the from_raw method, we are converting a raw pointer into a CString object, which is an unsafe operation. It requires us to guarantee that the passed pointer is valid, the pointed-to memory is not freed, and it meets the constraints of the CString type (i.e., null-terminated). Therefore, to avoid potential memory safety issues, we must use the unsafe keyword to mark this operation and explicitly indicate that it is an unsafe operation.
https://doc.rust-lang.org/std/ffi/struct.CString.html#method.from_raw
it is not a good choice to mark the entire function body as unsafe, which will make the caller ignore the safety requirements that the function parameters must guarantee, the developer who calls the free_str function may not notice this safety requirement.
Marking them unsafe also means that callers must make sure they know what they're doing.

Copy link
Owner

@jjfiv jjfiv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kitcatier Thank you for bringing this up; while I agree that these functions should be marked unsafe in a rust library; these are not a rust library, but exposing the rust library to C (so it can be called from python).

In almost any way that matters, the entire module and entire C API is unsafe, like all C APIs. I'm supportive of these changes for the single-line "free" functions, but I like having the absolutely minimal unsafe blocks that are here, because I want the unsafe parts of the FFI bindings to be obvious, and have as much "safe" code as possible in these unsafe blocks.

Perhaps the "this entire module is unsafe" could be documented better.

This module should probably be rewritten to use PyO3, but last I looked there was no e.g., numpy array support in the more stable abi3 in Python, so I've left the "raw FFI" but there may be some better tools for that now.

pub extern "C" fn cqrel_query_json(cqrel: *const CQRel, query_str: *const c_void) -> *const c_void {
let cqrel: Option<&CQRel> = unsafe { (cqrel as *mut CQRel).as_ref() };
pub unsafe extern "C" fn cqrel_query_json(cqrel: *const CQRel, query_str: *const c_void) -> *const c_void {
let cqrel: Option<&CQRel> = (cqrel as *mut CQRel).as_ref();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this function and the following, I was following the philosphy of "the smaller unsafe blocks the better". I don't want to mark the whole functions as unsafe because then more unsafe behavior can be added to the function without the compiler noticing.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The philosophy of "the smaller the unsafe block, the better" is very correct

@jjfiv
Copy link
Owner

jjfiv commented Mar 25, 2023

#56 introduced to have macros write this unsafe for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants