Skip to content

Fix excessive allocation in MsgPack schema inference for huge container headers#109019

Merged
Algunenano merged 3 commits into
ClickHouse:masterfrom
groeneai:fix-msgpack-schema-inference-allocation-limit
Jul 8, 2026
Merged

Fix excessive allocation in MsgPack schema inference for huge container headers#109019
Algunenano merged 3 commits into
ClickHouse:masterfrom
groeneai:fix-msgpack-schema-inference-allocation-limit

Conversation

@groeneai

@groeneai groeneai commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Changelog category (leave one):

  • Bug Fix (user-visible misbehavior in an official stable release)

Changelog entry (a user-readable short description of the changes that goes into CHANGELOG.md):

Fixed an excessive memory allocation during schema inference of the MsgPack format: a corrupted or fuzzed input whose array/map/string/binary header declares a huge element count no longer drives a single multi-gigabyte allocation (it is now rejected as malformed input). This fixes an allocation-size-too-big abort under sanitizers and an out-of-memory condition in release builds.

Description

No related open issue found. Found by the CI stress fuzzer (AddressSanitizer, STID 3243-1df9) on the Stress test (arm_asan_ubsan) job.

Report: https://s3.amazonaws.com/clickhouse-test-reports/json.html?PR=108724&sha=48e7880e5fbbdf289911c01ba3f856af22d85f64&name_0=PR&name_1=Stress%20test%20%28arm_asan_ubsan%29

Signature:

==3172==ERROR: AddressSanitizer: requested allocation size 0x2000000008 (0x2000001008 after adjustments for alignment, red zones etc.) exceeds maximum supported size of 0x800000000 (thread T522 (TCPHandler))
  ...
  #12 DB::MsgPackSchemaReader::readObject()            src/Processors/Formats/Impl/MsgPackRowInputFormat.cpp:620
  #13 DB::MsgPackSchemaReader::readRowAndGetDataTypes()  MsgPackRowInputFormat.cpp:731

Root cause: MsgPackSchemaReader::readObject() called msgpack::unpack() with the default msgpack::unpack_limit(), which caps array/map/str/bin/ext at 0xffffffff. msgpack::unpack builds the whole object tree up front, and for each container it eagerly allocates storage sized by the count/length declared in the header (create_object_visitor::start_map/start_array/visit_str/... call zone::allocate_align(count * sizeof(...))) before reading the payload. A tiny fuzzed header, e.g. an array32 declaring 0xffffffff elements (5 bytes: dd ff ff ff ff), therefore drives a single ~128 GiB allocation that bypasses the query memory tracker: it aborts under a sanitizer (allocation-size-too-big) and throws std::bad_alloc / exhausts memory in release. This is schema-inference-only; the runtime MsgPackVisitor uses a streaming parser and is not affected.

Fix: bound every container by the number of bytes currently buffered. A valid element occupies at least one encoded byte, so a fully buffered object never trips the bound, while an over-declaration throws msgpack::size_overflow, handled exactly like insufficient_bytes (grow the buffer and retry; reject as UNEXPECTED_END_OF_FILE once the whole input is buffered and it still does not fit). Covers all eager-allocation paths (array/map/str/bin/ext).

Reproducer (rejected cleanly after the fix, no giant allocation):

printf '\xdd\xff\xff\xff\xff' | clickhouse local --input-format MsgPack --input_format_msgpack_number_of_columns=1 -q "SELECT toTypeName(c1) FROM table"

Regression test: tests/queries/0_stateless/04366_msgpack_schema_inference_huge_container.sh (over-declared array32/map32/str32/bin32 rejected; valid data, including a multi-megabyte string that spans several buffer reads, still inferred correctly).

Version info

  • Merged into: 26.7.1.681 (included in 26.7 and later)

…er headers

MsgPackSchemaReader::readObject() called msgpack::unpack() with the default
unpack_limit(), which caps array/map/str/bin/ext at 0xffffffff. msgpack::unpack
builds the object tree up front and eagerly allocates storage sized by the
count/length declared in the header before reading the payload, so a tiny fuzzed
header (e.g. an array32 declaring 0xffffffff elements) drove a single
multi-gigabyte allocation that bypassed the query memory tracker: an
allocation-size-too-big abort under sanitizers, and std::bad_alloc / OOM in
release builds.

