From f3eaf7b0b1e0086233bea9250fc3aeb85af27198 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 01:48:09 +0800 Subject: [PATCH 1/7] change the dir&file name in the list test --- bindings/c/tests/list.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index de905f17df5d..8e5e34c27424 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -47,8 +47,8 @@ class OpendalListTest : public ::testing::Test { // Basic usecase of list TEST_F(OpendalListTest, ListDirTest) { - std::string dname = "some_random_dir_name_152312dbfas"; - std::string fname = "some_random_file_name_2138912rbf"; + std::string dname = "some_random_dir_name_152312"; + std::string fname = "some_random_file_name_21389"; // 4 MiB of random bytes uintptr_t nbytes = 4 * 1024 * 1024; From 0ad479d50b23fd94f4626b07bd4608ad1425981f Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 02:26:43 +0800 Subject: [PATCH 2/7] Fix memory leak in bdd test --- bindings/c/include/opendal.h | 9 ++++++--- bindings/c/src/lib.rs | 7 +++++-- bindings/c/src/types.rs | 31 +++++++++++++++---------------- 3 files changed, 26 insertions(+), 21 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index c2aa31721b4f..af75843e875f 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -191,7 +191,7 @@ typedef struct opendal_bytes { /** * Pointing to the byte array on heap */ - const uint8_t *data; + uint8_t *data; /** * The length of the byte array */ @@ -364,7 +364,10 @@ const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, * \brief Blockingly write raw bytes to `path`. * * Write the `bytes` into the `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK - * if succeeds, others otherwise + * if succeeds, others otherwise. + * + * @NOTE It is important to notice that the `bytes` that is passes in will be comsumed by this + * function. * * @param ptr The opendal_operator_ptr created previously * @param path The designated path you want to write your bytes in @@ -653,7 +656,7 @@ void opendal_operator_free(const struct opendal_operator_ptr *op); /** * \brief Frees the heap memory used by the opendal_bytes */ -void opendal_bytes_free(const struct opendal_bytes *self); +void opendal_bytes_free(struct opendal_bytes *ptr); /** * \brief Free the heap-allocated metadata used by opendal_metadata diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index b749a5d6b064..7a01dc25dfbb 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -126,7 +126,10 @@ pub unsafe extern "C" fn opendal_operator_new( /// \brief Blockingly write raw bytes to `path`. /// /// Write the `bytes` into the `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK -/// if succeeds, others otherwise +/// if succeeds, others otherwise. +/// +/// @NOTE It is important to notice that the `bytes` that is passes in will be comsumed by this +/// function. /// /// @param ptr The opendal_operator_ptr created previously /// @param path The designated path you want to write your bytes in @@ -235,7 +238,7 @@ pub unsafe extern "C" fn opendal_operator_blocking_read( let data = op.read(path); match data { Ok(d) => { - let v = Box::new(opendal_bytes::from_vec(d)); + let v = Box::new(opendal_bytes::new(d)); opendal_result_read { data: Box::into_raw(v), code: opendal_code::OPENDAL_OK, diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index e324b508e873..3f23edeafd4a 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -93,31 +93,30 @@ impl From<*mut od::BlockingOperator> for opendal_operator_ptr { #[repr(C)] pub struct opendal_bytes { /// Pointing to the byte array on heap - pub data: *const u8, + pub data: *mut u8, /// The length of the byte array pub len: usize, } -impl opendal_bytes { - /// \brief Frees the heap memory used by the opendal_bytes - #[no_mangle] - pub extern "C" fn opendal_bytes_free(&self) { - unsafe { - // this deallocates the vector by reconstructing the vector and letting - // it be dropped when its out of scope - Vec::from_raw_parts(self.data as *mut u8, self.len, self.len); - } - } -} - impl opendal_bytes { /// Construct a [`opendal_bytes`] from the Rust [`Vec`] of bytes - pub(crate) fn from_vec(vec: Vec) -> Self { - let data = vec.as_ptr() as *const u8; + pub(crate) fn new(vec: Vec) -> Self { + let data = vec.as_ptr() as *mut u8; let len = vec.len(); - std::mem::forget(vec); // To avoid deallocation of the vec. + std::mem::forget(vec); Self { data, len } } + + /// \brief Frees the heap memory used by the opendal_bytes + #[no_mangle] + pub extern "C" fn opendal_bytes_free(ptr: *mut opendal_bytes) { + if !ptr.is_null() { + // free the vector + let _ = unsafe { Vec::from_raw_parts((*ptr).data, (*ptr).len, (*ptr).len) }; + // free the pointer + let _ = unsafe { Box::from_raw(ptr) }; + } + } } #[allow(clippy::from_over_into)] From 3f7bc4d6c693bb3542841386c6f0d46fc9f34e1b Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 02:52:27 +0800 Subject: [PATCH 3/7] Fix test suite --- bindings/c/tests/list.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index 8e5e34c27424..562ab6951642 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -82,16 +82,15 @@ TEST_F(OpendalListTest, ListDirTest) opendal_result_stat s = opendal_operator_stat(this->p, de_path); EXPECT_EQ(s.code, OPENDAL_OK); - opendal_metadata* meta = s.meta; - if (!strcmp(de_path, path.c_str())) { found = true; // the path we found has to be a file, and the length must be coherent - EXPECT_TRUE(opendal_metadata_is_file(meta)); - EXPECT_EQ(opendal_metadata_content_length(meta), nbytes); + EXPECT_TRUE(opendal_metadata_is_file(s.meta)); + EXPECT_EQ(opendal_metadata_content_length(s.meta), nbytes); } + opendal_metadata_free(s.meta); opendal_list_entry_free(entry); entry = opendal_lister_next(lister); } From dafb523a138e93adf00f41d1addccc12e71bf982 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 02:59:05 +0800 Subject: [PATCH 4/7] Fix memory leak in the list test --- bindings/c/include/opendal.h | 4 ++-- bindings/c/src/types.rs | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index af75843e875f..c8cc8ae7846e 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -661,7 +661,7 @@ void opendal_bytes_free(struct opendal_bytes *ptr); /** * \brief Free the heap-allocated metadata used by opendal_metadata */ -void opendal_metadata_free(struct opendal_metadata *self); +void opendal_metadata_free(struct opendal_metadata *ptr); /** * \brief Return the content_length of the metadata @@ -784,7 +784,7 @@ const char *opendal_list_entry_name(const struct opendal_list_entry *self); /** * \brief Frees the heap memory used by the opendal_list_entry */ -void opendal_list_entry_free(const struct opendal_list_entry *p); +void opendal_list_entry_free(struct opendal_list_entry *ptr); #ifdef __cplusplus } // extern "C" diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 3f23edeafd4a..6f172451ebe3 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -16,7 +16,6 @@ // under the License. use std::collections::HashMap; -use std::mem; use std::os::raw::c_char; use ::opendal as od; @@ -154,10 +153,10 @@ impl opendal_metadata { /// \brief Free the heap-allocated metadata used by opendal_metadata #[no_mangle] - pub extern "C" fn opendal_metadata_free(&mut self) { + pub extern "C" fn opendal_metadata_free(ptr: *mut opendal_metadata) { unsafe { - mem::drop(Box::from_raw(self.inner)); - mem::drop(Box::from_raw(self as *mut Self)); + let _ = Box::from_raw((*ptr).inner); + let _ = Box::from_raw(ptr); } } @@ -390,9 +389,10 @@ impl opendal_list_entry { /// \brief Frees the heap memory used by the opendal_list_entry #[no_mangle] - pub unsafe extern "C" fn opendal_list_entry_free(p: *const opendal_list_entry) { - unsafe { - let _ = Box::from_raw(p as *mut opendal_list_entry); + pub unsafe extern "C" fn opendal_list_entry_free(ptr: *mut opendal_list_entry) { + if !ptr.is_null() { + let _ = unsafe { Box::from_raw((*ptr).inner) }; + let _ = unsafe { Box::from_raw(ptr) }; } } } From 99d7ee28f3c2b993180001a0d5021b179a39236c Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 03:04:16 +0800 Subject: [PATCH 5/7] typo --- bindings/c/include/opendal.h | 2 +- bindings/c/src/lib.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index c8cc8ae7846e..4b24bdedfdd5 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -366,7 +366,7 @@ const struct opendal_operator_ptr *opendal_operator_new(const char *scheme, * Write the `bytes` into the `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK * if succeeds, others otherwise. * - * @NOTE It is important to notice that the `bytes` that is passes in will be comsumed by this + * @NOTE It is important to notice that the `bytes` that is passes in will be consumed by this * function. * * @param ptr The opendal_operator_ptr created previously diff --git a/bindings/c/src/lib.rs b/bindings/c/src/lib.rs index 7a01dc25dfbb..6e51302656cb 100644 --- a/bindings/c/src/lib.rs +++ b/bindings/c/src/lib.rs @@ -128,7 +128,7 @@ pub unsafe extern "C" fn opendal_operator_new( /// Write the `bytes` into the `path` blockingly by `op_ptr`, returns the opendal_code OPENDAL_OK /// if succeeds, others otherwise. /// -/// @NOTE It is important to notice that the `bytes` that is passes in will be comsumed by this +/// @NOTE It is important to notice that the `bytes` that is passes in will be consumed by this /// function. /// /// @param ptr The opendal_operator_ptr created previously From a0e1e82ce763e121dfa86d087ec9d7143b9e7490 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 03:23:53 +0800 Subject: [PATCH 6/7] prevent breaking swift and zig ci --- bindings/c/include/opendal.h | 2 +- bindings/c/src/types.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 4b24bdedfdd5..d5df0f498b92 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -191,7 +191,7 @@ typedef struct opendal_bytes { /** * Pointing to the byte array on heap */ - uint8_t *data; + const uint8_t *data; /** * The length of the byte array */ diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 6f172451ebe3..a9b45fcd2711 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -92,7 +92,7 @@ impl From<*mut od::BlockingOperator> for opendal_operator_ptr { #[repr(C)] pub struct opendal_bytes { /// Pointing to the byte array on heap - pub data: *mut u8, + pub data: *const u8, /// The length of the byte array pub len: usize, } @@ -100,7 +100,7 @@ pub struct opendal_bytes { impl opendal_bytes { /// Construct a [`opendal_bytes`] from the Rust [`Vec`] of bytes pub(crate) fn new(vec: Vec) -> Self { - let data = vec.as_ptr() as *mut u8; + let data = vec.as_ptr(); let len = vec.len(); std::mem::forget(vec); Self { data, len } @@ -110,8 +110,9 @@ impl opendal_bytes { #[no_mangle] pub extern "C" fn opendal_bytes_free(ptr: *mut opendal_bytes) { if !ptr.is_null() { + let data_mut = unsafe { (*ptr).data as *mut u8 }; // free the vector - let _ = unsafe { Vec::from_raw_parts((*ptr).data, (*ptr).len, (*ptr).len) }; + let _ = unsafe { Vec::from_raw_parts(data_mut, (*ptr).len, (*ptr).len) }; // free the pointer let _ = unsafe { Box::from_raw(ptr) }; } From 5945d0f0e12aeac6acc38f1de0d2b7ed78b72f93 Mon Sep 17 00:00:00 2001 From: Ji-Xinyou Date: Thu, 20 Jul 2023 03:53:53 +0800 Subject: [PATCH 7/7] fix the invalid read --- bindings/c/include/opendal.h | 16 +++++++++++----- bindings/c/src/types.rs | 25 ++++++++++++++++++------- bindings/c/tests/list.cpp | 4 +++- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index d5df0f498b92..d833563242a5 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -768,18 +768,24 @@ struct opendal_list_entry *opendal_lister_next(const struct opendal_blocking_lis void opendal_lister_free(const struct opendal_blocking_lister *p); /** - * Path of entry. Path is relative to operator's root. - * Only valid in current operator. + * \brief Path of entry. + * + * Path is relative to operator's root. Only valid in current operator. + * + * @NOTE To free the string, you can directly call free() */ -const char *opendal_list_entry_path(const struct opendal_list_entry *self); +char *opendal_list_entry_path(const struct opendal_list_entry *self); /** - * Name of entry. Name is the last segment of path. + * \brief Name of entry. * + * Name is the last segment of path. * If this entry is a dir, `Name` MUST endswith `/` * Otherwise, `Name` MUST NOT endswith `/`. + * + * @NOTE To free the string, you can directly call free() */ -const char *opendal_list_entry_name(const struct opendal_list_entry *self); +char *opendal_list_entry_name(const struct opendal_list_entry *self); /** * \brief Frees the heap memory used by the opendal_list_entry diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index a9b45fcd2711..77d6a1c225c3 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -16,6 +16,7 @@ // under the License. use std::collections::HashMap; +use std::ffi::CString; use std::os::raw::c_char; use ::opendal as od; @@ -372,20 +373,30 @@ impl opendal_list_entry { } } - /// Path of entry. Path is relative to operator's root. - /// Only valid in current operator. + /// \brief Path of entry. + /// + /// Path is relative to operator's root. Only valid in current operator. + /// + /// @NOTE To free the string, you can directly call free() #[no_mangle] - pub unsafe extern "C" fn opendal_list_entry_path(&self) -> *const c_char { - (*self.inner).path().as_ptr() as *const c_char + pub unsafe extern "C" fn opendal_list_entry_path(&self) -> *mut c_char { + let s = (*self.inner).path(); + let c_str = CString::new(s).unwrap(); + c_str.into_raw() } - /// Name of entry. Name is the last segment of path. + /// \brief Name of entry. /// + /// Name is the last segment of path. /// If this entry is a dir, `Name` MUST endswith `/` /// Otherwise, `Name` MUST NOT endswith `/`. + /// + /// @NOTE To free the string, you can directly call free() #[no_mangle] - pub unsafe extern "C" fn opendal_list_entry_name(&self) -> *const c_char { - (*self.inner).name().as_ptr() as *const c_char + pub unsafe extern "C" fn opendal_list_entry_name(&self) -> *mut c_char { + let s = (*self.inner).name(); + let c_str = CString::new(s).unwrap(); + c_str.into_raw() } /// \brief Frees the heap memory used by the opendal_list_entry diff --git a/bindings/c/tests/list.cpp b/bindings/c/tests/list.cpp index 562ab6951642..b6b44dd44d02 100644 --- a/bindings/c/tests/list.cpp +++ b/bindings/c/tests/list.cpp @@ -76,7 +76,7 @@ TEST_F(OpendalListTest, ListDirTest) opendal_list_entry* entry = opendal_lister_next(lister); while (entry) { - const char* de_path = opendal_list_entry_path(entry); + char* de_path = opendal_list_entry_path(entry); // stat must succeed opendal_result_stat s = opendal_operator_stat(this->p, de_path); @@ -90,8 +90,10 @@ TEST_F(OpendalListTest, ListDirTest) EXPECT_EQ(opendal_metadata_content_length(s.meta), nbytes); } + free(de_path); opendal_metadata_free(s.meta); opendal_list_entry_free(entry); + entry = opendal_lister_next(lister); }