GH-49740: [C++][Python] Fix casts to view types leaving null variadic buffers by fenfeng9 · Pull Request #50166 · apache/arrow

fenfeng9 · 2026-06-12T16:07:50Z

Rationale for this change

Casting to binary_view or string_view could leave a null variadic buffer slot when all values were inline. This could happen for casts from binary, large_binary, string, large_string, and fixed_size_binary.

The C Data Interface exporter reads every variadic buffer to get its size. Because of that, exporting such an array could crash, for example through PyArrow _export_to_c.

Validation also passed for these arrays. For all-inline view arrays, validation never needed to read an out-of-line data buffer.

What changes are included in this PR?

This PR fixes the cast kernels so all-inline view arrays do not keep a null variadic buffer slot.

It also makes validation reject null variadic buffer slots, and makes C Data export return an error instead of crashing.

C++ and Python regression tests cover the cast, validation, and export paths.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

This PR contains a "Critical Fix" Exporting an all-inline view array through the C Data Interface could crash the process while using only public APIs.

GitHub Issue: [C++][Python] _export_to_c segmentation fault for binary_view array #49740

pitrou

Thanks a lot @fenfeng9 . Two minor suggestions, but otherwise this PR looks good to me.

pitrou · 2026-06-15T08:31:05Z

+ util::ToInlineBinaryView("hello"),
+ util::ToInlineBinaryView("world"),
+ }),
+ Raises(StatusCode::Invalid));


Can we test the error message somehow?

Sure, I'll update the tests later.

pitrou · 2026-06-15T08:31:28Z

+ nullptr}));
+
+ struct ArrowArray c_export;
+ ASSERT_RAISES(Invalid, ExportArray(*arr, &c_export));


Can we test the error message too?

pitrou · 2026-06-15T09:32:35Z

Hmm, I think we should try to produce some additional columns without any variadic buffers in the integration tests. Are you comfortable doing that @fenfeng9 ? Otherwise I can do it.

fenfeng9 · 2026-06-15T10:55:45Z

Hmm, I think we should try to produce some additional columns without any variadic buffers in the integration tests. Are you comfortable doing that @fenfeng9 ? Otherwise I can do it.

Yes, I'm willing to do that. Should I open a new issue for it?

pitrou · 2026-06-15T11:50:18Z

Yes, I'm willing to do that. Should I open a new issue for it?

Or you can do it in this PR, because it would help validate that all implementations are in agreement.

fenfeng9 · 2026-06-15T11:55:02Z

Yes, I'm willing to do that. Should I open a new issue for it?
Or you can do it in this PR, because it would help validate that all implementations are in agreement.

Sounds good, I'll update it in this PR later. Thanks!

pitrou · 2026-06-15T14:33:22Z

We may want to wait for the outcome of the discussion in #50172, actually

fenfeng9 · 2026-06-15T14:36:55Z

We may want to wait for the outcome of the discussion in #50172, actually

It's OK, I'll update the code after the discussion is concluded.

pitrou · 2026-06-25T11:13:20Z

Ok, in #50172 the consensus is that null variadic buffers should be allowed, at least when importing from the C Data Interface.

So we can either:

detect null variadic buffers when importing from the C Data Interface, and normalize them as empty non-null buffers
or allow null variadic buffers everywhere in Arrow C++ (which may be more tedious and more fragile, as we would have to test for this condition explicitly)

fenfeng9 · 2026-06-25T12:09:53Z

Ok, in #50172 the consensus is that null variadic buffers should be allowed, at least when importing from the C Data Interface.
So we can either:
detect null variadic buffers when importing from the C Data Interface, and normalize them as empty non-null buffers
or allow null variadic buffers everywhere in Arrow C++ (which may be more tedious and more fragile, as we would have to test for this condition explicitly)

Agree. Then we just need to check for nullptr variadic buffers during Export and set their sizes to 0 instead of calling buf->size().

arrow/cpp/src/arrow/c/bridge.cc

Lines 606 to 614 in 445851a

if (need_variadic_buffer_sizes) { 
auto variadic_buffers = std::span(data->buffers).subspan(2); 
 export_.variadic_buffer_sizes_.resize(variadic_buffers.size()); 
size_t i = 0; 
for (constauto& buf : variadic_buffers) { 
 export_.variadic_buffer_sizes_[i++] = buf->size(); 
 } 
 export_.buffers_.back() = export_.variadic_buffer_sizes_.data(); 
 } 

fenfeng9 · 2026-06-25T12:17:07Z

I'll update this PR when that documentation PR is merged.

fenfeng9 · 2026-06-25T12:35:37Z

I'll update this PR when that documentation PR is merged.

Sorry, it seems we only agreed to allow nullptr variadic buffers for the C Data Interface #50172 , but not for Arrow C++ internally.

pitrou · 2026-06-25T12:38:01Z

