From ab6066a0f4b37acb42639165d0465b2b09fc7143 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Sat, 20 Jun 2026 17:10:21 +0200 Subject: [PATCH] Fix C binding empty byte buffers --- bindings/c/include/opendal.h | 12 +++--- bindings/c/src/operator.rs | 12 +++++- bindings/c/src/types.rs | 33 ++++++++++------ bindings/c/src/writer.rs | 9 +++++ bindings/c/tests/bdd.cpp | 76 ++++++++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 18 deletions(-) diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 266677444970..3ce6e916aa1a 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -92,9 +92,11 @@ typedef struct opendal_presigned_request_inner opendal_presigned_request_inner; /** * \brief opendal_bytes carries raw-bytes with its length * - * The opendal_bytes type is a C-compatible substitute for Vec type - * in Rust, it has to be manually freed. You have to call opendal_bytes_free() - * to free the heap memory to avoid memory leak. + * The opendal_bytes type is a C-compatible substitute for Vec type in Rust. + * For buffers returned by OpenDAL C APIs, call opendal_bytes_free() to free + * the heap memory and avoid memory leaks. For caller-owned input buffers + * passed to OpenDAL C APIs, the caller keeps ownership and must not call + * opendal_bytes_free() on them. * * @see opendal_bytes_free */ @@ -1420,8 +1422,8 @@ struct opendal_result_operator_new opendal_operator_new_with_layers(const char * * It is **safe** under the cases below * * The memory pointed to by `path` must contain a valid nul terminator at the end of * the string. - * * The `bytes` provided has valid byte in the `data` field and the `len` field is set - * correctly. + * * If `bytes.len` is greater than 0, `bytes.data` must point to at least + * `bytes.len` valid bytes. If `bytes.len` is 0, `bytes.data` may be NULL. * * # Panic * diff --git a/bindings/c/src/operator.rs b/bindings/c/src/operator.rs index cb610640242f..02850bfa8fd1 100644 --- a/bindings/c/src/operator.rs +++ b/bindings/c/src/operator.rs @@ -259,8 +259,8 @@ pub unsafe extern "C" fn opendal_operator_new_with_layers( /// It is **safe** under the cases below /// * The memory pointed to by `path` must contain a valid nul terminator at the end of /// the string. -/// * The `bytes` provided has valid byte in the `data` field and the `len` field is set -/// correctly. +/// * If `bytes.len` is greater than 0, `bytes.data` must point to at least +/// `bytes.len` valid bytes. If `bytes.len` is 0, `bytes.data` may be NULL. /// /// # Panic /// @@ -275,6 +275,10 @@ pub unsafe extern "C" fn opendal_operator_write( let path = std::ffi::CStr::from_ptr(path) .to_str() .expect("malformed path"); + let bytes = match bytes.to_buffer() { + Ok(bytes) => bytes, + Err(e) => return opendal_error::new(e), + }; match op.deref().write(path, bytes) { Ok(_) => std::ptr::null_mut(), Err(e) => opendal_error::new(e), @@ -298,6 +302,10 @@ pub unsafe extern "C" fn opendal_operator_write_with( } else { (&*opts).into() }; + let bytes = match bytes.to_buffer() { + Ok(bytes) => bytes, + Err(e) => return opendal_error::new(e), + }; match op.deref().write_options(path, bytes, opts) { Ok(_) => std::ptr::null_mut(), Err(e) => opendal_error::new(e), diff --git a/bindings/c/src/types.rs b/bindings/c/src/types.rs index 85810f276827..af5cd98464f4 100644 --- a/bindings/c/src/types.rs +++ b/bindings/c/src/types.rs @@ -21,8 +21,8 @@ use std::os::raw::c_char; use opendal::options; use opendal::raw::Timestamp; -use opendal::Buffer; use opendal::BytesRange; +use opendal::{Buffer, Error, ErrorKind}; /// \brief Frees a heap-allocated string returned by OpenDAL C APIs. /// @@ -36,9 +36,11 @@ pub unsafe extern "C" fn opendal_string_free(ptr: *mut c_char) { /// \brief opendal_bytes carries raw-bytes with its length /// -/// The opendal_bytes type is a C-compatible substitute for Vec type -/// in Rust, it has to be manually freed. You have to call opendal_bytes_free() -/// to free the heap memory to avoid memory leak. +/// The opendal_bytes type is a C-compatible substitute for Vec type in Rust. +/// For buffers returned by OpenDAL C APIs, call opendal_bytes_free() to free +/// the heap memory and avoid memory leaks. For caller-owned input buffers +/// passed to OpenDAL C APIs, the caller keeps ownership and must not call +/// opendal_bytes_free() on them. /// /// @see opendal_bytes_free #[repr(C)] @@ -85,6 +87,22 @@ impl opendal_bytes { } } } + + pub(crate) fn to_buffer(&self) -> opendal::Result { + if self.len == 0 { + return Ok(Buffer::new()); + } + + if self.data.is_null() { + return Err(Error::new( + ErrorKind::Unexpected, + "non-empty opendal_bytes has null data", + )); + } + + let slice = unsafe { std::slice::from_raw_parts(self.data, self.len) }; + Ok(Buffer::from(bytes::Bytes::copy_from_slice(slice))) + } } /// \brief The options for the list operation. @@ -1204,13 +1222,6 @@ impl Drop for opendal_bytes { } } -impl From<&opendal_bytes> for Buffer { - fn from(v: &opendal_bytes) -> Self { - let slice = unsafe { std::slice::from_raw_parts(v.data, v.len) }; - Buffer::from(bytes::Bytes::copy_from_slice(slice)) - } -} - /// \brief The configuration for the initialization of opendal_operator. /// /// \note This is also a heap-allocated struct, please free it after you use it diff --git a/bindings/c/src/writer.rs b/bindings/c/src/writer.rs index 2b0eafa05a8c..b5fbffd2b99d 100644 --- a/bindings/c/src/writer.rs +++ b/bindings/c/src/writer.rs @@ -52,6 +52,15 @@ impl opendal_writer { bytes: &opendal_bytes, ) -> opendal_result_writer_write { let size = bytes.len; + let bytes = match bytes.to_buffer() { + Ok(bytes) => bytes, + Err(e) => { + return opendal_result_writer_write { + size: 0, + error: opendal_error::new(e), + } + } + }; match self.deref_mut().write(bytes) { Ok(()) => opendal_result_writer_write { size, diff --git a/bindings/c/tests/bdd.cpp b/bindings/c/tests/bdd.cpp index 946df0c86630..e678a7c102e9 100644 --- a/bindings/c/tests/bdd.cpp +++ b/bindings/c/tests/bdd.cpp @@ -138,3 +138,79 @@ TEST_F(OpendalBddTest, FeatureTest) error = opendal_operator_delete(this->p, "tmpdir/"); EXPECT_EQ(error, nullptr); } + +TEST_F(OpendalBddTest, WriteEmptyNullBytes) +{ + const opendal_bytes empty = { + .data = nullptr, + .len = 0, + .capacity = 0, + }; + + opendal_error* error = opendal_operator_write(this->p, "empty", &empty); + EXPECT_EQ(error, nullptr); + + opendal_result_read read = opendal_operator_read(this->p, "empty"); + EXPECT_EQ(read.error, nullptr); + EXPECT_EQ(read.data.len, 0); + opendal_bytes_free(&read.data); + + error = opendal_operator_write_with(this->p, "empty-with-options", &empty, nullptr); + EXPECT_EQ(error, nullptr); + + read = opendal_operator_read(this->p, "empty-with-options"); + EXPECT_EQ(read.error, nullptr); + EXPECT_EQ(read.data.len, 0); + opendal_bytes_free(&read.data); + + opendal_result_operator_writer writer = opendal_operator_writer(this->p, "empty-writer"); + EXPECT_EQ(writer.error, nullptr); + ASSERT_NE(writer.writer, nullptr); + + opendal_result_writer_write write = opendal_writer_write(writer.writer, &empty); + EXPECT_EQ(write.error, nullptr); + EXPECT_EQ(write.size, 0); + + error = opendal_writer_close(writer.writer); + EXPECT_EQ(error, nullptr); + opendal_writer_free(writer.writer); + + read = opendal_operator_read(this->p, "empty-writer"); + EXPECT_EQ(read.error, nullptr); + EXPECT_EQ(read.data.len, 0); + opendal_bytes_free(&read.data); + + EXPECT_EQ(opendal_operator_delete(this->p, "empty"), nullptr); + EXPECT_EQ(opendal_operator_delete(this->p, "empty-with-options"), nullptr); + EXPECT_EQ(opendal_operator_delete(this->p, "empty-writer"), nullptr); +} + +TEST_F(OpendalBddTest, RejectNonEmptyNullBytes) +{ + const opendal_bytes invalid = { + .data = nullptr, + .len = 1, + .capacity = 0, + }; + + opendal_error* error = opendal_operator_write(this->p, "invalid", &invalid); + ASSERT_NE(error, nullptr); + EXPECT_EQ(error->code, OPENDAL_UNEXPECTED); + opendal_error_free(error); + + error = opendal_operator_write_with(this->p, "invalid-with-options", &invalid, nullptr); + ASSERT_NE(error, nullptr); + EXPECT_EQ(error->code, OPENDAL_UNEXPECTED); + opendal_error_free(error); + + opendal_result_operator_writer writer = opendal_operator_writer(this->p, "invalid-writer"); + EXPECT_EQ(writer.error, nullptr); + ASSERT_NE(writer.writer, nullptr); + + opendal_result_writer_write write = opendal_writer_write(writer.writer, &invalid); + EXPECT_EQ(write.size, 0); + ASSERT_NE(write.error, nullptr); + EXPECT_EQ(write.error->code, OPENDAL_UNEXPECTED); + opendal_error_free(write.error); + opendal_writer_free(writer.writer); +}