Skip to content

Kill the naked int/long size: ZstdByteSize value type#98

Merged
dfa1 merged 1 commit into
mainfrom
feature/zstd-byte-size
Jul 24, 2026
Merged

Kill the naked int/long size: ZstdByteSize value type#98
dfa1 merged 1 commit into
mainfrom
feature/zstd-byte-size

Conversation

@dfa1

@dfa1 dfa1 commented Jul 22, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds ZstdByteSize, a record wrapping a byte size/count as a validated non-negative long, with toIntExact() for callers that must narrow to an int because the result ends up in a byte[] (throws ArithmeticException past Integer.MAX_VALUE, matching Math.toIntExact).
  • Replaces every naked int/long size parameter with it: Zstd.decompress, Zstd.compressBound, Zstd.estimateCompressDictSize/estimateDecompressDictSize, ZstdDictionary.train/trainCover/trainFastCover/finalizeFrom, ZstdOutputStream.withPledgedSize.
  • Centralizes interpretation of zstd's raw frame-content-size results as ZstdByteSize factories rather than scattered sentinel checks:
    • ZstdByteSize.fromFrameContentSize(long) turns the ZSTD_getFrameContentSize/ZSTD_findDecompressedSize/ZSTD_decompressBound sentinels — and any other negative reading (an unsigned value ≥ 2^63) — into a ZstdException instead of a bogus negative size or the wrong exception type. Used by Zstd.decompress(byte[]), Zstd.decompressedSize(MemorySegment), and ZstdFrame.decompressedSize/decompressedBound, which now return ZstdByteSize instead of unwrapping immediately back to long.
    • ZstdByteSize.fromFrameHeaderContentSize(long) backs ZstdFrameHeader.contentSize(), now Optional<ZstdByteSize> instead of OptionalLong.
    • The CONTENTSIZE_UNKNOWN/CONTENTSIZE_ERROR sentinel constants move from Zstd to become a private implementation detail of ZstdByteSize.
  • ZstdFrame's other size-returning methods (compressedSize, decompressionMargin, headerSize) are unaffected — they never construct a ZstdByteSize, no sentinel semantics involved.
  • Breaking change (pre-1.0, no compatibility shims). CHANGELOG updated under [Unreleased].

Closes #96.

Design notes

  • One long-backed type, not two split by width — narrowing happens at the four array-bounded call sites via toIntExact(); native/streaming totals pass long straight through. See discussion on Consider a size value type for byte-size parameters #96.
  • A real regression was caught and fixed during review: a frame declaring a content size with the sign bit set (unsigned ≥ 2^63) was falling through both named sentinel checks and throwing the wrong exception type (IllegalArgumentException instead of the documented ZstdException). Covered by a new regression test (ZstdTest.UntrustedInput.rejectsAFrameDeclaringAContentSizeWithTheSignBitSet).

Test plan

  • ./mvnw -pl zstd,integration-tests,benchmark -am clean verify — build success, all unit + integration tests pass
  • ./mvnw -pl zstd validate — checkstyle clean
  • ./mvnw -pl zstd javadoc:javadoc — zero output (failOnError/failOnWarnings)
  • Added ZstdByteSizeTest covering construction, negative rejection, toIntExact() in-range/overflow
  • Manually reproduced and verified the sign-bit-set regression fix against a hand-crafted frame
  • Multiple independent review passes focused on native-call boundary correctness, exception-type consistency, and Javadoc/call-site accuracy — no outstanding issues

🤖 Generated with Claude Code

