GH-50184: [C++][Parquet] Avoid reading past truncated statistics values in FormatStatValue by jmestwa-coder · Pull Request #50025 · apache/arrow

jmestwa-coder · 2026-05-23T10:33:35Z

Rationale for this change

FormatStatValue in cpp/src/parquet/types.cc does fixed-width memcpy/byte loads on a statistics value that comes straight out of the file's Thrift-encoded metadata, so its length is attacker controlled. The Parquet spec allows truncated statistics, so a min_value/max_value shorter than the column's physical type (for example a zero-byte stat on an INT96 column) is valid input, but the fixed-width loads still read past the end of the source buffer. It is reachable from the printer/debug path that formats a file's column statistics.

What changes are included in this PR?

Each fixed-width read in FormatStatValue and the helpers it delegates to now clamps its copy to val.size() instead of assuming the full type width is present: the BOOLEAN/INT32/INT64/INT96/FLOAT/DOUBLE numeric loads, the int32/int64-backed decimal loads, and the float16 FIXED_LEN_BYTE_ARRAY load. A truncated value is zero-padded rather than over-read, and no exception is raised since truncated statistics are spec-legal. The variable-length FIXED_LEN_BYTE_ARRAY (decimal/string) and BYTE_ARRAY paths were already keyed off val.size() and are unchanged.

Are these changes tested?

Yes. TypePrinter.StatisticsTypesShortValue in types_test.cc formats a too-short value for each path and checks it returns without reading past the buffer (caught under ASan).

Are there any user-facing changes?

No API change. A truncated statistics value is formatted from the bytes that are present instead of reading out of bounds.

This PR contains a "Critical Fix". It stops an out-of-bounds read on a buffer whose length is controlled by the input Parquet file.

GitHub Issue: [C++][Parquet] FormatStatValue reads past the buffer for too-short statistics values #50184

wgtmac · 2026-05-24T09:44:08Z

