[PR] fix(cpp): bound TS_2DIFF block headers by the page value count [tsfile]

["kkzi (via GitHub)" <gi...@apache.org>]

kkzi opened a new pull request, #931:
URL: https://github.com/apache/tsfile/pull/931

   ## 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.
   


-- 
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: notifications-unsubscribe@tsfile.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] fix(cpp): bound TS_2DIFF block headers by the page value count [tsfile]

["ColinLeeo (via GitHub)" <gi...@apache.org>]

ColinLeeo commented on PR #931:
URL: https://github.com/apache/tsfile/pull/931#issuecomment-5579280324

   Thanks for @kkzi and thanks for the PR and for bringing this issue to our attention.
   
   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.


-- 
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: notifications-unsubscribe@tsfile.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org