Bound every container by the bytes currently buffered. A valid element occupies
at least one encoded byte, so a fully buffered object never trips the bound,
while an over-declaration throws msgpack::size_overflow, handled exactly like
insufficient_bytes (grow the buffer and retry; reject as UNEXPECTED_END_OF_FILE
once the whole input is buffered and it still does not fit). Covers all
eager-allocation paths (array/map/str/bin/ext). Schema-inference only; the
runtime MsgPackVisitor uses a streaming parser and is unaffected.

Found by the CI stress fuzzer (AddressSanitizer, STID 3243-1df9).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

groeneai commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author
Pre-PR validation gate (click to expand)
# Question Answer
a Deterministic repro? Yes. 5-byte input printf '\xdd\xff\xff\xff\xff' (array32 header declaring 0xffffffff elements) piped to clickhouse local --input-format MsgPack --input_format_msgpack_number_of_columns=1 -q "SELECT toTypeName(c1) FROM table" aborts every time on an unfixed ASan build with AddressSanitizer: requested allocation size 0x2000000008 exceeds maximum supported size. Same value as CI STID 3243-1df9.
b Root cause explained? MsgPackSchemaReader::readObject() passed the default msgpack::unpack_limit() (cap 0xffffffff). msgpack::unpack builds the DOM up front: create_object_visitor::start_map/start_array/visit_str/... allocate count*sizeof(...) via the zone allocator BEFORE reading the payload. A fuzzed header declaring ~2^32 elements ⇒ one ~128 GiB malloc that bypasses the query memory tracker ⇒ ASan abort (release: std::bad_alloc/OOM).
c Fix matches root cause? Yes. Bounds the unpack_limit for array/map/str/bin/ext by the bytes currently buffered — directly caps the count the visitor is allowed to allocate for, at the exact start_map/start_array/visit_* pre-allocation check. Not a symptom guard.
d Test intent preserved / new tests added? New regression test 04366_msgpack_schema_inference_huge_container.sh: over-declared array32/map32/str32/bin32 rejected as UNEXPECTED_END_OF_FILE; valid data (incl. a 2 MB string spanning several buffer reads) still inferred. Existing msgpack tests 04335/04338/04365/04029/03717/02594/02422 all pass.
e Both directions demonstrated? Yes. Unfixed ASan binary (build id b494fc46): ASan abort. Fixed binary (a99b2e33): clean UNEXPECTED_END_OF_FILE, no allocation. New test 20/20 with CI randomization; valid inference unchanged.
f Fix is general across code paths? Yes. The bound covers all five eager-allocation paths in create_object_visitor (array/map/str/bin/ext), verified individually. The sibling runtime path (MsgPackVisitor) is a streaming parser with no eager DOM allocation and is unaffected.
g Fix generalizes across inputs? Yes. Verified array32/map32/str32/bin32/ext32 over-declarations all rejected; valid fixint/array/map/nested-array-of-maps/empty-fixstr and a 3 MB valid string still infer correctly. Bound holds for every declared count because a valid element occupies >=1 encoded byte.
h Backward compatible? Yes. No setting/format/wire change. Only malformed inputs that previously aborted/OOM'd now raise a clean exception; all well-formed MsgPack behaves identically. No SettingsChangesHistory.cpp change needed.
i Invariants and contracts preserved? Yes. readObject()'s contract (return one parsed object, grow the peekable buffer on partial input, throw at EOF) is preserved: size_overflow is routed through the same grow-and-retry as insufficient_bytes. depth limit left at default so the existing deep-recursion handling in getDataType/checkStackSize is untouched.

Session id: cron:clickhouse-worker-slot-5:20260701-041300

@groeneai

groeneai commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

cc @Avogar @al13n321 - could you review this? It bounds msgpack::unpack's array/map/str/bin/ext limits by the bytes currently buffered in MsgPack schema inference, so a fuzzed container header (e.g. an array32 declaring 0xffffffff elements) is rejected as malformed instead of driving a single ~128 GiB allocation (ASan allocation-size-too-big abort / OOM in release). Found by the CI stress fuzzer (STID 3243-1df9).

@alexey-milovidov alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 1, 2026
@clickhouse-gh

clickhouse-gh Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Workflow [PR], commit [65b84b8]

Summary:


AI Review

Summary

This PR hardens MsgPack schema inference by bounding msgpack::unpack allocations for oversized container headers, by threading max_parser_depth into the DOM-building path, and by adding focused regressions for malformed array32/map32/str32/bin32 headers plus valid boundary cases. I re-checked the current code and prior discussion, including the earlier MAP underbound thread, and I did not find any remaining correctness or safety issue in the current diff.

Final Verdict
  • Status: ✅ Approve

@groeneai

groeneai commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