/// [#toIntExact()] rather than silently truncating.
///
/// @param value the byte size or count, non-negative
public record ZstdByteSize(long value) {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe it should be called ZstdSize?

Comment thread README.md Outdated

List<byte[]> samples = ...; // representative records
ZstdDictionary dict = ZstdDictionary.train(samples, 8 * 1024);
ZstdDictionary dict = ZstdDictionary.train(samples, new ZstdByteSize(8 * 1024));

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should also expose a better way to build 8K or 1M constants? this could be useful for tests internally but also for end users to define the buffer size for copies, etc

@dfa1
dfa1 force-pushed the feature/zstd-byte-size branch from 3de81d6 to b4773df Compare July 22, 2026 06:14
@dfa1

dfa1 commented Jul 22, 2026

Copy link
Copy Markdown
Owner Author

Re: ZstdSize naming — kept ZstdByteSize. It matches the house convention of descriptive multi-word names (ZstdCompressionLevel, ZstdDictionaryId, ZstdFrameHeader) — "Size" alone felt more ambiguous (could read as a count, a dimension) where "ByteSize" says exactly what's inside.

Re: KB/MB constants — added ZstdByteSize.ofKiB(long) / ofMiB(long) (using Math.multiplyExact so an overflow fails fast instead of wrapping to a bogus size), and swapped the naked new ZstdByteSize(16 * 1024)-style literals across README/docs/tests/smoke for it. Pushed as b4773df.

Introduces ZstdByteSize, a record wrapping a byte size/count as a validated
non-negative long, so that holding one guarantees validity instead of leaving
callers to wonder whether a raw long from the library still needs checking.
Provides toIntExact() for callers that must narrow to an int because the
result ends up in a byte[] (throws ArithmeticException past
Integer.MAX_VALUE, matching Math.toIntExact), and ofKiB(long)/ofMiB(long)
factories for common buffer sizes instead of spelling out arithmetic like
`new ZstdByteSize(16 * 1024)`.

Replaces every naked int/long size parameter, and every long return value
that represents a byte count, across the public API: Zstd.compressBound,
Zstd.estimateCompressContextSize/estimateDecompressContextSize/
estimateCompressDictSize/estimateDecompressDictSize,
Zstd.decompressedSize(MemorySegment), ZstdDictionary.train/trainCover/
trainFastCover/finalizeFrom, ZstdOutputStream.withPledgedSize,
ZstdFrame.decompressedSize/decompressedBound, and sizeOf() on
ZstdCompressContext/ZstdDecompressContext/ZstdCompressStream/
ZstdDecompressStream/ZstdCompressDictionary/ZstdDecompressDictionary.

Centralizes interpretation of zstd's raw frame-content-size results as
ZstdByteSize factories rather than scattered sentinel checks:
- ZstdByteSize.fromFrameContentSize(long) turns the
  ZSTD_getFrameContentSize/ZSTD_findDecompressedSize/ZSTD_decompressBound
  sentinels (and any other negative reading, i.e. an unsigned value >= 2^63)
  into a ZstdException instead of a bogus negative size or the wrong
  exception type.
- ZstdByteSize.fromFrameHeaderContentSize(long) backs
  ZstdFrameHeader.contentSize(), now Optional<ZstdByteSize> instead of
  OptionalLong. It treats any negative reading as absent (not just the
  sentinel), since ZSTD_getFrameHeader never validates the declared size's
  magnitude, so a hostile header can wrap unsigned >= 2^63 into a negative
  long the same way a raw content-size read can.
The CONTENTSIZE_UNKNOWN/CONTENTSIZE_ERROR sentinel constants move from Zstd
to become a private implementation detail of ZstdByteSize.

Zstd.decompress(byte[]) and decompress(byte[], ZstdByteSize) both route
through a shared decompressInternal(byte[], int), so a caller-supplied bound
too large for a byte[] fails with the documented ZstdException instead of a
bare ArithmeticException, matching the frame-header path. ZstdDictionary's
train/trainCover/trainFastCover/finalizeFrom do the same via a shared
toBufferLength helper, and their public parameter names are now consistent
(maxDictBytes throughout, no -Bound suffix on a subset of the overloads).

Breaking change; the library is pre-1.0 so no compatibility shims are added.

Closes #96.
@dfa1
dfa1 force-pushed the feature/zstd-byte-size branch from b4773df to 0e12130 Compare July 23, 2026 19:09
@dfa1

dfa1 commented Jul 23, 2026

Copy link
Copy Markdown
Owner Author

Ran an independent review over the whole diff and applied the findings that survived verification:

  1. Real bug, confirmed empirically: ZstdByteSize.fromFrameHeaderContentSize only guarded the -1 "not stored" sentinel, so a hostile frame header declaring a content size ≥ 2^63 (sign bit set) parsed cleanly through ZstdFrame.header() and then ZstdFrameHeader.contentSize() leaked an IllegalArgumentException instead of the documented "empty" contract — the same attack class this PR already fixed on the decompress path, just through a different entry point. Fixed by treating any negative reading as absent, with a new regression test (ZstdFrameTest.Header.contentSizeIsEmptyForAContentSizeWithTheSignBitSet).
  2. Undocumented ArithmeticException: decompress(byte[], ZstdByteSize) and ZstdDictionary's train/finalize family let a bare ArithmeticException escape for an in-range-for-the-type-but-too-big-for-a-byte[] bound, while their @throws only documented ZstdException. Fixed by routing both decompress overloads through a shared decompressInternal, and the dictionary methods through a shared toBufferLength helper — both now wrap the overflow into the documented ZstdException.
  3. Naming inconsistency: train/finalizeFrom had renamed their parameter to maxDictBytesBound while trainCover/trainFastCover kept maxDictBytes — same concept, two names across sibling methods. Reverted to maxDictBytes everywhere, narrowing into a distinctly-named local instead of shadowing.
  4. Two leftover new ZstdByteSize(8 * 1024) literals in the integration tests that missed the earlier ofKiB sweep.

Full build/tests/checkstyle/javadoc all green. Pushed as 0e12130.

@dfa1
dfa1 merged commit f0581ca into main Jul 24, 2026
1 check passed
@dfa1
dfa1 deleted the feature/zstd-byte-size branch July 24, 2026 05:55
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.

Consider a size value type for byte-size parameters

1 participant