zeroshade opened a new pull request, #917: URL: https://github.com/apache/arrow-go/pull/917
### Rationale for this change `ByteArrayStatistics` (and the other `[]byte`-backed statistics types, `FixedLenByteArrayStatistics` and `Float16Statistics`) stored their min/max as slices that **alias the caller's value buffer** — e.g. an Arrow array's value buffer — instead of copying the bytes. Every update path (`Update`, `UpdateSpaced`, `UpdateFromArrow`, `Merge`) funnels through `SetMinMax`, which retained those slices directly. When a column is written from a streaming `RecordReader` that releases each batch as it advances (the standard Arrow ownership contract, e.g. `BindStream` + `ExecuteUpdate` in the ADBC drivers), a previous batch's buffer is freed before the statistics are serialized. The retained min/max then reference freed memory and `bytes.Compare` in `less()` can segfault: ``` ByteArrayStatistics.less (statistics.go) ByteArrayStatistics.SetMinMax (statistics_types.gen.go) ByteArrayStatistics.UpdateSpaced (statistics_types.gen.go) ByteArrayColumnChunkWriter.writeValuesSpaced ByteArrayColumnChunkWriter.WriteBatchSpaced pqarrow.writeDenseArrow ``` This mirrors the Apache Arrow C++ implementation (`TypedStatisticsImpl<ByteArrayType>`), which stores min/max in statistics-owned buffers and never aliases the input. Reported at https://github.com/adbc-drivers/bigquery/issues/229. ### What changes are included in this PR? Two separate commits: 1. **fix(parquet): copy ByteArray statistics min/max to prevent use-after-free** - `SetMinMax` now copies min/max into statistics-owned buffers for the `[]byte`-backed physical types (`BYTE_ARRAY`, `FIXED_LEN_BYTE_ARRAY`, and the `FLOAT16` logical type), reusing the existing backing array to avoid extra allocations. Scalar / `[N]byte` types are unchanged. - Documents that `Min()`/`Max()` return a slice valid only until the next update for those types. - The change is made in the generator template (`statistics_types.gen.go.tmpl`); the generated file was regenerated. 2. **fix(parquet): correct FixedLenByteArray UpdateFromArrow max computation** - Pre-existing, independent bug: `FixedLenByteArray.UpdateFromArrow` accumulated the running max with `maxval(min, v)` (the running *min*) instead of `maxval(max, v)`. Kept as a separate commit for clarity. ### Are these changes tested? Yes. - `parquet/metadata`: new unit regressions assert stored min/max survive the source buffer being freed/overwritten for `BYTE_ARRAY`, `FIXED_LEN_BYTE_ARRAY` and `FLOAT16` (via `Update`/`UpdateSpaced`/`Merge`), plus a dedicated test for the `FixedLenByteArray.UpdateFromArrow` max-correctness fix. - `parquet/pqarrow`: an end-to-end regression writes a string column across two buffered batches, releasing each batch before the next write. A poison-on-free allocator overwrites released buffers so the use-after-free is deterministic; the test asserts the round-tripped min/max are correct and the `CheckedAllocator` reports no leaks. This test was confirmed to fail without the fix (max reads back as `\xff\xff\xff`) and to pass with it. - `go test ./parquet/metadata/ ./parquet/file/ ./parquet/pqarrow/` all pass; `gofmt` and `go vet` are clean. ### Are there any user-facing changes? No API changes. For `BYTE_ARRAY` / `FIXED_LEN_BYTE_ARRAY` / `FLOAT16` statistics, `Min()`/`Max()` now return a slice backed by a reused, statistics-owned buffer that is only valid until the next update that replaces the value (documented on the accessors). Statistics min/max are now correct in the streaming scenario that previously crashed, and the `FixedLenByteArray` Arrow-update max is now correct. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