CI finish ledger — 4f77970

Every failure below has an owner: a fixing PR (ours or external). Neither is caused by this PR (it touches only `src/Processors/Formats/Impl/MsgPackRowInputFormat.cpp` + a MsgPack schema-inference test; it cannot affect the query planner or IntersectOrExceptStep).

Check / test Reason Owner / fixing PR
AST fuzzer (amd_debug/amd_msan) / STID 0993-2cc2 Block structure mismatch in IntersectOrExceptStep stream flaky (chronic trunk: 20+ unrelated PRs, 2 master hits / 30d) #107719 (ours, open)
PR (aggregator) rollup rolls up the AST fuzzer red above

Session id: cron:our-pr-ci-monitor:20260701-130000

@Algunenano Algunenano self-assigned this Jul 7, 2026
Comment thread src/Processors/Formats/Impl/MsgPackRowInputFormat.cpp Outdated
Thread format_settings.max_parser_depth into msgpack::unpack as the depth limit
so a deeply nested header is rejected while the DOM tree is being built, before
getDataType walks it and before the deep tree is fully allocated. Extends the
allocation bounding already applied to array/map/str/bin/ext sizes.

msgpack throws msgpack::depth_size_overflow, which derives from size_overflow, so
it must be caught before the size_overflow grow-and-retry handler; otherwise deep
nesting would be misrouted into UNEXPECTED_END_OF_FILE. It is rejected with the
same TOO_DEEP_RECURSION message getDataType uses. max_parser_depth == 0 means
unlimited (matching getDataType and the SQL parser); msgpack spells that 0xffffffff.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@groeneai

groeneai commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

Good catch, done in ac873e2.

unpack_limit takes a 6th depth arg (default 0xffffffff), and msgpack::unpack enforces it while building the DOM, so threading max_parser_depth in rejects a deeply nested header before the deep tree is allocated. getDataType already enforced the same limit, but only after unpack had built the whole tree, so this moves the check earlier and completes the allocation bounding.

One subtlety: msgpack throws msgpack::depth_size_overflow, which derives from size_overflow, so it needed a catch before the existing size_overflow grow-and-retry handler (otherwise deep nesting would be misreported as UNEXPECTED_END_OF_FILE). It now throws the same TOO_DEEP_RECURSION as getDataType. max_parser_depth == 0 maps to msgpack's unlimited sentinel 0xffffffff, matching getDataType and the SQL parser. Added depth cases to the regression test.

The runtime (data) path is unaffected: it uses the streaming parse_helper with a heap-backed stack (no eager sizing, no native-stack recursion), not unpack.

Comment thread src/Processors/Formats/Impl/MsgPackRowInputFormat.cpp Outdated
msgpack allocates sizeof(object_kv) per declared map pair as soon as the
pair count fits limit.map(), before the payload is checked for
completeness. A valid map pair is two encoded objects (>= 2 bytes), so
bounding the pair count by the raw byte count let a malformed map32
over-declare by almost 2x and still drive a large object_kv allocation
before insufficient_bytes fired. Cap the map slot at available / 2: a
fully buffered valid map has payload >= 2 * pairs so it never trips the
bound, while an over-declaration is rejected before the allocation.

Add a map32 regression case that keeps a full payload buffered past the
header (exercising the map allocation path the raw available bound left
open) plus a tight-boundary fixmap case guarding that valid maps are not
over-rejected.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@clickhouse-gh

clickhouse-gh Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

LLVM Coverage Report

Metric Baseline Current Δ
Lines 85.70% 85.70% +0.00%
Functions 92.70% 92.70% +0.00%
Branches 77.90% 77.90% +0.00%

Changed lines: Changed C/C++ lines covered: 32/32 (100.00%) · Uncovered code

Full report · Diff report

@groeneai

groeneai commented Jul 7, 2026

Copy link
Copy Markdown
Contributor Author

CI finish ledger — 65b84b8

CI fully finished (Finish Workflow pass, Mergeable Check pass). No failed checks. All builds, functional/integration/stress/fuzzer/perf checks green after the map-underbound tightening (commit 65b84b8).

Check / test Reason Owner / fixing PR
(none)

Session id: cron:our-pr-ci-monitor:20260707-190000

@Algunenano
Algunenano added this pull request to the merge queue Jul 8, 2026
Merged via the queue into ClickHouse:master with commit da1e3b3 Jul 8, 2026
174 checks passed
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

can be tested Allows running workflows for external contributors pr-bugfix Pull request with bugfix, not backported by default pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants