[tomasilluminati]

Second version after the review. The first version added a public guard
class and its own exception; this one keeps the agreed shape: an
opt-in limit on the decompressed size, configured on
CompressorStreamFactory, composed over Commons IO BoundedInputStream.

What changed

• CompressorStreamFactory.builder() with setMaxDecompressedSize(long),
  setMemoryLimitKiB(int) and setDecompressUntilEOF(boolean); the existing
  constructors are untouched.
• A package-private BoundedCompressorInputStream wraps the stream only when
  a limit is set. It uses BoundedInputStream.setMaxCount with a throwing
  setOnMaxCount. The callback reads one byte from the decompressor first, so
  a payload of exactly the limit passes and only real excess throws; after
  the first violation every read throws again.
• CompressorInputStream implements InputStreamStatistics; the base
  getCompressedCount() returns -1 and declares throws IOException like
  the interface, so an external subclass that already declares it still
  compiles (JLS 8.4.8.3).
• Limit hits throw CompressorException, the exception the compressors
  package already uses.
• Two changes.xml entries.

Boundary
setMaxDecompressedSize(N): at most N bytes are returned; N bytes exactly
reads to EOF; N + 1 throws after exactly N bytes were returned. This matches
MemoryLimitException.check (request > max) and lets a caller use a known
size as the limit, which is the follow-up Piotr described for ZIP and 7z
entries.

Out of scope, as discussed
Ratio guard; inheriting from BoundedInputStream (binary break on
getCount(), and the input versus output counting question is still open);
the FramedLZ4/FramedSnappy getUncompressedCount() inaccuracy, which
deserves its own ticket.

Tests
86 new tests in three classes: the wrapper (boundary at N, N + 1, single
byte reads, skip, zero-length reads, mark/reset, close, concatenated gzip
members, a corrupt trailer at exactly N, a randomised invariant over 500
size/limit pairs), the factory (builder defaults, memory limit and size
limit together, every create path, ten writable formats plus Brotli, Z and
Pack200 fixtures), and the base class (interface, default, subclasses that
declare the checked exception).

Gates (all on the final tree, evidence in the vault note evidence/2026-09-20-phase5-gates.md)

• Bare mvn default goal: green on macOS/JDK 21 and, in Docker as the non-root
  host uid, on Ubuntu 24.04/Temurin 21, Alpine/Temurin 21, Amazon Linux/Corretto 21,
  Ubuntu/Temurin 8 and Ubuntu/Temurin 17: 4867 tests, 0 failures, 0 Checkstyle
  violations, PMD and Javadoc clean, japicmp compatible (interface and method
  added on CompressorInputStream, builder() and Builder added on the factory).
  One linux/amd64 emulated pass of the compressors tests, and a native x86_64 run
  on Arch Linux with OpenJDK 26 (i9-14900HX): full default goal, 4867 tests, 0 failures.
• SpotBugs 4.10.4 + FindSecBugs 1.14.0 at effort Max: identical findings to
  untouched master (607, 16 SECURITY, none in the changed classes).
• JaCoCo: BoundedCompressorInputStream and CompressorStreamFactory.Builder 100 %
  on every counter; every line of the change covered except the unreachable
  IOException catch in the factory hook.
• PIT: 173 mutants, 150 killed, test strength 96 %, no surviving or uncovered
  mutant in the changed code.
• Source compatibility: four external subclass shapes compile against the built
  jar. pom.xml untouched, so no dependency delta.

[tomasilluminati]

@ppkarwasz COMPRESS-726 PR Opened

[garydgregory]

Hi All,
I have some initial thoughts on this line of work:

• Let's loose the whole "bomb" FUD terminology. I might just want to enforce limits. I should be able to do this in one place that also happens to catch possible attacks if an app decides to accept untrusted input. The goal is to enforce a limit, regardless of the input's intent. I have a door on my house to avoid people, animals, weather, and so on from coming in, I don't have a "thief-guard".
• Use a builder for construction with a private ctor(Builder), otherwise, this will be another class that will get one constructor after another over time.
• Why no pull up the feature in the super class or move it to IO's BoundedInputStream? The code already uses a BoundedInputStream after all.

[tomasilluminati]

@garydgregory
Thanks, this is really helpful, and honestly it points at a simpler shape than what I pushed.

If all we want is a decompressed-size limit, IO's BoundedInputStream already does it (setMaxCount plus an onMaxCount that throws). So we don't really need a compress-specific class or a new exception. I'm happy to drop BombGuardCompressorInputStream and DecompressionBombException and just have CompressorStreamFactory wrap the stream in a BoundedInputStream when a max size is set. That's the single place you're describing, it reuses IO, and the bomb wording goes away. The builder point sorts itself out too, since there's no class left to build.

