fix:[3.0] prevent crash on zero-length Arrow C Data buffers

Project: milvus-io/milvus | Language: Go | Concepts: memory-management, error-handling, testing-strategy | Difficulty: mid
Link: https://github.com/milvus-io/milvus/pull/52652

The problem

When importing Arrow C Data buffers with zero length, the importer could call unsafe.Slice on a sentinel address (like 0x1). This led to Go treating the sentinel as a real pointer, which could cause runtime crashes, especially during stack growth, due to invalid pointer retention.

Why it's non-trivial

A naive fix might only check for nil pointers, but the Arrow C Data interface sometimes uses sentinel values (like 0x1) to represent empty buffers. These are not nil, so code must specifically avoid treating them as valid pointers. The fix also needs to avoid introducing regressions or memory leaks, and should be covered by a regression test.

The approach

1. Upgrade the Arrow Go fork to a version that handles zero-length buffers safely:

-	github.com/apache/arrow/go/v17 => github.com/milvus-io/arrow/go/v17 v17.0.1
+	github.com/apache/arrow/go/v17 => github.com/milvus-io/arrow/go/v17 v17.0.2

This line in go.mod upgrades the Arrow Go dependency to v17.0.2, which contains the upstream fix to avoid unsafe pointer conversions on zero-length buffers.

-github.com/milvus-io/arrow/go/v17 v17.0.1 h1:juKuEgTNQbe8a+LuBnyt9/UkfllTwtQIB+K7PvK4+Y4=
-github.com/milvus-io/arrow/go/v17 v17.0.1/go.mod h1:jR7QHkODl15PfYyjM2nU+yTLScZ/qfj7OSUZmJ8putc=
+github.com/milvus-io/arrow/go/v17 v17.0.2 h1:vHaLmnERUWU5m6uggpqT2yoUV0gjvx2n9P1XjnPg01w=
+github.com/milvus-io/arrow/go/v17 v17.0.2/go.mod h1:jR7QHkODl15PfYyjM2nU+yTLScZ/qfj7OSUZmJ8putc=

go.sum is updated to match the new dependency version, ensuring reproducible builds.

2. Add a regression test to verify the fix and guard against future regressions:

+func TestIssue52233ImportAllNullStringWithSentinelValuesPointer(t *testing.T) {
+	const rowCount = 1000
+
+	builder := array.NewStringBuilder(memory.DefaultAllocator)
+	defer builder.Release()
+	builder.AppendNulls(rowCount)
+	source := builder.NewStringArray()
+	defer source.Release()
+
+	var exported cdata.CArrowArray
+	cdata.ExportArrowArray(source, &exported, nil)
+	defer cdata.ReleaseCArrowArray(&exported)
+
+	// ArrowArray is part of the stable C Data Interface ABI. Treat the buffer
+	// entries as uintptr values so the Go runtime never treats the 0x1 sentinel
+	// as a Go pointer while constructing the fixture.
+	type arrowArrayLayout struct {
+		length    int64
+		nullCount int64
+		offset    int64
+		nBuffers  int64
+		nChildren int64
+		buffers   unsafe.Pointer
+	}
+	layout := (*arrowArrayLayout)(unsafe.Pointer(&exported))
+	if layout.nBuffers != 3 {
+		t.Fatalf("C ArrowArray buffer count = %d, want 3", layout.nBuffers)
+	}
+	buffers := unsafe.Slice((*uintptr)(layout.buffers), int(layout.nBuffers))
+	buffers[2] = 1
+
+	imported, err := cdata.ImportCArrayWithType(&exported, arrow.BinaryTypes.String)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer imported.Release()
+
+	values := imported.Data().Buffers()[2]
+	if values == nil {
+		t.Fatal("values buffer is nil")
+	}
+	bytes := values.Bytes()
+	if got := len(bytes); got != 0 {
+		t.Fatalf("values buffer length = %d, want 0", got)
+	}
+	if got := imported.Len(); got != rowCount {
+		t.Fatalf("array length = %d, want %d", got, rowCount)
+	}
+	if got := imported.NullN(); got != rowCount {
+		t.Fatalf("array null count = %d, want %d", got, rowCount)
+	}
+
+	var growStack func(int) int
+	growStack = func(depth int) int {
+		var padding [1024]byte
+		padding[0] = byte(depth)
+		if depth == 0 {
+			return int(padding[0])
+		}
+		return int(padding[0]) + growStack(depth-1)
+	}
+	_ = growStack(128)
+	runtime.KeepAlive(bytes)
+}

This test constructs an Arrow array of all null strings, exports it to the C Data interface, manually sets the buffer pointer to the sentinel value 1, then imports it back. It checks that the imported buffer is non-nil, has zero length, and does not crash even after stack growth. This directly verifies the bug is fixed and prevents regressions.

From the review thread

No notable review discussion on this PR.

Takeaways

• When dealing with C data interfaces in Go, always account for sentinel values (not just nil) that may represent empty buffers, to avoid unsafe pointer conversions.
• Upgrading dependencies to pull in upstream bugfixes is often safer and more maintainable than patching around issues locally.
• Adding regression tests that reproduce subtle memory or pointer bugs is essential to prevent future reintroductions of such errors.