You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "fallintoplace (via GitHub)" <gi...@apache.org> on 2026/06/20 15:11:47 UTC
[PR] Fix C binding empty byte buffers [opendal]
fallintoplace opened a new pull request, #7807:
URL: https://github.com/apache/opendal/pull/7807
## Summary
- Handle empty C `opendal_bytes` without constructing a Rust slice from a null pointer.
- Return an OpenDAL error when non-empty input buffers have null data.
- Clarify C byte buffer ownership docs and add focused coverage for operator and writer writes.
## Why
C callers can reasonably pass `{ .data = NULL, .len = 0, .capacity = 0 }` for an empty buffer. Rust still requires `slice::from_raw_parts` to receive a non-null, aligned pointer even when the length is zero, so the old conversion could invoke undefined behavior.
## Tests
- `cargo clippy -- -D warnings`
- `cmake -S . -B build -DTEST_ENABLE_ASAN=ON -DFEATURES=opendal/services-memory && cmake --build build -j4 && ./build/tests`
- `cargo build --features="opendal/services-memory" && make -C tests clean all && OPENDAL_TEST=memory make -C tests behavior_test`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix C binding empty byte buffers [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan merged PR #7807:
URL: https://github.com/apache/opendal/pull/7807
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix C binding empty byte buffers [opendal]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace commented on code in PR #7807:
URL: https://github.com/apache/opendal/pull/7807#discussion_r3487624581
##########
bindings/c/src/types.rs:
##########
@@ -85,6 +87,22 @@ impl opendal_bytes {
}
}
}
+
+ pub(crate) fn to_buffer(&self) -> opendal::Result<Buffer> {
+ if self.len == 0 {
Review Comment:
Sounds good to me. I initially allowed zero-length non-null buffers for C compatibility, but I’m happy to make the contract stricter and require NULL for empty input as well. I’ll update the checks, docs, and tests accordingly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix C binding empty byte buffers [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7807:
URL: https://github.com/apache/opendal/pull/7807#discussion_r3486205417
##########
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.
Review Comment:
👍
##########
bindings/c/src/types.rs:
##########
@@ -85,6 +87,22 @@ impl opendal_bytes {
}
}
}
+
+ pub(crate) fn to_buffer(&self) -> opendal::Result<Buffer> {
+ if self.len == 0 {
Review Comment:
I am also thinking we could make checks somewhat more strict - to disallow users passing in len == 0 and data != nullptr.
##########
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.
Review Comment:
Do we really want to allow non-null pointers when len is 0? e.g. malloc(0)'s non-null result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix C binding empty byte buffers [opendal]
Posted by "fallintoplace (via GitHub)" <gi...@apache.org>.
fallintoplace commented on code in PR #7807:
URL: https://github.com/apache/opendal/pull/7807#discussion_r3487694843
##########
bindings/c/tests/bdd.cpp:
##########
@@ -138,3 +138,110 @@ 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");
Review Comment:
It’s covering opendal_operator_write_with specifically. This patch adds the same opendal_bytes validation to both opendal_operator_write and opendal_operator_write_with, so I wanted one happy-path check for each public write entry point plus the writer path. Since opts is NULL here, it isn’t testing options behavior, only that empty-null buffers are accepted through the write_with API as well
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix C binding empty byte buffers [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7807:
URL: https://github.com/apache/opendal/pull/7807#discussion_r3487863765
##########
bindings/c/tests/bdd.cpp:
##########
@@ -138,3 +138,110 @@ 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");
Review Comment:
I understand.
nitpick: we can have a comment here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix C binding empty byte buffers [opendal]
Posted by "erickguan (via GitHub)" <gi...@apache.org>.
erickguan commented on code in PR #7807:
URL: https://github.com/apache/opendal/pull/7807#discussion_r3487692353
##########
bindings/c/tests/bdd.cpp:
##########
@@ -138,3 +138,110 @@ 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");
Review Comment:
What is this test for?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org