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

fix(bindings/c): use ManuallyDrop instead of forget to make sure pointer is valid #5166

Merged
merged 1 commit into from
Oct 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions bindings/c/include/opendal.h
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ typedef struct opendal_bytes {
* The length of the byte array
*/
uintptr_t len;
/**
* The capacity of the byte array
*/
uintptr_t capacity;
} opendal_bytes;

/**
Expand Down
26 changes: 19 additions & 7 deletions bindings/c/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,25 +34,37 @@ pub struct opendal_bytes {
pub data: *const u8,
/// The length of the byte array
pub len: usize,
/// The capacity of the byte array
pub capacity: usize,
}

impl opendal_bytes {
/// Construct a [`opendal_bytes`] from the Rust [`Vec`] of bytes
pub(crate) fn new(buf: Buffer) -> Self {
let vec = buf.to_vec();
let data = vec.as_ptr();
let len = vec.len();
std::mem::forget(vec);
Self { data, len }
let mut buf = std::mem::ManuallyDrop::new(vec);
let data = buf.as_mut_ptr();
let len = buf.len();
let capacity = buf.capacity();
Self {
data,
len,
capacity,
}
}

/// \brief Frees the heap memory used by the opendal_bytes
#[no_mangle]
pub unsafe extern "C" fn opendal_bytes_free(ptr: *mut opendal_bytes) {
if !ptr.is_null() {
let data_mut = (*ptr).data as *mut u8;
drop(Vec::from_raw_parts(data_mut, (*ptr).len, (*ptr).len));
drop(Box::from_raw(ptr));
// transmuting `*const u8` to `*mut u8` is undefined behavior in any cases
// however, fields type of `opendal_bytes` is already related to the zig binding
Copy link
Member

Choose a reason for hiding this comment

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

already related to the zig binding

Do you mean that the Zig binding should use non-const pointer also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think so

// it should be fixed later
let _ = Vec::from_raw_parts((*ptr).data as *mut u8, (*ptr).len, (*ptr).capacity);
// it is too weird that call `Box::new` outside `opendal_bytes::new` but dealloc it here
// also, boxing `opendal_bytes` might not be necessary
// `data` points to heap, so `opendal_bytes` could be passed as a stack value
Comment on lines +64 to +66
Copy link
Member

Choose a reason for hiding this comment

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

Reasonable. But IIRC other bindings rely on this interface also. We can update them all in a follow-up.

Then the free method is guarded by !self.data.is_null().

Copy link
Member

@tisonkun tisonkun Oct 8, 2024

Choose a reason for hiding this comment

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

Try in #5171

let _ = Box::from_raw(ptr);
}
}
}
Expand Down
11 changes: 7 additions & 4 deletions bindings/go/types.go
Copy link
Member

Choose a reason for hiding this comment

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

So this is the real reason my previous patch failed, lol.

This can be a hidden knowledge that Go binding's struct should be synced with C binding's ..

Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ var (
typeBytes = ffi.Type{
Type: ffi.Struct,
Elements: &[]*ffi.Type{
&ffi.TypePointer,
&ffi.TypePointer,
&ffi.TypePointer,
nil,
Expand Down Expand Up @@ -257,8 +258,9 @@ type resultStat struct {
type opendalMetadata struct{}

type opendalBytes struct {
data *byte
len uintptr
data *byte
len uintptr
capacity uintptr
}

type opendalError struct {
Expand Down Expand Up @@ -292,8 +294,9 @@ func toOpendalBytes(data []byte) opendalBytes {
ptr = &b
}
return opendalBytes{
data: ptr,
len: uintptr(l),
data: ptr,
len: uintptr(l),
capacity: uintptr(l),
}
}

Expand Down
Loading