Well, allowing null variadic buffers inside Arrow C++ might lead to hidden issues if we dereference them. Perhaps it's better to normalize null variadic buffers when importing them over the C Data Interface.

fenfeng9 · 2026-06-25T12:54:52Z

Well, allowing null variadic buffers inside Arrow C++ might lead to hidden issues if we dereference them. Perhaps it's better to normalize null variadic buffers when importing them over the C Data Interface.

It looks like the import path already handles null buffers — when the data pointer is null and size is 0, it gets normalized to zero_size_buffer_.

Status ImportBinaryView(const BinaryViewType&):

arrow/cpp/src/arrow/c/bridge.cc

Lines 1767 to 1774 in 445851a

// The last C data buffer stores buffer sizes, and shouldn't be imported
auto* buffer_sizes = 
static_cast<constint64_t*>(c_struct_->buffers[c_struct_->n_buffers - 1]); 
for (int32_t buffer_id = 2; buffer_id < c_struct_->n_buffers - 1; ++buffer_id) { 
RETURN_NOT_OK(ImportBuffer(buffer_id, buffer_sizes[buffer_id - 2])); 
 } 
 data_->buffers.pop_back(); 

ImportBuffer:

arrow/cpp/src/arrow/c/bridge.cc

Lines 1912 to 1935 in 445851a

 Status ImportBuffer(int32_t buffer_id, int64_t buffer_size, 
bool is_null_bitmap = false) { 
 std::shared_ptr<Buffer>* out = &data_->buffers[buffer_id]; 
auto data = reinterpret_cast<constuint8_t*>(c_struct_->buffers[buffer_id]); 
if (data != nullptr) { 
if (memory_mgr_) { 
 *out = std::make_shared<ImportedBuffer>(data, buffer_size, memory_mgr_, 
 device_type_, import_); 
 } else { 
 *out = std::make_shared<ImportedBuffer>(data, buffer_size, import_); 
 } 
 } elseif (is_null_bitmap) { 
 out->reset(); 
 } else { 
// Ensure that imported buffers are never null (except for the null bitmap)
if (buffer_size != 0) { 
returnStatus::Invalid( 
"ArrowArrayStruct contains null data pointer "
"for a buffer with non-zero computed size"); 
 } 
 *out = zero_size_buffer_; 
 } 
returnStatus::OK(); 
 } 

arrow/cpp/src/arrow/c/bridge.cc

Lines 1944 to 1947 in 445851a

// For imported null buffer pointers
 std::shared_ptr<Buffer> zero_size_buffer_; 

pitrou · 2026-06-25T12:58:21Z

Ok, so this implies we probably don't want to produce null variadic buffers in other Arrow functionality (such as casting to binary-view).

pitrou · 2026-06-25T13:01:13Z

Also, we need to add tests for null variadic buffers in arrow/c/bridge_test.cc.

fenfeng9 · 2026-06-25T13:12:09Z

Ok, so this implies we probably don't want to produce null variadic buffers in other Arrow functionality (such as casting to binary-view).

Okay, so we keep the other changes? The validate still reject null variadic buffers?

And do we also need to update other documentation to state that we do not allow generating null variadic buffers?

fenfeng9 · 2026-06-25T13:15:16Z

Also, we need to add tests for null variadic buffers in arrow/c/bridge_test.cc.

I will update pr later.

pitrou · 2026-06-25T13:19:07Z

Okay, so we keep the other changes? The validate still reject null variadic buffers?

Yes, I think validation should still reject them.

And do we also need to update other documentation to state that we do not allow generating null variadic buffers?

I don't think we have a piece of documentation that covers this, do we?

fenfeng9 · 2026-06-25T13:26:51Z

I don't think we have a piece of documentation that covers this, do we?

No, there isn't.

Thanks for the patient reply. Now I understand: we don't generate null variadic buffers inside Arrow C++.

fenfeng9 · 2026-06-26T18:21:08Z

CI failures seems unrelated to this PR:

Python docs: Doxygen fails
macOS S3 test: 79 - arrow-s3fs-test (Timeout) arrow-tests filesystem unittest
GLib/Ruby: Conda/Homebrew ABI mismatch causing Bus error

fenfeng9 · 2026-06-26T18:28:34Z

Hmm, I think we should try to produce some additional columns without any variadic buffers in the integration tests. Are you comfortable doing that @fenfeng9 ? Otherwise I can do it.

I added bv_inline and sv_inline to the binary_view integration case; both only generate inline values.

pitrou

Thanks a lot @fenfeng9 . Some comments below. Also, can you rebase/merge from main?