The only bit that doesn't fit there is the compression-ratio check, since BoundedInputStream has no idea about ratios and it needs getCompressedCount() from the compressor side. So really the only open question is scope. Just the size limit, or the ratio guard too?

• Size only, I drop the class and the exception and wire BoundedInputStream into the factory. Smallest change.
• Ratio too, I keep it but move it onto CompressorInputStream where both counts already live, still exposed through the factory.

My hunch from your note is size-only, but just say the word and I'll redo it that way. Either way the InputStreamStatistics change stays, the compressed count is handy on its own.

Thanks again for the steer.

[garydgregory]

@tomasilluminati
My initial reaction to the ratio check feature is that it's nice but I doubt folks will know how to use it properly without shooting themselves in the foot. "Our app blew up when we allowed users to import files of type foo but everything was fine with bar files" kind of thing. That said, there is a way to implement a ratio check I think using org.apache.commons.io.input.ProxyInputStream.AbstractBuilder.setAfterRead(IOIntConsumer) where you'd do the check in the consumer (BoundedInputStream extends ProxyInputStream).
This one can simmer... ;) maybe @ppkarwasz has some thoughts.

[tomasilluminati]

@garydgregory
Yeah, I'm with you on the footgun. The ratio is genuinely hard to tune and easy to get wrong, exactly the "works for bar, blows up on foo" trap, so I'd rather not hand people a sharp tool they can't aim.

Nice pointer on setAfterRead, hadn't thought of it. That actually means neither piece needs a new class. The size cap is setMaxCount plus a throwing onMaxCount, and if the ratio ever comes back it can ride along as a setAfterRead consumer on the same BoundedInputStream.

So my lean is to keep it minimal, wire an absolute decompressed-size limit into CompressorStreamFactory via BoundedInputStream, drop the bomb class and the exception, and leave the ratio out for now. We can always document it as a recipe later.

Happy to let it simmer and hear what @ppkarwasz thinks before I change anything.

[tomasilluminati]

@garydgregory , I've got the minimal version ready, absolute decompressed-size limit composed over IO's BoundedInputStream, dropping the extra class and the exception. One note, since the cap is on the real decompressed output, it already covers the faked-payload-size case Piotr raised (it ignores any declared size). @ppkarwasz, given that, do you still want the mean-ratio guard and an on-by-default limit? The ratio fires earlier and needs no tuned number, so I'm happy to fold it back as a second, opt-in check if you want it. Otherwise I'll push the size-only version.

[ppkarwasz]

My initial reaction to the ratio check feature is that it's nice but I doubt folks will know how to use it properly without shooting themselves in the foot. "Our app blew up when we allowed users to import files of type foo but everything was fine with bar files" kind of thing.

As with most features, we probably need to provide sensible defaults, so users don't need to modify them. AI gives the following usual compression rates:

• Binaries: 2-3×
• Text/source code: 3-5×
• XML/JSON: 5-15×
• Log files: 10-30×

Higher compression rates are impossible, unless you compress zeros, so 100× looks like a safe default. When I chose the default ratio for apache/mina#52, I tried to copy what HTTP Server does: mod_deflate, has these knobs:

• DeflateInflateRatioLimit defaults to 200,
• Rather than a grace size, mod_deflate only rejects a stream once the cumulative ratio stays above the limit for DeflateInflateRatioBurst + 1 (default 4) consecutive output buffers. With the default 8 KiB buffer, that corresponds to roughly 32 KiB of decompressed output.

[ppkarwasz]

@ppkarwasz, given that, do you still want the mean-ratio guard and an on-by-default limit?

I guess, we can deliver a mean-ratio guard in a separate wrapper: only a limited number of users need it. Basically only if the compressed stream comes primarily from untrusted sources (like in a server), users might want to add a mean-ratio check as a fail-fast mechanism to prevent a DoS.

However, I think it would be nice if every CompressorInputStream inherited from BoundedInputStream: for archive formats that contain compressed data (ZIP, 7z), we usually have a (declared) decompressed size, so we could set that size as limit and prevent corrupted archives to fill the disk. Malicious archives, of course, would need the user to set a hard maximum decompressed size.

[garydgregory]

To make it clear, I'll move this PR draft since it is not ready as there are a few aspects still being discussed.

[tomasilluminati]

@ppkarwasz thanks, that lines up well.

A separate opt-in wrapper for the mean-ratio guard sounds right, it keeps the cost at zero for the common case and puts the fail-fast check where untrusted-source users actually want it. Happy to add it that way, following the mod_deflate shape you described (a burst tolerance in output buffers rather than a byte grace), with 100x as the default once it is opt-in.

On making every CompressorInputStream inherit from BoundedInputStream to use the declared ZIP/7z size: appealing for catching corrupt archives, but it is a much bigger change than this PR, it touches the base type every format extends and the japicmp surface, and the declared size lives at the archive layer rather than the codec layer, so wiring it cleanly is its own design. I'd keep it out of 726 and open a separate ticket if you want to pursue it.

