Skip to content

Add resource limits for archive extensions and sparse maps - #118

Closed
cxymds wants to merge 7 commits into
astral-sh:mainfrom
cxymds:fix/archive-extension-limits
Closed

Add resource limits for archive extensions and sparse maps#118
cxymds wants to merge 7 commits into
astral-sh:mainfrom
cxymds:fix/archive-extension-limits

Conversation

@cxymds

@cxymds cxymds commented Aug 30, 2026

Copy link
Copy Markdown

Motivation

Server-side consumers may parse TAR archives supplied by untrusted clients. They can bound logical entries after Archive::entries() yields them, but tokio-tar consumes GNU long-name and long-link payloads, PAX extensions, and GNU sparse continuation data before the corresponding logical entry is exposed. At that point an application-level check is too late to prevent extension buffering, sparse-map growth, or excessive continuation reads. Counting logical entries also does not bound the physical metadata records consumed by the parser.

This was encountered while hardening RustFS's MinIO-compatible Snowball ingestion path in rustfs/rustfs#6942. The limits belong at the parser boundary so a caller can reject an archive before the associated allocation or out-of-budget read. They remain opt-in and default to unlimited, preserving behavior for existing callers. The cancellation-safety and stream-fusing changes ensure that an error or cancelled next() future cannot leave extension parsing in a partially advanced state.

The problem statement and compatibility goals are tracked in #119.

Summary

  • add optional per-entry, cumulative, physical-entry, GNU sparse map, and sparse continuation resource limits to ArchiveBuilder
  • preserve GNU sparse parser state across Pending and cancelled next() futures
  • fuse raw and logical entry streams after parser errors so callers cannot continue past an invalid extension
  • reject over-limit metadata before allocation or out-of-budget continuation reads, with exact-boundary and malformed-input coverage

All new limits default to unlimited, preserving existing behavior unless a caller opts in.

Testing

  • cargo fmt --all -- --check
  • UV_NO_CONFIG=1 uvx --from 'astral-dev-toolchain-cargo-hack>=0.6.45' cargo-hack hack check --all --ignore-private --each-feature --no-dev-deps
  • UV_NO_CONFIG=1 uvx --from 'astral-dev-toolchain-cargo-shear>=1.13.4' cargo-shear shear
  • cargo check --all --all-targets --all-features
  • cargo test
  • cargo test --no-default-features
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
  • cargo +1.83.0 test

Closes #119.

@zanieb

zanieb commented Aug 31, 2026

Copy link
Copy Markdown
Member

Please describe your motivation?

@cxymds

cxymds commented Aug 31, 2026

Copy link
Copy Markdown
Author

Thanks for asking. I have added a Motivation section and opened #119 to separate the problem statement from the implementation.

This originated in RustFS's Snowball ingestion path, which streams TAR archives supplied by untrusted network clients. RustFS can enforce logical member limits after Archive::entries() yields an entry, but tokio-tar has already consumed GNU long-name/link data, PAX extensions, and GNU sparse continuation metadata before that boundary. A caller-side check is therefore too late to prevent the associated buffering, sparse-map growth, or continuation reads, and a logical entry count does not cover those physical metadata records.

The proposed limits are opt-in and remain unlimited by default. The downstream integration is rustfs/rustfs#6942, and the full motivation and compatibility goals are in #119.

@cxymds

cxymds commented Sep 2, 2026

Copy link
Copy Markdown
Author

Hi @zanieb , could you please review this PR when you have a chance? If everything looks good, I'd appreciate it if you could merge it. Thanks!

@woodruffw

Copy link
Copy Markdown
Member

I'm pretty hesitant to land any version of this in astral-tokio-tar: the codebase isn't really set up for it, and my experience adding similar limits to tar-codec is that there are a lot of subtleties that are difficult to catch. That combined with the breakage risk to uv itself.

@cxymds Have you looked at tar-codec to see if it satisfies your use case? It's a much more constrained parser (no sparse support for example), but it was designed from the ground up to express these kinds of limits.

@cxymds

cxymds commented Sep 4, 2026

Copy link
Copy Markdown
Author

Thanks for the suggestion. I took a closer look at both tar-codec and tar-framing. The state machine and resource-limiting design in tar-framing do seem closer to what we need, but it is not yet a drop-in replacement for RustFS.

The main blockers are that MinIO Snowball archives may end at a complete member boundary without the standard two zero blocks, and that we need access to raw per-member PAX metadata, as well as limits on cumulative extension data and the number of physical headers. The high-level TarArchive API does not expose all of this, so an integration would likely need to use the lower-level tar-framing interface.

Given the potential regression risk to uv, I agree that we should not pursue #118 in its current form.

Would you be open to smaller, focused contributions to tar-framing, such as an opt-in policy for accepting EOF at a complete member boundary and the missing cumulative resource limits? If so, I can first validate the approach using real minio-go Snowball fixtures.

@woodruffw

Copy link
Copy Markdown
Member

It sounds like you need a lot of non-standard and differential-prone features. I'd suggest rethinking your approach here: astral-tokio-tar is only going to become more strict over time (until we deprecate it entirely), and handling pax records without managing their precedence (which tar-codec does automatically) is a security footgun.

In terms of wanting to bypass the two block terminator: tar-codec exposes a member iterator, so you can just prematurely end your iteration rather than letting it fail when the EOF is invalid. I would not recommend that (and it's an error that other libraries let you do it by default), but there's no technical reason why you can't do it.

@cxymds

cxymds commented Sep 4, 2026

Copy link
Copy Markdown
Author

Thanks, that clarification is helpful. I also phrased the PAX requirement too broadly: RustFS does not need to interpret arbitrary raw PAX records independently. It needs the effective minio.* vendor values after normal PAX precedence has been applied. I agree that this should be built on tar-codec’s precedence-managed state rather than reimplemented downstream.
For footerless Snowball streams, we’ll investigate whether the request framing provides an authoritative application-level point at which to stop the member iterator. If it does not, we will not weaken tar-codec’s end-marker validation.
Given the direction for astral-tokio-tar, I’ll close this PR and prototype the RustFS integration on tar-codec/tar-framing first. Thanks for the guidance.

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.

Add opt-in limits for archive extension metadata and sparse maps

3 participants