pitrou · 2026-06-30T08:11:58Z

 size_t i = 0;
 for (const auto& buf : variadic_buffers) {
- export_.variadic_buffer_sizes_[i++] = buf->size();
+ // The C Data Interface allows null variadic buffer pointers with size 0.


But they are invalid in Arrow C++, so we can revert this change, or explicitly raise an error if we want to avoid a segfault.
(sorry, I think I said something different about this earlier; but I don't think it makes sense to explicitly allow exporting data that is invalid)

Agree. We should raise an error on export when the data is invalid.

Done, export now rejects null variadic buffers with Invalid.

pitrou · 2026-06-30T08:16:04Z

+ # Generate only inline values, leaving no variadic data buffers.
+
+ def _random_sizes(self, size):
+ return np.arange(size, dtype=np.int32) % (BINARY_VIEW_INLINE_SIZE + 1)


Why not use https://numpy.org/doc/stable/reference/random/generated/numpy.random.random_integers.html ?

update to np.random.randint

pitrou · 2026-06-30T08:16:22Z

+ # Generate only inline values, leaving no variadic data buffers.
+
+ def _random_sizes(self, size):
+ return np.arange(size, dtype=np.int32) % (BINARY_VIEW_INLINE_SIZE + 1)


Why not use https://numpy.org/doc/stable/reference/random/generated/numpy.random.random_integers.html ?

fenfeng9 · 2026-06-30T08:27:40Z

Thanks a lot @fenfeng9 . Some comments below. Also, can you rebase/merge from main?

Sure, I will update pr late.

fenfeng9 · 2026-06-30T17:55:17Z

One test failed because:

The following tests FAILED:
79 - arrow-s3fs-test (Timeout) arrow-tests filesystem unittest

pitrou · 2026-07-01T08:00:56Z

One test failed because:

Yes, feel free to disregard as it's unrelated :)

pitrou · 2026-07-01T08:07:46Z

Thanks a lot @fenfeng9 . Also, I've checked that the newly generated integration data doesn't contain non-inline views.

@Alex-PLACET This adds some integration data, you might want to later check the Sparrow integration tests still pass with it.

conbench-apache-arrow · 2026-07-02T10:25:47Z

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 4bedf49.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 13 possible false positives for unstable benchmarks that are known to sometimes produce them.

fenfeng9 requested review from AlenkaF, pitrou, raulcd and rok as code owners June 12, 2026 16:07

github-actionsBot added Component: C++ Component: Python awaiting review Awaiting review labels Jun 12, 2026

pitrou reviewed Jun 15, 2026
View reviewed changes

github-actionsBot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels Jun 15, 2026

fenfeng9 force-pushed the fix-gh-49740-view-export-to-c branch from 9c8b268 to d889932Compare June 26, 2026 17:40

fenfeng9 requested a review from pitrou June 26, 2026 18:28

pitrou mentioned this pull request Jun 30, 2026
[Docs] Clarify whether the data buffers of Binary|String View can contain null buffers #50172
Closed

pitrou reviewed Jun 30, 2026
View reviewed changes

fenfeng9 added 10 commits July 1, 2026 00:59

Fix all-inline binary view casts for C export
b8f9113

Validate non-null variadic buffers
034f252

Validate variadic buffers before C export
11dd55b

Test C export guard for string_view
da4186a

update cast tests for inline view buffers
ca9536d

test null variadic buffer error messages
5b9fc57

add all-inline view integration data
b4f0989

handle null variadic buffers in C bridge
7e4d069

Use random inline view sizes
fdc35be

reject null variadic buffers on export
474dc3e

fenfeng9 force-pushed the fix-gh-49740-view-export-to-c branch from d889932 to 474dc3eCompare June 30, 2026 17:00

fenfeng9 requested a review from pitrou June 30, 2026 17:55

pitrou approved these changes Jul 1, 2026
View reviewed changes

pitrou merged commit 4bedf49 into apache:mainJul 1, 2026
61 of 62 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Jul 1, 2026

pitrou mentioned this pull request Jul 1, 2026
[C++][Python] _export_to_c segmentation fault for binary_view array #49740
Closed

fangchenli mentioned this pull request Jul 11, 2026
GH-50478: [C++][Compute] Support string_view/binary_view in scalar string predicate kernels #50479
Open

Uh oh!

Conversation

fenfeng9 commented Jun 12, 2026• edited by github-actions BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pitrou commented Jun 15, 2026

Uh oh!

fenfeng9 commented Jun 15, 2026

Uh oh!

pitrou commented Jun 15, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fenfeng9 commented Jun 15, 2026

Uh oh!

pitrou commented Jun 15, 2026

Uh oh!

fenfeng9 commented Jun 15, 2026

Uh oh!

pitrou commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

pitrou commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

pitrou commented Jun 25, 2026

Uh oh!

pitrou commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

pitrou commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 25, 2026

Uh oh!

fenfeng9 commented Jun 26, 2026

Uh oh!

fenfeng9 commented Jun 26, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fenfeng9 commented Jun 30, 2026

Uh oh!

fenfeng9 commented Jun 30, 2026

Uh oh!

pitrou commented Jul 1, 2026

Uh oh!

fenfeng9 commented Jun 12, 2026•
edited by github-actions Bot
Loading

pitrou commented Jun 15, 2026•
edited
Loading