So where I think this lands: absolute decompressed-size cap as the core here (composed over IO's BoundedInputStream), the mean-ratio guard as a separate opt-in wrapper with a 100x default, and the inherit-from-BoundedInputStream idea as its own follow-up. Does that match what you and @garydgregory have in mind before I rework the PR?

[garydgregory]

Giving defaults like:

• Binaries: 2-3×
• Text/source code: 3-5×
• XML/JSON: 5-15×
• Log files: 10-30×

is why this should NOT be added as a feature to this class. It's based on guesses at best and will certainly shoot someone in the foot or a more useful body part.

[garydgregory]

@ppkarwasz, given that, do you still want the mean-ratio guard and an on-by-default limit?

I guess, we can deliver a mean-ratio guard in a separate wrapper: only a limited number of users need it. Basically only if the compressed stream comes primarily from untrusted sources (like in a server), users might want to add a mean-ratio check as a fail-fast mechanism to prevent a DoS.

However, I think it would be nice if every CompressorInputStream inherited from BoundedInputStream: for archive formats that contain compressed data (ZIP, 7z), we usually have a (declared) decompressed size, so we could set that size as limit and prevent corrupted archives to fill the disk. Malicious archives, of course, would need the user to set a hard maximum decompressed size.

I think it would be nice if every CompressorInputStream inherited from BoundedInputStream:

I think I like that idea.

What we are saying today is: If you want to bound decompression, you must compose your CompressorInputStream with a BoundedInputStream yourself and configure it.

Instead, what we would be saying is: Bounding decompression is so important to all possible call sites, that we are no longer offering composition as an option, we are now hard-coding the feature by inheriting it and making it available to all call sites, all the time. You still need to set a boundary in you call site.

Doing so would require breaking binary compatibility because the deprecated method CompressorInputStream.getCount() returns an int and BoundedInputStream.getCount() returns a long. Food for thought...

[tomasilluminati]

@ppkarwasz, given that, do you still want the mean-ratio guard and an on-by-default limit?

I guess, we can deliver a mean-ratio guard in a separate wrapper: only a limited number of users need it. Basically only if the compressed stream comes primarily from untrusted sources (like in a server), users might want to add a mean-ratio check as a fail-fast mechanism to prevent a DoS.
However, I think it would be nice if every CompressorInputStream inherited from BoundedInputStream: for archive formats that contain compressed data (ZIP, 7z), we usually have a (declared) decompressed size, so we could set that size as limit and prevent corrupted archives to fill the disk. Malicious archives, of course, would need the user to set a hard maximum decompressed size.

I think it would be nice if every CompressorInputStream inherited from BoundedInputStream:

I think I like that idea.

What we are saying today is: If you want to bound decompression, you must compose your CompressorInputStream with a BoundedInputStream yourself and configure it.

Instead, what we would be saying is: Bounding decompression is so important to all possible call sites, that we are no longer offering composition as an option, we are now hard-coding the feature by inheriting it and making it available to all call sites, all the time. You still need to set a boundary in you call site.

Doing so would require breaking binary compatibility because the deprecated method CompressorInputStream.getCount() returns an int and BoundedInputStream.getCount() returns a long. Food for thought…

I like the intent, one honest catch beyond the getCount int/long break you flagged. BoundedInputStream bounds the bytes it reads from its wrapped stream, which for a decompressor is the compressed input side, whereas bomb protection needs to bound the decompressed output. So the inheritance only does the right thing if every format routes its output through the proxy, which is the big part of the change. Before anyone builds it, worth pinning down what the inherited bound counts, decompressed output or the compressed input it proxies. Happy to prototype whichever way you and @ppkarwasz settle on.

[tomasilluminati]

@garydgregory @ppkarwasz
Pushed the second version. It is the shape we settled on: size limit only, configured through CompressorStreamFactory.builder().setMaxDecompressedSize(long), composed over BoundedInputStream with setMaxCount and a throwing setOnMaxCount. The guard class and its exception are gone; the limit throws
CompressorException. The factory got a builder rather than a fourth constructor, following the review; the existing constructors are untouched.

One detail worth a look is that the callback reads a single byte from the decompressor before throwing, so a stream of exactly the limit reads to EOF and only real excess fails. Without that, IOUtils.toByteArray on a legitimate file of
exactly N bytes would throw, and Piotr's idea of using a declared entry size as the limit would not work.

CompressorInputStream now implements InputStreamStatistics; the base getCompressedCount() returns -1 and keeps the interface's throws IOException so subclasses that already declare it keep compiling.

Rebased on master; the branch name still says bomb-guard because the PR is bound to it.