Bun.gzipSync/deflateSync: throw invalid-argument for out-of-range libdeflate level - #34114
Conversation
…deflate level libdeflate_alloc_compressor returns NULL both on OOM and when the level is outside [0, 12]. Bun.gzipSync/deflateSync mapped every NULL to throw_out_of_memory, so passing level:-1 or level:13 with library:'libdeflate' threw 'Out of memory' instead of an argument error. Range-check the level before calling the allocator and throw a clear 'Compression level must be between 0 and 12 for libdeflate' error, matching the zstd path.
|
Warning Review limit reached
Next review available in: 10 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 12:43 AM PT - Jul 14th, 2026
❌ @robobun, your commit 3306df1 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 34114That installs a local version of the PR into your bun-34114 --bun |
There was a problem hiding this comment.
LGTM — targeted fix that turns a misleading "Out of memory" into a proper invalid-argument error for out-of-range libdeflate levels.
What was reviewed:
- Range check in
gzip_or_deflate_sync:0..=12inclusive matches libdeflate's documentedlibdeflate_alloc_compressorcontract; defaultlevel.unwrap_or(6)preserved. - New
MIN/MAX_COMPRESSION_LEVELconstants and doc comments inlibdeflate.rsare additive-only; no behavior change toOwnedCompressor::new. - Tests cover both
gzipSync/deflateSync, boundary levels 0/12 succeed, -1/13 throw with the specific message and not "memory". - Checked sibling
OwnedCompressor::newcall sites (compress_body.rs,audit_command.rs,Archive.rs) — all use fixed or pre-validated levels, so the bug class doesn't reproduce there.
Extended reasoning...
Overview
This PR fixes Bun.gzipSync / Bun.deflateSync with { library: "libdeflate" } to throw a descriptive invalid-argument error when level is outside libdeflate's [0, 12] range, instead of a misleading "Out of memory". Three files touched:
src/libdeflate_sys/libdeflate.rs: addspub const MIN_COMPRESSION_LEVEL = 0/MAX_COMPRESSION_LEVEL = 12and updates doc comments to note thatlibdeflate_alloc_compressorreturns NULL for out-of-range levels as well as OOM. No behavioral change.src/runtime/api/BunObject.rs(JSZlib::gzip_or_deflate_sync,Library::Libdeflatearm): hoistslevel.unwrap_or(6)to a local, range-checks it against the new constants viaRangeInclusive::contains, and throwsthrow_invalid_argumentswith a message naming the valid range. The subsequentOwnedCompressor::new(level)→throw_out_of_memory()fallback is unchanged and now only reachable on real OOM.test/js/node/zlib/zlib.test.js: newdescribeblock exercising bothgzipSyncanddeflateSync— levels-2, -1, 13, 100throw with "Compression level must be between 0 and 12" and not "memory"; levels0, 1, 6, 9, 12compress and round-trip through the matching decompressor.
Security risks
None. This is input validation on a scalar integer that produces a better error message. No new allocation, no new FFI surface, no user-controlled data reaching a new code path — the range check runs strictly before the existing allocator call.
Level of scrutiny
Low. The change is a ~10-line validate-before-allocate guard plus two named constants and doc-comment updates. The constants (0 and 12) match libdeflate's documented API contract. The error path uses the existing throw_invalid_arguments helper consistent with neighboring code. The default level (6) is unchanged. The inclusive range check is correct at both boundaries, and the tests explicitly exercise 0, 12, -1, and 13.
Other factors
- Sibling call sites: I grepped for all
OwnedCompressor::newcallers.audit_command.rsand the cached-handle path incompress_body.rsuse hardcoded level 6;Archive.rsmapsNonetoCompressError::GzipInitFailed(not OOM); the custom-level path incompress_body.rswas flagged by a finder and refuted by verifiers as not sharing this bug. None of these are the user-facingBun.gzipSync/deflateSyncpath this PR targets. - Test quality: Boundary coverage is good (0 and 12 succeed, -1 and 13 fail), both entry points (
gzipSync/deflateSync) are covered, and the assertion checks the specific message substring rather than a baretoThrow(). The round-trip test confirms in-range levels still work end-to-end. - No CODEOWNERS entries match the touched paths.
- No outstanding review comments — timeline contains only bot noise (CodeRabbit rate-limit notice, robobun build link).
|
CI: Remaining failures are unrelated to this diff:
Ready for review. |
What does this PR do?
Bun.gzipSync/Bun.deflateSyncwith{ library: "libdeflate" }threw"Out of memory"when given a compression level outside libdeflate's[0, 12]range, includinglevel: -1(zlib'sZ_DEFAULT_COMPRESSION, accepted by the same call without thelibraryoption).Cause
libdeflate_alloc_compressorreturnsNULLboth for real allocation failure and for an out-of-range level (documented invendor/libdeflate/libdeflate.h).gzip_or_deflate_syncmapped everyNonefromOwnedCompressor::newtothrow_out_of_memory(), so an argument error surfaced as a fake OOM.Fix
Range-check the level against new
MIN_COMPRESSION_LEVEL/MAX_COMPRESSION_LEVELconstants inbun_libdeflate_sysbefore calling the allocator, and throw an invalid-argument error that names the valid range. The remainingNonefrom the allocator is now only reachable on real OOM.How did you verify your code works?
New tests in
test/js/node/zlib/zlib.test.jscover bothgzipSyncanddeflateSync:-2, -1, 13, 100throw with a message containing"Compression level must be between 0 and 12"and not"memory"0, 1, 6, 9, 12compress and round-trip through the matching decompressorFails on released bun with
"Out of memory", passes on this branch.[review] gate passed · iteration 0 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file