fix(parquet): copy ByteArray statistics min/max to prevent use-after-free#917
fix(parquet): copy ByteArray statistics min/max to prevent use-after-free#917zeroshade wants to merge 4 commits into
Conversation
…free ByteArrayStatistics, FixedLenByteArrayStatistics and Float16Statistics stored their min/max as slices aliasing the caller's value buffer (e.g. an Arrow array value buffer) instead of copying the bytes. Every update path (Update, UpdateSpaced, UpdateFromArrow, Merge) funnels through SetMinMax, which retained those slices directly. With a streaming RecordReader that releases each batch as it advances (the standard Arrow ownership contract), 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. Copy the min/max into statistics-owned buffers in SetMinMax 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. This mirrors the Apache Arrow C++ TypedStatisticsImpl, which keeps min/max in owned buffers. Min/Max now return a slice that is only valid until the next update, which is documented on the accessors. Reported at adbc-drivers/bigquery#229
FixedLenByteArrayStatistics.UpdateFromArrow accumulated the running maximum with s.maxval(min, v), comparing against the running minimum instead of the running maximum. The max statistic for FIXED_LEN_BYTE_ARRAY columns updated directly from an Arrow array could therefore be incorrect. Pass the running max instead. This is a pre-existing bug, independent of the use-after-free fix, kept as a separate commit for clarity.
…ructors
New{ByteArray,FixedLenByteArray,Float16}StatisticsFromEncoded stored the decoded min/max as slices aliasing the caller's encoded metadata buffer. Because SetMinMax now reuses the statistics-owned min/max buffers via append(s.min[:0], ...), a later Update or Merge on stats built from encoded metadata could write through that alias and corrupt the caller's buffer.
Copy the decoded min/max into statistics-owned buffers in the encoded constructors so byte-backed statistics always own their min/max. Addresses a review finding on the preceding use-after-free fix.
There was a problem hiding this comment.
Pull request overview
This PR fixes a use-after-free bug in Parquet statistics for []byte-backed physical/logical types by ensuring statistics-owned copies of min/max are stored (rather than aliasing Arrow/value buffers that may be released in streaming writes). It also corrects a separate bug in FixedLenByteArray.UpdateFromArrow where the running max was computed against the running min.
Changes:
- Copy
BYTE_ARRAY/FIXED_LEN_BYTE_ARRAY(andFLOAT16via fixed-len backing) statistics min/max into statistics-owned buffers, and document the buffer reuse semantics onMin()/Max(). - Fix
FixedLenByteArrayStatistics.UpdateFromArrowmax computation (maxval(max, v)). - Add unit and end-to-end regressions covering streaming batch release/poisoning and the max-computation fix.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| parquet/pqarrow/encode_arrow_test.go | Adds an end-to-end regression that poisons freed buffers between streamed batches to catch min/max aliasing issues deterministically. |
| parquet/metadata/statistics_types.gen.go.tmpl | Updates generator template to copy min/max for []byte-backed stats and fixes FixedLenByteArray UpdateFromArrow max logic. |
| parquet/metadata/statistics_types.gen.go | Regenerated typed statistics with the copy-on-store behavior and corrected max computation. |
| parquet/metadata/statistics_test.go | Adds targeted unit regressions for non-aliasing behavior and the FixedLenByteArray Arrow-update max bug. |
Files not reviewed (1)
- parquet/metadata/statistics_types.gen.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Mutating the merge source's backing buffer must not affect the merged stats. | ||
| copy(buf, "aaaaa") | ||
| assert.Equal(t, "kkkkk", string(dst.Min()), "merged min must be an independent copy") | ||
| assert.Equal(t, "kkkkk", string(dst.Max()), "merged max must be an independent copy") |
There was a problem hiding this comment.
Good catch — addressed in 0716782. The subtest now updates src after the merge (which overwrites src's statistics-owned min/max buffers in place via append(s.min[:0], ...)) and asserts dst stays "kkkkk", so it actually validates that Merge deep-copies rather than relying on the already-decoupled input buffer.
Per review feedback on apache#917: the Merge subtest mutated the original input buffer after the merge, but since Update now copies min/max into src-owned buffers, that buffer is already decoupled and the mutation validated nothing about Merge. Instead, update src after the merge (which overwrites src's statistics-owned min/max buffers in place) and assert dst is unchanged, which actually exercises that Merge deep-copies.
Rationale for this change
ByteArrayStatistics(and the other[]byte-backed statistics types,FixedLenByteArrayStatisticsandFloat16Statistics) stored their min/max asslices 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 throughSetMinMax, whichretained those slices directly.
When a column is written from a streaming
RecordReaderthat releases eachbatch as it advances (the standard Arrow ownership contract, e.g.
BindStream+ExecuteUpdatein the ADBC drivers), a previous batch's buffer is freed beforethe statistics are serialized. The retained min/max then reference freed memory
and
bytes.Compareinless()can segfault:This mirrors the Apache Arrow C++ implementation
(
TypedStatisticsImpl<ByteArrayType>), which stores min/max instatistics-owned buffers and never aliases the input.
Reported at adbc-drivers/bigquery#229.
What changes are included in this PR?
Two separate commits:
fix(parquet): copy ByteArray statistics min/max to prevent use-after-free
SetMinMaxnow copies min/max into statistics-owned buffers for the[]byte-backed physical types (BYTE_ARRAY,FIXED_LEN_BYTE_ARRAY, andthe
FLOAT16logical type), reusing the existing backing array to avoidextra allocations. Scalar /
[N]bytetypes are unchanged.Min()/Max()return a slice valid only until the nextupdate for those types.
(
statistics_types.gen.go.tmpl); the generated file was regenerated.fix(parquet): correct FixedLenByteArray UpdateFromArrow max computation
FixedLenByteArray.UpdateFromArrowaccumulated 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 thesource buffer being freed/overwritten for
BYTE_ARRAY,FIXED_LEN_BYTE_ARRAYand
FLOAT16(viaUpdate/UpdateSpaced/Merge), plus a dedicated test forthe
FixedLenByteArray.UpdateFromArrowmax-correctness fix.parquet/pqarrow: an end-to-end regression writes a string column across twobuffered 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
CheckedAllocatorreports no leaks. This test was confirmed to fail withoutthe fix (max reads back as
\xff\xff\xff) and to pass with it.go test ./parquet/metadata/ ./parquet/file/ ./parquet/pqarrow/all pass;gofmtandgo vetare clean.Are there any user-facing changes?
No API changes. For
BYTE_ARRAY/FIXED_LEN_BYTE_ARRAY/FLOAT16statistics,
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
FixedLenByteArrayArrow-update max is now correct.