+      break;
+    case Type::BYTE_ARRAY:
+    case Type::FIXED_LEN_BYTE_ARRAY:
+      if (logical_type != nullptr && logical_type->is_float16()) {


Why not dealing with the length of FIXED_LEN_BYTE_ARRAY?

FormatStatValue doesn't receive the column's type_length, so it can't validate the full FLBA width here. The only fixed-size read for FLBA is the 2-byte float16 load, which is what this case guards. The decimal, string and hex paths all key off val.size() and stay in bounds.

I admit that it is better than nothing to check types with known sizes. It still looks confusing with this partial support. If we do want to add the check, I think we need to support all types. We can either pass the type instance instead of only the enum or pass the type length to FormatStatValue. The challenge is that FormatStatValue is a public API so we cannot changing its signature without breaking backward compatibility.

BTW, we need to create an issue for changes like this.

Fair. Full per-length validation needs the column type_length, and since FormatStatValue is public I left the signature alone, so the guard only covers the types that do an unconditional fixed-width load (BOOLEAN/INT32/INT64/INT96/FLOAT/DOUBLE and the float16 FLBA), which are the ones that actually over-read. The variable-length FLBA (decimal/string) and BYTE_ARRAY paths are already size-driven and stay in bounds. If you'd rather thread the type_length through a new overload to cover everything, I'm happy to go that way.

wgtmac · 2026-05-24T09:45:27Z

+    case Type::INT96:
+      required = 3 * sizeof(int32_t);
+      break;
+    case Type::BYTE_ARRAY:


It is better to put BYTE_ARRAY and UNDEFINED together since they are skipped. In case you don't know, BYTE_ARRAY cannot have float16 logical type.

Good point. Moved BYTE_ARRAY down with UNDEFINED since it can't carry a float16 logical type and has no fixed-width load here anyway.

wgtmac · 2026-05-24T09:46:47Z

+    default:
+      break;
+  }
+  if (val.size() < required) {


I'm a little hesitant to complicate FormatStatValue by introducing this check. FormatStatValue is usually called in the printer when we want to print some information of a parquet file for debugging purpuse.

Fair, it's mostly the debug/printer path. The thing is val is taken verbatim from the file's Thrift stats, so a truncated min/max makes the BOOLEAN/INT96/numeric memcpys read past the end of the buffer even there. The guard is just a size check in front of the existing loads, so I tried to keep it minimal. Happy to trim it further if it still feels too heavy.

I admit that it is better than nothing to check types with known sizes. It still looks confusing with this partial support. If we do want to add the check, I think we need to support all types. We can either pass the type instance instead of only the enum or pass the type length to FormatStatValue. The challenge is that FormatStatValue is a public API so we cannot changing its signature without breaking backward compatibility.

(I thought I have posted the above comment but it was a draft before I posted it just now)

@pitrou Do you have any suggestion?

switched to clamping each fixed-width read to val.size() rather than the required-length table, so every over-reading path is covered without touching the public signature, and there's no throw since truncated stats are valid.

github-actions · 2026-06-15T18:59:29Z

⚠️ GitHub issue #50184 has been automatically assigned in GitHub to PR creator.

jmestwa-coder · 2026-06-15T19:00:22Z

Filed #50184 for this and retitled the PR to reference it. Also reworked the description into the template sections.

pitrou · 2026-06-16T08:52:57Z

The Parquet spec allows for truncated statistics, so raising an exception sounds wrong here. Just make sure that we're not copying too many bytes from the std::string_view .

…ngth The Parquet spec allows truncated statistics, so a too-short min/max is valid and must not throw. Bound every fixed-width memcpy/Float16 load in FormatStatValue to val.size() and zero-pad the rest instead of reading past the buffer.

jmestwa-coder · 2026-06-16T15:27:40Z

@pitrou makes sense, truncated stats are spec-legal so throwing was wrong. reworked it to clamp every fixed-width memcpy/Float16 load in FormatStatValue to val.size(), so a short min/max is zero-padded instead of read past the buffer and nothing throws. the variable-length FLBA (decimal/string) and BYTE_ARRAY paths were already size-driven, so they're untouched.

@wgtmac this also drops the partial required-length table you flagged. every read now bounds itself, so there's no half-covered enum check and the public signature stays the same. updated the test and the title/description to match.

pitrou

+1, thank you @jmestwa-coder

reject too-short statistics values in FormatStatValue

5c73eed

jmestwa-coder requested a review from wgtmac as a code owner May 23, 2026 10:33

github-actions Bot added Component: Parquet Component: C++ awaiting review Awaiting review labels May 23, 2026

wgtmac reviewed May 24, 2026

View reviewed changes

Group BYTE_ARRAY with skipped types in stat length guard

4897dee

github-actions Bot added awaiting committer review Awaiting committer review and removed awaiting review Awaiting review labels May 27, 2026

jmestwa-coder mentioned this pull request Jun 15, 2026

[C++][Parquet] FormatStatValue reads past the buffer for too-short statistics values #50184

Closed

jmestwa-coder changed the title ~~MINOR: [C++][Parquet] Reject too-short statistics values in FormatStatValue~~ GH-50184: [C++][Parquet] Reject too-short statistics values in FormatStatValue Jun 15, 2026

jmestwa-coder requested a review from pitrou as a code owner June 16, 2026 15:26

jmestwa-coder changed the title ~~GH-50184: [C++][Parquet] Reject too-short statistics values in FormatStatValue~~ GH-50184: [C++][Parquet] Avoid reading past truncated statistics values in FormatStatValue Jun 16, 2026

pitrou approved these changes Jun 17, 2026

View reviewed changes

pitrou merged commit 546d42c into apache:main Jun 17, 2026
50 of 53 checks passed

pitrou removed the awaiting committer review Awaiting committer review label Jun 17, 2026

Conversation

jmestwa-coder commented May 23, 2026 • edited Loading 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!

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!

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!

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

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!

github-actions Bot commented Jun 15, 2026

Uh oh!

jmestwa-coder commented Jun 15, 2026

Uh oh!

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

Uh oh!

Uh oh!

jmestwa-coder commented Jun 16, 2026

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmestwa-coder commented May 23, 2026 •

edited

Loading

wgtmac Jun 16, 2026 •

edited

Loading

pitrou commented Jun 16, 2026 •

edited

Loading