Skip to content

fix(cpp): bound TS_2DIFF block headers by the page value count - #931

Open
kkzi wants to merge 1 commit into
apache:developfrom
kkzi:fix/cpp-ts2diff-page-value-count-bound
Open

fix(cpp): bound TS_2DIFF block headers by the page value count#931
kkzi wants to merge 1 commit into
apache:developfrom
kkzi:fix/cpp-ts2diff-page-value-count-bound

Conversation

@kkzi

@kkzi kkzi commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What

A crafted 16-byte TS_2DIFF block header (writeIndex = INT32_MAX, bitWidth = 0) makes the decoder emit up to 2^31 values, hanging the reader loops (while (decoder->has_remaining(...))). Zero-width blocks consume no packed bytes, so byte availability cannot bound writeIndex. Affected: plain int32/int64 pages (no page metadata at all) and Form 1 float/double pages (metadata carries no value count). Form 2/3 pages were already bounded.

Fix

Bound block headers by the page's point count from the page header statistic:

  • Decoder gains a set_page_value_count() hook (default no-op).
  • TS2DIFFDecoder rejects blocks with writeIndex + 1 > page count (E_DECODE_ERR).
  • Every page-decode site plumbs the count: ChunkReader::decode_cur_page_data (time + value), AlignedChunkReader::decode_cur_time_page_data / decode_cur_value_page_data / decode_time_page_with (sequential and worker-pool). The aligned value-page predecode path is already bounded by its non-null count.

Java grounding: DeltaBinaryDecoder allocates new int[packNum], so such pages are not valid Java input either.

Tests

  • 4 codec-level tests: crafted plain-int / Form 1 pages rejected, zero-count page rejects any block, count survives the reader's reset sequence.
  • 2 round-trip tests pin the count units against false rejections: TS_2DIFF columns with sparse nulls (value pages count non-null values, time pages count rows) and a fully-null column (legitimate zero-count page).
  • Full suite: 811 passed locally (MSVC); clang-format and cppcheck clean against baseline.

Known residuals (format-level, documented in the commit)

  • Single-page chunks carry no page statistic on the wire, so those pages decode unbounded, as before.
  • The bound enforces the block-vs-page invariant, not an absolute work limit: a crafted file also controls the statistic itself.

A zero-bit-width block consumes no packed bytes, so byte availability
cannot bound its writeIndex.  With no page-value-count bound in scope,
a 16-byte crafted block header (writeIndex = INT32_MAX, bitWidth = 0)
made the decoder emit 2^31 values and hang the reader loops.  Plain
int32/int64 TS_2DIFF pages (no page metadata at all) and Form 1
float/double pages (metadata carries no value count) were both
affected; Form 2/3 pages were already bounded.

Root-cause fix where all readers route through: Decoder gains a
set_page_value_count() hook (default no-op), TS2DIFFDecoder bounds
writeIndex by it, and every page-decode site plumbs the page's point
count into the decoder: ChunkReader::decode_cur_page_data (time and
value), AlignedChunkReader::decode_cur_time_page_data /
decode_cur_value_page_data, and AlignedChunkReader::
decode_time_page_with (sequential and worker-pool time decode, whose
count is captured from the page statistic at plan time — row_begin/
row_end cannot serve, being the filtered sub-span on boundary pages).
The aligned value-page predecode path is already bounded by the page's
non-null count.  Float/Double decoders apply the plumbed count in
ensure_page_meta for Form 1 pages; a page declaring zero points
rejects every block rather than disabling the bound.

Known residuals, both rooted in the format rather than this change:
- Single-page chunks carry no page statistic on the wire (the page
  header is deserialized without statistics when the chunk has one
  page), so those pages keep decoding unbounded.
- The bound is only as tight as the page statistic itself, which a
  crafted file also controls; it enforces the block-vs-page invariant,
  not an absolute work limit.  The single-page recovery loops in
  RestorableTsFileIoWriter likewise keep relying on read errors.

Java grounding: DeltaBinaryDecoder allocates new int[packNum], so a
writeIndex = INT32_MAX page is not valid Java input either.
@ColinLeeo

ColinLeeo commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Thanks for @kkzi and thanks for the PR.

I think we should distinguish between two goals here. TsFile reads data according to the lengths, counts, offsets, and encoding parameters stored in the file. We should certainly ensure that loops make progress, stop on read errors, and perform the necessary bounds, integer overflow, and consistency checks. But guaranteeing that arbitrary malicious input cannot exhaust CPU or memory is a separate goal.

Using the point count from the page statistics to bound a TS_2DIFF block does catch some inconsistencies. However, those statistics also come from the file and can be modified, so this check does not establish a reliable limit on resource consumption. If we want to address arbitrary manipulation of file contents, the scope extends beyond TS_2DIFF to other counts, lengths, offsets, encodings, compression methods, and language implementations. Java currently has no equivalent page-count bound either, and failing to allocate a large array should not be considered protection against resource exhaustion.

If we want to explicitly support files from untrusted sources, I suggest discussing that separately: which inputs to reject, how much resource consumption to allow, when to stop processing, and how the different language implementations should handle these cases. This can be done incrementally and does not need to be solved in one PR, but we should have a clear direction first.

For these reasons, I would prefer to hold off on merging the current version. If we want to pursue it as a local data consistency check, please clarify what the interface guarantees, where the point count comes from in each case, and add the corresponding tests. That would help us assess whether the benefit justifies the additional maintenance cost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants