fix(ipc): bound the sizes MessageDecoder asks callers to allocate - #9186
Draft
joseph-isaacs wants to merge 1 commit into
Draft
fix(ipc): bound the sizes MessageDecoder asks callers to allocate#9186joseph-isaacs wants to merge 1 commit into
joseph-isaacs wants to merge 1 commit into
Conversation
An IPC message declares the length of its flatbuffer header and of its body before either has been received, and `PollRead::NeedMore` asks the caller to size its buffer from those declarations. Neither was bounded, so a short frame declaring a huge body made `SyncMessageReader` and `AsyncMessageReader` resize their read buffers to an arbitrary length before a single body byte had arrived. Add `MessageLimits` and enforce it in `MessageDecoder::read_next`, which rejects an oversized declaration before reporting `NeedMore`. Putting the check in the decoder rather than at each `resize` call site means every reader is covered, including `BufMessageReader` and any future one. Defaults are 16 MiB for a header and 1 GiB for a body, both far above anything the encoder emits; `MessageLimits::UNLIMITED` restores the old behaviour for trusted input. `with_limits` constructors are added to the three readers. This mirrors the existing `MAX_METADATA_SEGMENTS` check in `footer/postscript.rs`, which bounds a declared count before allocating. Signed-off-by: "Claude" <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
An IPC message declares the length of its flatbuffer header and of its body before either has been received, and
PollRead::NeedMoreasks the caller to size its read buffer from those declarations:Neither declaration was bounded.
msg_lengthis au32read straight off the wire (decoder.rs,State::Length) andbody_sizeis au64from the message header (State::Reading), so a short frame declaring a large body made bothSyncMessageReaderandAsyncMessageReaderresize their buffers to that length before a single body byte had arrived. Resource use was decoupled from bytes actually delivered, which matters wherever a Vortex IPC stream can be fed by something other than a Vortex writer.The repository already has the pattern this follows:
footer/postscript.rschecks a declared metadata count againstMAX_METADATA_SEGMENTSbefore using it to size aVec/HashSet. This applies the same shape to the IPC decoder.What changes are included in this PR?
MessageLimits— aCopystruct withmax_header_size/max_body_size, aDefault, and anUNLIMITEDconstant.MessageDecoder::read_next, at both declaration sites, placed before the correspondingNeedMorereturn so no caller ever sizes a buffer from a rejected declaration.resizecall sites. The decoder is what turns untrusted bytes into a size request, so bounding it there covers every reader — includingBufMessageReader, which errors onNeedMoretoday, and any reader added later — rather than requiring each to remember the check.with_limitsconstructors onSyncMessageReader,AsyncMessageReader, andBufMessageReader.decoder.rs: an oversized header declaration and an oversized body declaration are rejected; a body under the limit still returnsNeedMorenormally (so the check does not short-circuit the ordinary incomplete-read path); and the limits are configurable in both directions.Choice of defaults
16 MiB header / 1 GiB body. Headers carry only flatbuffer metadata — encoding ids, row count, dtype — which stays in the kilobytes even for wide schemas. Bodies carry one message's serialized array buffers, which writers chunk far below 1 GiB. Both are well clear of anything
MessageEncoderproduces, so this should reject only declarations that could not have come from a well-formed stream. Happy to move either number if you have a workload closer to the line than I'd assume.What APIs are changed? Are there any user-facing changes?
Additive, no breaking changes:
MessageLimitsexported fromvortex_ipc::messages.MessageDecoder::new(limits)andMessageDecoder::limits().MessageDecoder::default()still exists and now carries the default limits.with_limitsconstructors on the three readers; existingnewconstructors are unchanged and delegate to the defaults.The one behavioural change: a stream declaring a header above 16 MiB or a body above 1 GiB now returns an error where it previously attempted the allocation.
MessageLimits::UNLIMITEDrestores the prior behaviour for trusted local input.Related sites not changed here
I looked for the same shape elsewhere — a length read from the stream driving an allocation before the corresponding bytes are known to exist — and found these, all left alone to keep this reviewable:
encodings/zstd/src/array.rs(~L1055) —with_capacity_alignedfrom a summed declared uncompressed size. Note the sibling path inzstd_buffers.rs(~L264) does callvalidate_frame_content_sizeagainst the real zstd frame header first, so the guarded version already exists next door.encodings/pco/src/array.rs(~L611, ~L630) —with_capacity/reservefrom metadata value counts.encodings/fsst/src/canonical.rs(~L56) — capacity from the summeduncompressed_lengthschild.Two adjacent things I noticed and did not touch:
zstdandpcobothset_lenbefore filling and then hand out&mut [T]over the uninitialised tail. The FSST path already does this correctly withspare_capacity_mut.SyncMessageReader::nextresizes tonbytesand then callsRead::readonce, which may fill only part of the buffer — but the nextread_nextseesremaining() == nbytesand decodes the zero-padded tail as if it were data.AsyncMessageReaderhandles this properly with itsFillingstate. This looks like a real partial-read bug for non-file readers, separate from this change; happy to open an issue or fold in a fix if you'd prefer.Checks
cargo test -p vortex-ipc— 10 passed, including the 4 new tests and the existing round-trip / chunked / single-byte-chunk partial-read tests.cargo clippy -p vortex-ipc --all-targets --all-features— clean.cargo +nightly fmt— applied.Not run: workspace-wide build/clippy/tests. The change is confined to
vortex-ipcand is additive, but the new public type is exported, so a wider check before merge is worthwhile.Generated by Claude Code