From d276718151ddc08c36ab96e4131bbcc44107cae9 Mon Sep 17 00:00:00 2001 From: Orson Peters Date: Mon, 22 Sep 2025 21:40:50 +0200 Subject: [PATCH 1/2] fix(arrow/cdata): Avoid calling unsafe.Slice on zero-length pointers (#513) Slices from FFI may have an arbitrary pointer when the length is zero, but this is not allowed in Go, where the pointer must always be valid. I believe this fixes https://github.com/apache/arrow-go/issues/28. I changed the instances of `unsafe.Slice` in `arrow/cdata` I could find to be robust when used with length zero. No, I don't have a Go setup at all. No. --------- Co-authored-by: Matt Topol Signed-off-by: jiaqizho --- go/arrow/cdata/cdata.go | 20 +++++++++-------- go/arrow/cdata/cdata_fulltest.c | 30 +++++++++++++++++++++++++- go/arrow/cdata/cdata_test.go | 15 +++++++++++++ go/arrow/cdata/cdata_test_framework.go | 7 ++++++ 4 files changed, 62 insertions(+), 10 deletions(-) diff --git a/go/arrow/cdata/cdata.go b/go/arrow/cdata/cdata.go index 2853f09b76a4..d2430189f1f1 100644 --- a/go/arrow/cdata/cdata.go +++ b/go/arrow/cdata/cdata.go @@ -359,10 +359,10 @@ func (imp *cimporter) importChild(parent *cimporter, src *CArrowArray) error { // import any child arrays for lists, structs, and so on. func (imp *cimporter) doImportChildren() error { - children := unsafe.Slice(imp.arr.children, imp.arr.n_children) - - if len(children) > 0 { - imp.children = make([]cimporter, len(children)) + var children []*CArrowArray + if imp.arr.n_children > 0 { + children = unsafe.Slice(imp.arr.children, imp.arr.n_children) + imp.children = make([]cimporter, imp.arr.n_children) } // handle the cases @@ -701,10 +701,12 @@ func (imp *cimporter) importBinaryViewLike() (err error) { return } - dataBufferSizes := unsafe.Slice((*int64)(unsafe.Pointer(imp.cbuffers[len(buffers)])), len(buffers)-2) - for i, size := range dataBufferSizes { - if buffers[i+2], err = imp.importVariableValuesBuffer(i+2, 1, size); err != nil { - return + if len(buffers) > 2 { + dataBufferSizes := unsafe.Slice((*int64)(unsafe.Pointer(imp.cbuffers[len(buffers)])), len(buffers)-2) + for i, size := range dataBufferSizes { + if buffers[i+2], err = imp.importVariableValuesBuffer(i+2, 1, size); err != nil { + return + } } } @@ -856,7 +858,7 @@ func (imp *cimporter) checkNumBuffers(n int64) error { func (imp *cimporter) importBuffer(bufferID int, sz int64) (*memory.Buffer, error) { // this is not a copy, we're just having a slice which points at the data // it's still owned by the C.ArrowArray object and its backing C++ object. - if imp.cbuffers[bufferID] == nil { + if imp.cbuffers[bufferID] == nil || sz == 0 { if sz != 0 { return nil, errors.New("invalid buffer") } diff --git a/go/arrow/cdata/cdata_fulltest.c b/go/arrow/cdata/cdata_fulltest.c index 4291cfff865b..63d55a4c45e6 100644 --- a/go/arrow/cdata/cdata_fulltest.c +++ b/go/arrow/cdata/cdata_fulltest.c @@ -335,8 +335,13 @@ void export_int32_array(const int32_t*, int64_t, struct ArrowArray*); static void release_str_array(struct ArrowArray* array) { assert(array->n_buffers == 3); + if (array->buffers[0] != NULL) { + free((void*) array->buffers[0]); + } free((void*) array->buffers[1]); - free((void*) array->buffers[2]); + if (array->buffers[2] != NULL && array->buffers[2] != (void*)0x1) { + free((void*) array->buffers[2]); + } free(array->buffers); array->release = NULL; } @@ -361,6 +366,29 @@ void export_str_array(const char* data, const int32_t* offsets, int64_t nitems, out->buffers[2] = data; } +void export_str_array_with_nulls(int64_t nitems, struct ArrowArray* out) { + *out = (struct ArrowArray) { + .length = nitems, + .offset = 0, + .null_count = nitems, + .n_buffers = 3, + .n_children = 0, + .children = NULL, + .dictionary = NULL, + // bookkeeping + .release = &release_str_array + }; + + out->buffers = (const void**)malloc(sizeof(void*) * out->n_buffers); + assert(out->buffers != NULL); + int64_t bitmap_nbytes = (nitems + 7) / 8; + out->buffers[0] = malloc(bitmap_nbytes); + memset((void*)out->buffers[0], 0, bitmap_nbytes); + out->buffers[1] = malloc((nitems + 1) * sizeof(int32_t)); + memset((void*)out->buffers[1], 0, (nitems + 1) * sizeof(int32_t)); + out->buffers[2] = (void*)0x1; +} + static int next_record(struct ArrowArrayStream* st, struct ArrowArray* out) { struct streamcounter* cnter = (struct streamcounter*)(st->private_data); if (cnter->n == cnter->max) { diff --git a/go/arrow/cdata/cdata_test.go b/go/arrow/cdata/cdata_test.go index 6aad01103735..82e12bc3a96b 100644 --- a/go/arrow/cdata/cdata_test.go +++ b/go/arrow/cdata/cdata_test.go @@ -42,6 +42,7 @@ import ( "github.com/apache/arrow/go/v17/arrow/memory" "github.com/apache/arrow/go/v17/arrow/memory/mallocator" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSchemaExport(t *testing.T) { @@ -710,6 +711,20 @@ func TestNestedArrays(t *testing.T) { } } +func TestStrArrayAllNulls(t *testing.T) { + arr := exportStrArrayWithNulls(1000) + carr, err := ImportCArrayWithType(&arr, arrow.BinaryTypes.String) + require.NoError(t, err) + defer carr.Release() + + buffer := carr.Data().Buffers()[2] + assert.NotNil(t, buffer) + bs := buffer.Bytes() + assert.Equal(t, 1000, carr.Len()) + assert.Equal(t, 1000, carr.NullN()) + assert.Empty(t, bs) +} + func TestRecordBatch(t *testing.T) { mem := mallocator.NewMallocator() defer mem.AssertSize(t, 0) diff --git a/go/arrow/cdata/cdata_test_framework.go b/go/arrow/cdata/cdata_test_framework.go index caa1208a20ae..43b982e8dc7a 100644 --- a/go/arrow/cdata/cdata_test_framework.go +++ b/go/arrow/cdata/cdata_test_framework.go @@ -46,6 +46,7 @@ package cdata // } // void export_int32_type(struct ArrowSchema* schema); // void export_int32_array(const int32_t*, int64_t, struct ArrowArray*); +// void export_str_array_with_nulls(int64_t nitems, struct ArrowArray* out); // int test1_is_released(); // void test_primitive(struct ArrowSchema* schema, const char* fmt); // void free_malloced_schemas(struct ArrowSchema**); @@ -95,6 +96,12 @@ func releaseStream(s *CArrowArrayStream) { C.ArrowArrayStreamRelease(s) } +func exportStrArrayWithNulls(nitems int64) CArrowArray { + var arr CArrowArray + C.export_str_array_with_nulls(C.int64_t(nitems), &arr) + return arr +} + func schemaIsReleased(s *CArrowSchema) bool { return C.ArrowSchemaIsReleased(s) == 1 } From 24278c29920440f1f9e1d0130d9922a7a823147c Mon Sep 17 00:00:00 2001 From: wangting0128 Date: Wed, 12 Aug 2026 17:11:14 +0800 Subject: [PATCH 2/2] test(arrow/cdata): cover zero-length sentinel buffers Signed-off-by: wangting0128 --- go/arrow/cdata/cdata_fulltest.c | 182 ++++++++++++++++++++++++- go/arrow/cdata/cdata_test.go | 116 ++++++++++++++-- go/arrow/cdata/cdata_test_framework.go | 42 ++++++ 3 files changed, 329 insertions(+), 11 deletions(-) diff --git a/go/arrow/cdata/cdata_fulltest.c b/go/arrow/cdata/cdata_fulltest.c index 63d55a4c45e6..2b2d0ad7d9e2 100644 --- a/go/arrow/cdata/cdata_fulltest.c +++ b/go/arrow/cdata/cdata_fulltest.c @@ -338,7 +338,9 @@ static void release_str_array(struct ArrowArray* array) { if (array->buffers[0] != NULL) { free((void*) array->buffers[0]); } - free((void*) array->buffers[1]); + if (array->buffers[1] != NULL && array->buffers[1] != (void*)0x1) { + free((void*) array->buffers[1]); + } if (array->buffers[2] != NULL && array->buffers[2] != (void*)0x1) { free((void*) array->buffers[2]); } @@ -346,6 +348,19 @@ static void release_str_array(struct ArrowArray* array) { array->release = NULL; } +static void release_list_array(struct ArrowArray* array) { + assert(array->n_buffers == 2); + if (array->buffers[0] != NULL) { + free((void*) array->buffers[0]); + } + free((void*) array->buffers[1]); + free(array->buffers); + ArrowArrayRelease(array->children[0]); + free(array->children[0]); + free(array->children); + array->release = NULL; +} + void export_str_array(const char* data, const int32_t* offsets, int64_t nitems, struct ArrowArray* out) { *out = (struct ArrowArray) { .length = nitems, @@ -373,7 +388,10 @@ void export_str_array_with_nulls(int64_t nitems, struct ArrowArray* out) { .null_count = nitems, .n_buffers = 3, .n_children = 0, - .children = NULL, + // The C Data Interface permits arbitrary pointer values when + // n_children is zero. Keep a non-null sentinel here so the importer + // regression test exercises that contract as well. + .children = (struct ArrowArray**)0x1, .dictionary = NULL, // bookkeeping .release = &release_str_array @@ -389,6 +407,166 @@ void export_str_array_with_nulls(int64_t nitems, struct ArrowArray* out) { out->buffers[2] = (void*)0x1; } +void export_str_array_with_empty_values(int64_t nitems, struct ArrowArray* out) { + *out = (struct ArrowArray) { + .length = nitems, + .offset = 0, + .null_count = 0, + .n_buffers = 3, + .n_children = 0, + .children = (struct ArrowArray**)0x1, + .dictionary = NULL, + .release = &release_str_array + }; + + out->buffers = (const void**)malloc(sizeof(void*) * out->n_buffers); + assert(out->buffers != NULL); + out->buffers[0] = NULL; + out->buffers[1] = malloc((nitems + 1) * sizeof(int32_t)); + memset((void*)out->buffers[1], 0, (nitems + 1) * sizeof(int32_t)); + out->buffers[2] = (void*)0x1; +} + +void export_large_str_array_with_nulls(int64_t nitems, struct ArrowArray* out) { + *out = (struct ArrowArray) { + .length = nitems, + .offset = 0, + .null_count = nitems, + .n_buffers = 3, + .n_children = 0, + .children = (struct ArrowArray**)0x1, + .dictionary = NULL, + .release = &release_str_array + }; + + out->buffers = (const void**)malloc(sizeof(void*) * out->n_buffers); + assert(out->buffers != NULL); + int64_t bitmap_nbytes = (nitems + 7) / 8; + out->buffers[0] = malloc(bitmap_nbytes); + memset((void*)out->buffers[0], 0, bitmap_nbytes); + out->buffers[1] = malloc((nitems + 1) * sizeof(int64_t)); + memset((void*)out->buffers[1], 0, (nitems + 1) * sizeof(int64_t)); + out->buffers[2] = (void*)0x1; +} + +void export_large_str_array_with_empty_values(int64_t nitems, struct ArrowArray* out) { + *out = (struct ArrowArray) { + .length = nitems, + .offset = 0, + .null_count = 0, + .n_buffers = 3, + .n_children = 0, + .children = (struct ArrowArray**)0x1, + .dictionary = NULL, + .release = &release_str_array + }; + + out->buffers = (const void**)malloc(sizeof(void*) * out->n_buffers); + assert(out->buffers != NULL); + out->buffers[0] = NULL; + out->buffers[1] = malloc((nitems + 1) * sizeof(int64_t)); + memset((void*)out->buffers[1], 0, (nitems + 1) * sizeof(int64_t)); + out->buffers[2] = (void*)0x1; +} + +void export_list_str_array_with_null_child(int64_t nitems, struct ArrowArray* out) { + *out = (struct ArrowArray) { + .length = nitems, + .offset = 0, + .null_count = 0, + .n_buffers = 2, + .n_children = 1, + .dictionary = NULL, + .release = &release_list_array + }; + + out->buffers = (const void**)malloc(sizeof(void*) * out->n_buffers); + assert(out->buffers != NULL); + out->buffers[0] = NULL; + out->buffers[1] = malloc((nitems + 1) * sizeof(int32_t)); + int32_t* offsets = (int32_t*)out->buffers[1]; + for (int64_t i = 0; i <= nitems; ++i) { + offsets[i] = (int32_t)i; + } + + out->children = malloc(sizeof(struct ArrowArray*)); + out->children[0] = malloc(sizeof(struct ArrowArray)); + export_str_array_with_nulls(nitems, out->children[0]); +} + +void export_empty_string_view(struct ArrowArray* out) { + *out = (struct ArrowArray) { + .length = 0, + .offset = 0, + .null_count = 0, + .n_buffers = 3, + .n_children = 0, + .children = (struct ArrowArray**)0x1, + .dictionary = NULL, + .release = &release_str_array + }; + + out->buffers = (const void**)malloc(sizeof(void*) * out->n_buffers); + assert(out->buffers != NULL); + out->buffers[0] = NULL; + out->buffers[1] = (void*)0x1; + out->buffers[2] = (void*)0x1; +} + +static int all_null_string_stream_schema(struct ArrowArrayStream* st, struct ArrowSchema* out) { + out->children = malloc(sizeof(struct ArrowSchema*)); + out->n_children = 1; + out->children[0] = malloc(sizeof(struct ArrowSchema)); + *out->children[0] = (struct ArrowSchema) { + .format = "u", + .name = "value", + .metadata = NULL, + .flags = ARROW_FLAG_NULLABLE, + .children = (struct ArrowSchema**)0x1, + .n_children = 0, + .dictionary = NULL, + .release = &release_nested_static, + }; + out->format = "+s"; + out->release = &release_nested_static; + return 0; +} + +static int next_all_null_string_record(struct ArrowArrayStream* st, struct ArrowArray* out) { + struct streamcounter* cnter = (struct streamcounter*)(st->private_data); + if (cnter->n == cnter->max) { + ArrowArrayMarkReleased(out); + return 0; + } + cnter->n++; + + *out = (struct ArrowArray) { + .offset = 0, + .dictionary = NULL, + .length = 1000, + .null_count = 0, + .buffers = (const void**)malloc(sizeof(void*)), + .n_children = 1, + .n_buffers = 1, + .release = &release_the_array + }; + out->buffers[0] = NULL; + out->children = malloc(sizeof(struct ArrowArray*)); + out->children[0] = malloc(sizeof(struct ArrowArray)); + export_str_array_with_nulls(1000, out->children[0]); + return 0; +} + +void setup_all_null_string_stream(struct ArrowArrayStream* out) { + struct streamcounter* cnt = malloc(sizeof(struct streamcounter)); + cnt->max = 1; + cnt->n = 0; + out->get_next = &next_all_null_string_record; + out->get_schema = &all_null_string_stream_schema; + out->release = &release_stream; + out->private_data = cnt; +} + static int next_record(struct ArrowArrayStream* st, struct ArrowArray* out) { struct streamcounter* cnter = (struct streamcounter*)(st->private_data); if (cnter->n == cnter->max) { diff --git a/go/arrow/cdata/cdata_test.go b/go/arrow/cdata/cdata_test.go index 82e12bc3a96b..e801de2c11b8 100644 --- a/go/arrow/cdata/cdata_test.go +++ b/go/arrow/cdata/cdata_test.go @@ -712,17 +712,88 @@ func TestNestedArrays(t *testing.T) { } func TestStrArrayAllNulls(t *testing.T) { - arr := exportStrArrayWithNulls(1000) - carr, err := ImportCArrayWithType(&arr, arrow.BinaryTypes.String) + for _, tc := range []struct { + name string + dt arrow.DataType + export func(int64) CArrowArray + }{ + {name: "string", dt: arrow.BinaryTypes.String, export: exportStrArrayWithNulls}, + {name: "binary", dt: arrow.BinaryTypes.Binary, export: exportStrArrayWithNulls}, + {name: "large_string", dt: arrow.BinaryTypes.LargeString, export: exportLargeStrArrayWithNulls}, + {name: "large_binary", dt: arrow.BinaryTypes.LargeBinary, export: exportLargeStrArrayWithNulls}, + } { + t.Run(tc.name, func(t *testing.T) { + arr := tc.export(1000) + carr, err := ImportCArrayWithType(&arr, tc.dt) + require.NoError(t, err) + defer carr.Release() + + buffer := carr.Data().Buffers()[2] + require.NotNil(t, buffer) + require.Empty(t, buffer.Bytes()) + assert.NotEqual(t, uintptr(1), uintptr(unsafe.Pointer(unsafe.SliceData(buffer.Buf())))) + assert.Equal(t, 1000, carr.Len()) + assert.Equal(t, 1000, carr.NullN()) + for i := 0; i < carr.Len(); i++ { + assert.True(t, carr.IsNull(i)) + } + }) + } +} + +func TestStrArrayEmptyValues(t *testing.T) { + for _, tc := range []struct { + name string + dt arrow.DataType + export func(int64) CArrowArray + }{ + {name: "string", dt: arrow.BinaryTypes.String, export: exportStrArrayWithEmptyValues}, + {name: "binary", dt: arrow.BinaryTypes.Binary, export: exportStrArrayWithEmptyValues}, + {name: "large_string", dt: arrow.BinaryTypes.LargeString, export: exportLargeStrArrayWithEmptyValues}, + {name: "large_binary", dt: arrow.BinaryTypes.LargeBinary, export: exportLargeStrArrayWithEmptyValues}, + } { + t.Run(tc.name, func(t *testing.T) { + arr := tc.export(1000) + carr, err := ImportCArrayWithType(&arr, tc.dt) + require.NoError(t, err) + defer carr.Release() + + buffer := carr.Data().Buffers()[2] + require.NotNil(t, buffer) + require.Empty(t, buffer.Bytes()) + assert.NotEqual(t, uintptr(1), uintptr(unsafe.Pointer(unsafe.SliceData(buffer.Buf())))) + assert.Equal(t, 1000, carr.Len()) + assert.Zero(t, carr.NullN()) + for i := 0; i < carr.Len(); i++ { + assert.Empty(t, carr.ValueStr(i)) + } + }) + } +} + +func TestListStringArrayAllNullChildValues(t *testing.T) { + arr := exportListStrArrayWithNullChild(1000) + imported, err := ImportCArrayWithType(&arr, arrow.ListOf(arrow.BinaryTypes.String)) require.NoError(t, err) - defer carr.Release() + defer imported.Release() + + list := imported.(*array.List) + assert.Equal(t, 1000, list.Len()) + values := list.ListValues() + assert.Equal(t, 1000, values.NullN()) + buffer := values.Data().Buffers()[2] + require.NotNil(t, buffer) + require.Empty(t, buffer.Bytes()) + assert.NotEqual(t, uintptr(1), uintptr(unsafe.Pointer(unsafe.SliceData(buffer.Buf())))) +} - buffer := carr.Data().Buffers()[2] - assert.NotNil(t, buffer) - bs := buffer.Bytes() - assert.Equal(t, 1000, carr.Len()) - assert.Equal(t, 1000, carr.NullN()) - assert.Empty(t, bs) +func TestEmptyStringViewArbitraryZeroLengthPointer(t *testing.T) { + arr := exportEmptyStringView() + imported, err := ImportCArrayWithType(&arr, arrow.BinaryTypes.StringView) + require.NoError(t, err) + defer imported.Release() + assert.Zero(t, imported.Len()) + assert.IsType(t, &array.StringView{}, imported) } func TestRecordBatch(t *testing.T) { @@ -782,6 +853,33 @@ func TestRecordReaderStream(t *testing.T) { } } +func TestRecordReaderStreamAllNullStrings(t *testing.T) { + stream := allNullStringStreamTest() + defer releaseStream(stream) + + rdr, err := ImportCRecordReader(stream, nil) + require.NoError(t, err) + + rec, err := rdr.Read() + require.NoError(t, err) + require.NotNil(t, rec) + assert.EqualValues(t, 1000, rec.NumRows()) + require.EqualValues(t, 1, rec.NumCols()) + col := rec.Column(0) + assert.Equal(t, 1000, col.NullN()) + for i := 0; i < col.Len(); i++ { + assert.True(t, col.IsNull(i)) + } + + buffer := col.Data().Buffers()[2] + require.NotNil(t, buffer) + require.Empty(t, buffer.Bytes()) + assert.NotEqual(t, uintptr(1), uintptr(unsafe.Pointer(unsafe.SliceData(buffer.Buf())))) + + _, err = rdr.Read() + assert.ErrorIs(t, err, io.EOF) +} + func TestExportRecordReaderStream(t *testing.T) { reclist := arrdata.Records["primitives"] rdr, _ := array.NewRecordReader(reclist[0].Schema(), reclist) diff --git a/go/arrow/cdata/cdata_test_framework.go b/go/arrow/cdata/cdata_test_framework.go index 43b982e8dc7a..e936325343cc 100644 --- a/go/arrow/cdata/cdata_test_framework.go +++ b/go/arrow/cdata/cdata_test_framework.go @@ -26,6 +26,7 @@ package cdata // #include "arrow/c/helpers.h" // // void setup_array_stream_test(const int n_batches, struct ArrowArrayStream* out); +// void setup_all_null_string_stream(struct ArrowArrayStream* out); // static struct ArrowArray* get_test_arr() { // struct ArrowArray* array = (struct ArrowArray*)malloc(sizeof(struct ArrowArray)); // memset(array, 0, sizeof(*array)); @@ -47,6 +48,11 @@ package cdata // void export_int32_type(struct ArrowSchema* schema); // void export_int32_array(const int32_t*, int64_t, struct ArrowArray*); // void export_str_array_with_nulls(int64_t nitems, struct ArrowArray* out); +// void export_str_array_with_empty_values(int64_t nitems, struct ArrowArray* out); +// void export_large_str_array_with_nulls(int64_t nitems, struct ArrowArray* out); +// void export_large_str_array_with_empty_values(int64_t nitems, struct ArrowArray* out); +// void export_list_str_array_with_null_child(int64_t nitems, struct ArrowArray* out); +// void export_empty_string_view(struct ArrowArray* out); // int test1_is_released(); // void test_primitive(struct ArrowSchema* schema, const char* fmt); // void free_malloced_schemas(struct ArrowSchema**); @@ -102,6 +108,36 @@ func exportStrArrayWithNulls(nitems int64) CArrowArray { return arr } +func exportStrArrayWithEmptyValues(nitems int64) CArrowArray { + var arr CArrowArray + C.export_str_array_with_empty_values(C.int64_t(nitems), &arr) + return arr +} + +func exportLargeStrArrayWithNulls(nitems int64) CArrowArray { + var arr CArrowArray + C.export_large_str_array_with_nulls(C.int64_t(nitems), &arr) + return arr +} + +func exportLargeStrArrayWithEmptyValues(nitems int64) CArrowArray { + var arr CArrowArray + C.export_large_str_array_with_empty_values(C.int64_t(nitems), &arr) + return arr +} + +func exportListStrArrayWithNullChild(nitems int64) CArrowArray { + var arr CArrowArray + C.export_list_str_array_with_null_child(C.int64_t(nitems), &arr) + return arr +} + +func exportEmptyStringView() CArrowArray { + var arr CArrowArray + C.export_empty_string_view(&arr) + return arr +} + func schemaIsReleased(s *CArrowSchema) bool { return C.ArrowSchemaIsReleased(s) == 1 } @@ -391,6 +427,12 @@ func arrayStreamTest() *CArrowArrayStream { return st } +func allNullStringStreamTest() *CArrowArrayStream { + st := C.get_test_stream() + C.setup_all_null_string_stream(st) + return st +} + func exportedStreamTest(reader array.RecordReader) error { out := C.get_test_stream() ExportRecordReader(reader, out)