deps: replace cloudflare/zlib with zlib-ng 2.3.3 - #29433
Conversation
cloudflare/zlib has had no commits since Oct 2023. zlib-ng is actively maintained, ships in Node 24+ and Chromium, and provides runtime-dispatched SIMD (AVX-512/AVX2/NEON/SVE/RVV) for CRC32, adler32, hash, and chunk ops. Benchmark results on Xeon 8375C (Ice Lake, AVX-512), linux-x64 release: - gzipSync level=1: 1.8x-2.6x faster - gzipSync level=6: 1.1x-1.9x faster - deflate 123K level=6: 5.5x faster (373µs -> 68µs) - gunzipSync: 3-18% faster - createGzip stream: 1.13x-1.40x faster - fetch() gzip decode: parity (the #16100 blocker no longer reproduces) - Init overhead on 13B payload: +2µs (+40%), amortized away on >=4KB Build with WITH_INFLATE_STRICT=ON to harden inflateBack() against a heap OOB read on malicious raw-deflate with windowBits<15 (zlib-ng 340f2f6e moved this check behind a default-off ifdef; Bun doesn't call inflateBack but defense-in-depth is free here). Pinned to release tag 2.3.3 — two regressions on develop (172b8544 inverted CRC32 COPY guard, e5129cfe deflateBound UB) are NOT present at this commit. Build system: zlib-ng generates zlib.h at configure time into the build dir, so fetchDeps now resolves to the cross-dep's build outputs (not just source stamp) and libarchive's -I points at depBuildDir. Drops 4 vendor patches. Supersedes #16100, #8529. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughSwitches vendored zlib from the Cloudflare fork to zlib-ng (zlib-compat), updates build/config and dependency resolution, removes several zlib patches/workarounds, adds zlib/gzip benchmarks, adjusts Zig/Windows ABI types, and updates docs, licenses, and tests to the new zlib-ng revision. Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bench/snippets/fetch-gzip.mjs`:
- Around line 25-33: Add a one-time preflight before calling group("fetch + gzip
decode", ...) that fetches `${base}/gz` and `${base}/plain`, awaits res.text()
for each, and validates the decoded strings match the expected HTML payload (or
that both decode to the same canonical plain response); if they do not match,
throw or fail the run so the benches inside bench(...) do not record misleading
numbers. Use the existing symbols (group, bench, fetch, res.text, base, paths
/gz and /plain) to locate where to insert the preflight and perform the
validation.
In `@bench/snippets/zlib-comprehensive.mjs`:
- Around line 54-165: The benchmark currently discards outputs (especially in
streaming where drain() ignores bytes), so add a correctness warm-up that for
each corpus and for gzipped entries performs a one-time roundtrip check before
running timed groups: for sync cases call zlib.gzipSync/zlib.gunzipSync and
assert output length or hash equals the original; for async helpers (gzip,
gunzip) await a single call and assert equality; for streams
createGzip/createGunzip run pipeline once with Readable.from(chunks) and a drain
that verifies total bytes or computes a checksum, throwing or console.error on
mismatch to fail fast (refer to drain(), gzip, gunzip, zlib.gzipSync,
zlib.gunzipSync, pipeline, streamInputs/streamGzInputs and the bench group
names).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b7f787d2-6c75-448e-94ef-7a18f4392e65
📒 Files selected for processing (14)
CLAUDE.mdLICENSE.mdbench/snippets/fetch-gzip.mjsbench/snippets/zlib-comprehensive.mjsdocs/project/license.mdxpatches/zlib/CMakeLists.txt.patchpatches/zlib/deflate.h.patchpatches/zlib/ucm.cmake.patchscripts/build/bun.tsscripts/build/deps/libarchive.tsscripts/build/deps/zlib.tsscripts/build/patches/zlib/remove-machine-x64.patchscripts/build/source.tstest/js/node/process/process.test.js
💤 Files with no reviewable changes (4)
- patches/zlib/CMakeLists.txt.patch
- patches/zlib/ucm.cmake.patch
- scripts/build/patches/zlib/remove-machine-x64.patch
- patches/zlib/deflate.h.patch
| group("fetch + gzip decode", () => { | ||
| bench("11KB gzipped → text()", async () => { | ||
| const res = await fetch(`${base}/gz`); | ||
| await res.text(); | ||
| }); | ||
| bench("11KB plain → text()", async () => { | ||
| const res = await fetch(`${base}/plain`); | ||
| await res.text(); | ||
| }); |
There was a problem hiding this comment.
Validate the decoded payload before timing it.
await res.text() only proves the body was consumable. If fetch stops honoring Content-Encoding: gzip and returns compressed bytes, this benchmark still records a number for the wrong path. Add a one-time preflight that /gz and /plain both decode to the original HTML before group() starts.
Suggested guard
const base = `http://localhost:${server.port}`;
+
+const expectedText = html.toString();
+if ((await fetch(`${base}/gz`).then(res => res.text())) !== expectedText) {
+ throw new Error("gzip benchmark setup is not decoding the response body");
+}
+if ((await fetch(`${base}/plain`).then(res => res.text())) !== expectedText) {
+ throw new Error("plain benchmark setup returned an unexpected body");
+}
group("fetch + gzip decode", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bench/snippets/fetch-gzip.mjs` around lines 25 - 33, Add a one-time preflight
before calling group("fetch + gzip decode", ...) that fetches `${base}/gz` and
`${base}/plain`, awaits res.text() for each, and validates the decoded strings
match the expected HTML payload (or that both decode to the same canonical plain
response); if they do not match, throw or fail the run so the benches inside
bench(...) do not record misleading numbers. Use the existing symbols (group,
bench, fetch, res.text, base, paths /gz and /plain) to locate where to insert
the preflight and perform the validation.
| // Pre-compress for decompression benches | ||
| const gzipped = {}; | ||
| for (const [name, buf] of Object.entries(corpora)) { | ||
| gzipped[name] = zlib.gzipSync(buf, { level: 6 }); | ||
| } | ||
|
|
||
| // ─── Sync one-shot ─── | ||
| group("gzipSync level=1", () => { | ||
| for (const [name, buf] of Object.entries(corpora)) { | ||
| bench(name, () => zlib.gzipSync(buf, { level: 1 })); | ||
| } | ||
| }); | ||
|
|
||
| group("gzipSync level=6", () => { | ||
| for (const [name, buf] of Object.entries(corpora)) { | ||
| bench(name, () => zlib.gzipSync(buf, { level: 6 })); | ||
| } | ||
| }); | ||
|
|
||
| group("gunzipSync", () => { | ||
| for (const [name, buf] of Object.entries(gzipped)) { | ||
| bench(name, () => zlib.gunzipSync(buf)); | ||
| } | ||
| }); | ||
|
|
||
| // ─── Async one-shot (threadpool) ─── | ||
| group("gzip async level=6", () => { | ||
| for (const [name, buf] of Object.entries(corpora)) { | ||
| bench(name, async () => await gzip(buf, { level: 6 })); | ||
| } | ||
| }); | ||
|
|
||
| group("gunzip async", () => { | ||
| for (const [name, buf] of Object.entries(gzipped)) { | ||
| bench(name, async () => await gunzip(buf)); | ||
| } | ||
| }); | ||
|
|
||
| // ─── Streaming (HTTP server / npm install path) ─── | ||
| // Feed input in 16KB chunks like a real socket would. | ||
| function chunked(buf, size = 16 * 1024) { | ||
| const chunks = []; | ||
| for (let i = 0; i < buf.length; i += size) chunks.push(buf.subarray(i, i + size)); | ||
| return chunks; | ||
| } | ||
|
|
||
| const streamInputs = { | ||
| "html-128K": chunked(corpora["html-128K"]), | ||
| "html-1M": chunked(corpora["html-1M"]), | ||
| }; | ||
| const streamGzInputs = { | ||
| "html-128K": chunked(gzipped["html-128K"]), | ||
| "html-1M": chunked(gzipped["html-1M"]), | ||
| }; | ||
|
|
||
| async function drain(stream) { | ||
| for await (const _ of stream); | ||
| } | ||
|
|
||
| group("createGzip stream level=1", () => { | ||
| for (const [name, chunks] of Object.entries(streamInputs)) { | ||
| bench(name, async () => { | ||
| const gz = zlib.createGzip({ level: 1 }); | ||
| const src = Readable.from(chunks); | ||
| await pipeline(src, gz, drain); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| group("createGzip stream level=6", () => { | ||
| for (const [name, chunks] of Object.entries(streamInputs)) { | ||
| bench(name, async () => { | ||
| const gz = zlib.createGzip({ level: 6 }); | ||
| const src = Readable.from(chunks); | ||
| await pipeline(src, gz, drain); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| group("createGunzip stream", () => { | ||
| for (const [name, chunks] of Object.entries(streamGzInputs)) { | ||
| bench(name, async () => { | ||
| const gz = zlib.createGunzip(); | ||
| const src = Readable.from(chunks); | ||
| await pipeline(src, gz, drain); | ||
| }); | ||
| } | ||
| }); | ||
|
|
||
| // ─── deflateInit/inflateInit overhead (small payloads, many iterations) ─── | ||
| // zlib-ng has higher init cost due to larger state structs. This matters for | ||
| // per-request gzip on tiny responses. | ||
| const tiny = Buffer.from("Hello, World!"); | ||
| const tinyGz = zlib.gzipSync(tiny); | ||
|
|
||
| group("init overhead (13B payload)", () => { | ||
| bench("gzipSync", () => zlib.gzipSync(tiny, { level: 6 })); | ||
| bench("gunzipSync", () => zlib.gunzipSync(tinyGz)); | ||
| bench("deflateSync", () => zlib.deflateSync(tiny, { level: 6 })); | ||
| bench("inflateSync", () => zlib.inflateSync(zlib.deflateSync(tiny))); | ||
| }); | ||
|
|
||
| // ─── Compression ratio (printed, not benched) ─── | ||
| console.log("\n# Compression ratio (output bytes, level=6):"); | ||
| console.log("# corpus input output ratio"); | ||
| for (const [name, buf] of Object.entries(corpora)) { | ||
| const out = zlib.gzipSync(buf, { level: 6 }); | ||
| console.log( | ||
| `# ${name.padEnd(12)} ${String(buf.length).padStart(8)} ${String(out.length).padStart(8)} ${((out.length / buf.length) * 100).toFixed(1)}%`, | ||
| ); | ||
| } | ||
| console.log(`# zlib version: ${process.versions.zlib}\n`); |
There was a problem hiding this comment.
Add a correctness check for the benchmark outputs.
These benches discard every result, so truncated or corrupted output still looks like a performance win. The stream case is especially vulnerable because drain() ignores total bytes entirely. Warm each corpus once and assert the decompressed length or hash matches the source before running the timed groups.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bench/snippets/zlib-comprehensive.mjs` around lines 54 - 165, The benchmark
currently discards outputs (especially in streaming where drain() ignores
bytes), so add a correctness warm-up that for each corpus and for gzipped
entries performs a one-time roundtrip check before running timed groups: for
sync cases call zlib.gzipSync/zlib.gunzipSync and assert output length or hash
equals the original; for async helpers (gzip, gunzip) await a single call and
assert equality; for streams createGzip/createGunzip run pipeline once with
Readable.from(chunks) and a drain that verifies total bytes or computes a
checksum, throwing or console.error on mismatch to fail fast (refer to drain(),
gzip, gunzip, zlib.gzipSync, zlib.gunzipSync, pipeline,
streamInputs/streamGzInputs and the bench group names).
zlib-ng's arch/arm headers and cmake feature probes gate MSVC-specific includes on _MSC_VER alone. clang-cl defines _MSC_VER but ships clang's own <arm_neon.h>/<arm_acle.h>, not MSVC's: - <arm64_neon.h> defines vld1q_u16_x4/vld1q_u8_x4/vst1q_u16_x4 as macros, which macro-expand the polyfill function definitions in neon_intrins.h into garbage (27 errors per file). - <intrin.h> on clang-cl lacks __crc32b/__crc32h/__crc32w/__crc32d; those live in clang's <arm_acle.h>. Patch the guards to `_MSC_VER && !__clang__` in both headers and the two matching cmake check_c_source_compiles probes, so clang-cl takes the standard ACLE/NEON path and ARM_CRC32_INTRIN/ARM_NEON_HASLD4 get defined. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@patches/zlib/clang-cl-arm64.patch`:
- Line 13: Update the patch metadata to record the upstream tracking reference:
open an issue or PR in the zlib-ng upstream repo describing the clang-cl ARM64
failure and this local fix, then edit the patch header in clang-cl-arm64.patch
(the line that currently reads "Upstream: not yet reported (zlib-ng CI does not
cover clang-cl ARM64).") to include the upstream issue/PR URL and ID plus a
brief one-line summary of the upstream ticket; keep the existing note and
[request_verification] tag so future bumps can determine whether the fix was
merged, reverted, or superseded.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ec1e8219-3ea4-415e-86b2-c08c62e980bd
📒 Files selected for processing (2)
patches/zlib/clang-cl-arm64.patchscripts/build/deps/zlib.ts
| so HAVE_ARMV8_INTRIN and NEON_HAS_LD4 succeed under clang-cl, which sets | ||
| ARM_CRC32_INTRIN/ARM_NEON_HASLD4 and skips the polyfills entirely. | ||
|
|
||
| Upstream: not yet reported (zlib-ng CI does not cover clang-cl ARM64). |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
File the upstream issue before this patch becomes institutional memory.
This fix is now load-bearing for a supported toolchain. Please add an upstream issue/PR reference here once filed so the next zlib-ng bump has a clear breadcrumb for whether the patch was merged, reverted, or superseded.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@patches/zlib/clang-cl-arm64.patch` at line 13, Update the patch metadata to
record the upstream tracking reference: open an issue or PR in the zlib-ng
upstream repo describing the clang-cl ARM64 failure and this local fix, then
edit the patch header in clang-cl-arm64.patch (the line that currently reads
"Upstream: not yet reported (zlib-ng CI does not cover clang-cl ARM64).") to
include the upstream issue/PR URL and ID plus a brief one-line summary of the
upstream ticket; keep the existing note and [request_verification] tag so future
bumps can determine whether the fix was merged, reverted, or superseded.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
cloudflare/zlib typedef'd uLong as uint64_t (8 bytes everywhere). zlib-ng compat mode (and stock zlib) use `unsigned long` — 4 bytes on Windows LLP64. Bun's Zig bindings hardcoded uLong = u64, so @sizeof(z_stream) on Windows was 112 vs zlib-ng's 88, tripping CHECK_VER_STSIZE in deflateInit_/inflateInit_ and returning Z_VERSION_ERROR. Downstream code that didn't check (s3) then deref'd a null state pointer. uLong = c_ulong matches the C side on all platforms. Version string is not the cause: all callers pass zlibVersion() and the check only compares [0]. Dropped the unused ZLIB_VERSION/ZLIB_VERNUM constants from the win32 binding since they were stale and misleading. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
zlib-ng sets CMAKE_DEBUG_POSTFIX "d" inside if(MSVC), which clang-cl satisfies, so debug builds produce zlibstaticd.lib. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/build/source.ts`:
- Around line 377-385: The fetchDeps resolution currently uses r.outputs (final
build artifacts) for nested-CMake dependencies causing downstream configure to
wait on final libs instead of configure-time artifacts; change the dependency
model so each dep exposes two outputs: a configure-stage stamp (e.g.,
r.configureStamp or r.configureOutput) produced by the cmake configure step and
the existing final outputs (r.outputs) produced by build. Update the logic in
fetchDeps resolution (the code that maps deps to r.outputs) to use the
configure-stage stamp as the implicit/Order-only input for downstream configure
runs while preserving r.outputs as the implicit/explicit build-time dependency,
and ensure zlib-ng and other nestedCMake deps create and export that
configure-stage stamp (and that provides.libs remains unchanged for link-time
libraries).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40804b71-11f6-4d39-9bf2-84250d37878f
📒 Files selected for processing (2)
scripts/build/deps/zlib.tsscripts/build/source.ts
| * Other deps that must be BUILT before this dep's configure runs. | ||
| * Used for header-level dependencies — e.g. libarchive needs zlib's | ||
| * headers at compile time (`-I${vendorDir}/zlib`), so zlib must be | ||
| * fetched first. This adds an order-only dep on the other dep's source | ||
| * stamp — it does NOT link the other dep's libs (that's `provides.libs`). | ||
| * headers at configure time (`check_include_file("zlib.h")`). zlib-ng | ||
| * generates `zlib.h` during its own cmake configure, so libarchive must | ||
| * wait for zlib's full build, not just its source fetch. | ||
| * | ||
| * Resolves to the named dep's build outputs (lib files for nested-cmake, | ||
| * source stamp for header-only). Order-only on configure, implicit on | ||
| * build. Does NOT link the other dep's libs (that's `provides.libs`). |
There was a problem hiding this comment.
Track configure-time artifacts separately from final build outputs.
Line 710 now resolves fetchDeps to r.outputs, but for nested-CMake deps those are the final lib*.a files, not the configure-time contract. For zlib-ng, zlib.h is produced during cmake -B, so downstream configure currently waits for the full zlib build and still does not get dirtied when that generated header changes. That leaves room for stale CMakeCache.txt results across dep bumps or branch switches.
Please expose a configure-stage output/stamp and use that as an implicit input to downstream configure, while keeping final libs as the build-time dependency.
Possible direction
export interface ResolvedDep {
+ configureOutputs: string[];
outputs: string[];
}
- const fetchDepStamps = (dep.fetchDeps ?? []).flatMap(d => {
+ const fetchDepConfigureInputs = (dep.fetchDeps ?? []).flatMap(d => {
const r = resolved.get(d);
assert(r, `${dep.name}: fetchDeps references '${d}' but it wasn't resolved first — fix allDeps ordering`);
- return r.outputs;
+ return r.configureOutputs;
});Also applies to: 697-714
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/build/source.ts` around lines 377 - 385, The fetchDeps resolution
currently uses r.outputs (final build artifacts) for nested-CMake dependencies
causing downstream configure to wait on final libs instead of configure-time
artifacts; change the dependency model so each dep exposes two outputs: a
configure-stage stamp (e.g., r.configureStamp or r.configureOutput) produced by
the cmake configure step and the existing final outputs (r.outputs) produced by
build. Update the logic in fetchDeps resolution (the code that maps deps to
r.outputs) to use the configure-stage stamp as the implicit/Order-only input for
downstream configure runs while preserving r.outputs as the implicit/explicit
build-time dependency, and ensure zlib-ng and other nestedCMake deps create and
export that configure-stage stamp (and that provides.libs remains unchanged for
link-time libraries).
- Export uLong/uLongf from src/zlib.zig so callers can use the portable type - HashObject crc32: local accumulator was u64, now bun.zlib.uLong - crash_handler compress2: len was *usize, now *bun.zlib.uLong; @intcast the message.len source and the slice index zig:check-all clean on all 14 platform/mode combos. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zlib.zig`:
- Line 847: The call to deflateBound uses `@intCast`(input.len) which can truncate
a 64-bit usize to 32-bit uLong on LLP64 (Windows); add an explicit bounds guard
before narrowing input.len to uLong in the
zlib_reader.list.ensureTotalCapacityPrecise call: check that input.len <=
`@as`(usize, `@intCast`(usize, `@as`(u64, `@as`(uLong, -1)))) or simply compare against
the max value of uLong, and return or error if it exceeds that limit; then
safely cast input.len to uLong when calling deflateBound(&zlib_reader.zlib,
`@intCast`(uLong, input.len)) to prevent silent truncation and potential heap
corruption.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6f749c96-9acb-406d-a2df-ab366d0be92a
📒 Files selected for processing (1)
src/zlib.zig
| )) { | ||
| ReturnCode.Ok => { | ||
| zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, input.len)) catch { | ||
| zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, @intCast(input.len))) catch { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify deflateBound usage and whether narrowing casts are guarded.
# Expected: each usize->uLong cast for deflateBound has an explicit pre-check.
rg -nP --type=zig -C3 '\bdeflateBound\s*\('
rg -nP --type=zig -C4 '@intCast\s*\(\s*input\.len\s*\)'
rg -nP --type=zig -C6 'input\.len\s*>\s*[^;\n]+'Repository: oven-sh/bun
Length of output: 21212
🏁 Script executed:
# Check uLong definition and platform-specific sizing
rg -nP 'typedef.*uLong|const.*uLong|pub\s+const.*uLong' --type=zig --type=c | head -20
# Look for uLong definition in C headers
fd -e h | xargs grep -l 'uLong' 2>/dev/null | head -10Repository: oven-sh/bun
Length of output: 1872
🏁 Script executed:
# Examine the context around all uLong casts in src/zlib.zig
sed -n '830,860p' src/zlib.zigRepository: oven-sh/bun
Length of output: 1170
🏁 Script executed:
# Check for other input.len casts in the file - specifically to smaller integer types
rg -nP '@intCast\s*\(\s*input\.len\s*\)|@as\s*\(\s*u(Int|Long)' src/zlib.zig -A2 -B2Repository: oven-sh/bun
Length of output: 623
🏁 Script executed:
# Look for existing bounds guards on input.len casts in the wider codebase
rg -nP 'input\.len\s*>\s*.*\n.*@intCast|if.*input\.len.*\n.*@intCast' --multiline --type=zig | head -15Repository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check what error types are used for input validation in zlib.zig
rg -nP '(return error\.|error\.)' src/zlib.zig | head -20
# Check if error.InvalidArgument is used elsewhere in codebase for input validation
rg -nP 'error\.InvalidArgument' --type=zig | head -10Repository: oven-sh/bun
Length of output: 2911
🏁 Script executed:
# Verify the max value computation is correct for uLong on different platforms
cat > /tmp/test_ulong.zig << 'EOF'
const std = `@import`("std");
const c = `@cImport`({
`@cInclude`("limits.h");
});
pub fn main() void {
var writer = std.io.getStdOut().writer();
// Simulating what ~@as(uLong, 0) would compute
const max_u32: u32 = ~@as(u32, 0);
const max_u64: u64 = ~@as(u64, 0);
writer.print("max u32: {}\n", .{max_u32}) catch {};
writer.print("max u64: {}\n", .{max_u64}) catch {};
}
EOF
# This shows the logic is sound for computing max valuesRepository: oven-sh/bun
Length of output: 37
🏁 Script executed:
# Check if there are similar checks elsewhere in zlib.zig or related files
rg -nP 'if.*\.len\s*>\s*' src/zlib.zig src/deps/zlib.*.zig -B2 -A3Repository: oven-sh/bun
Length of output: 501
🏁 Script executed:
# Specifically verify: are lines 171-172 in same function scope as line 847?
sed -n '160,180p' src/zlib.zigRepository: oven-sh/bun
Length of output: 836
Add an explicit bounds guard before narrowing input.len to uLong at line 847.
On Windows (LLP64 platform), uLong is 32-bit while usize is 64-bit. Direct casting via @intCast(input.len) silently truncates inputs larger than 4GB, causing deflateBound() to compute an undersized buffer allocation and risking heap corruption.
Proposed fix
)) {
ReturnCode.Ok => {
- zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, `@intCast`(input.len))) catch {
+ const max_source_len: usize = `@intCast`(~@as(uLong, 0));
+ if (input.len > max_source_len) {
+ zlib_reader.deinit();
+ return error.InvalidArgument;
+ }
+ const source_len: uLong = `@intCast`(input.len);
+ zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, source_len)) catch {
zlib_reader.deinit();
return error.OutOfMemory;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, @intCast(input.len))) catch { | |
| const max_source_len: usize = `@intCast`(~@as(uLong, 0)); | |
| if (input.len > max_source_len) { | |
| zlib_reader.deinit(); | |
| return error.InvalidArgument; | |
| } | |
| const source_len: uLong = `@intCast`(input.len); | |
| zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, source_len)) catch { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zlib.zig` at line 847, The call to deflateBound uses `@intCast`(input.len)
which can truncate a 64-bit usize to 32-bit uLong on LLP64 (Windows); add an
explicit bounds guard before narrowing input.len to uLong in the
zlib_reader.list.ensureTotalCapacityPrecise call: check that input.len <=
`@as`(usize, `@intCast`(usize, `@as`(u64, `@as`(uLong, -1)))) or simply compare against
the max value of uLong, and return or error if it exceeds that limit; then
safely cast input.len to uLong when calling deflateBound(&zlib_reader.zlib,
`@intCast`(uLong, input.len)) to prevent silent truncation and potential heap
corruption.
|
@robobun fix ❌ CPU instruction violation on Windows x64 — 1 check(s) failed Static instruction scan If there's no gate: this is a real bug — a -march leaked into a subbuild. Find the translation unit and fix its compile flags. ❌ CPU instruction violation on Linux x64 — 1 check(s) failed Static instruction scan If there's no gate: this is a real bug — a -march leaked into a subbuild. Find the translation unit and fix its compile flags. ❌ CPU instruction violation on Linux x64 — 1 check(s) failed Static instruction scan If there's no gate: this is a real bug — a -march leaked into a subbuild. Find the translation unit and fix its compile flags. |
|
✅ Resolved — zlib-ng functable dispatch targets were added to |
zlib-ng compiles per-ISA variants (PCLMULQDQ/AVX2/AVX512/VNNI/VPCLMULQDQ) and selects at runtime via init_functable() -> cpu_check_features() CPUID in vendor/zlib/functable.c. These are intentionally present in baseline builds; the static scanner just needs to know they're gated. Ceilings match the gate predicates in arch/x86/x86_features.c: has_pclmulqdq, has_avx2 && has_bmi2, has_avx512_common (F+DQ+BW+VL+BMI2), has_avx512vnni, has_vpclmulqdq. chunkcopy_safe is a static-inline (inflate_p.h) that the compiler may outline from one of the per-ISA inffast_tpl.h instantiations; the outlined local copy is only reachable from the gated inflate_fast_* of that TU. Replaces the old single-symbol cloudflare-zlib `crc32` entry on Linux. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
| )) { | ||
| ReturnCode.Ok => { | ||
| zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, input.len)) catch { | ||
| zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, @intCast(input.len))) catch { |
There was a problem hiding this comment.
🔴 At src/zlib.zig:847, deflateBound is called as deflateBound(&zlib_reader.zlib, @intCast(input.len)), but this PR changed uLong from u64 to c_ulong (= u32 on Windows LLP64), so @intCast will panic in Zig debug/safe builds for any input.len > 4GB. The surrounding code at lines 817–818 uses @truncate for the exact same narrowing cast (avail_in, total_in); line 847 should match. Fix: change @intCast(input.len) → @truncate(input.len).
Extended reasoning...
What the bug is and how it manifests
This PR correctly changes const uLong = c_ulong at src/zlib.zig lines 22–26 to fix the Windows LLP64 ABI (where unsigned long is 4 bytes, not 8). However, the deflateBound call site at line 847 was updated to add a cast but used @intCast instead of @truncate:
zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, @intCast(input.len)))In Zig, @intCast is a checked narrowing cast: it panics at runtime in debug and ReleaseSafe builds when the source value exceeds the destination type's range. On Windows where uLong = c_ulong = u32, any input.len > 0xFFFF_FFFF (4GB) will trigger a guaranteed runtime panic.
The specific code path that triggers it
ZlibCompressorArrayList.initWithListAllocator (called from ZlibCompressorArrayList.init, which backs synchronous zlib compression) calls deflateBound to pre-size the output buffer. input.len is usize = u64 on all 64-bit platforms; the cast to uLong = u32 on Windows silently works for inputs ≤ 4GB but panics or truncates for larger ones.
Why existing code doesn't prevent it
The two narrowing casts directly above (lines 817–818) for avail_in and total_in — the same usize → uLong narrowing — already use @truncate:
.avail_in = @truncate(input.len),
.total_in = @truncate(input.len),@truncate is the Zig idiom that explicitly acknowledges truncation without a safety check. Line 847 was missed in this PR when the @intCast was added to satisfy the new type, resulting in an inconsistency that compiles cleanly but behaves differently at runtime.
What the impact would be
- Debug/ReleaseSafe builds on Windows: guaranteed panic for inputs > 4GB (e.g. any internal Zig caller passing a large
[]u8slice). - ReleaseFast/ReleaseSmall builds on Windows: silent truncation —
deflateBoundreceives a wrong (too-small) length estimate. The output buffer is under-sized, causing the subsequentdeflate()call to returnZ_BUF_ERROR(no progress made), breaking synchronous compression. - All non-Windows platforms: no impact —
c_ulong = u64there, no truncation occurs.
In practice, JS ArrayBuffer inputs via the public API are capped below 4GB by V8/JSC, so this is unreachable from JavaScript. However, internal Zig callers (or future code) are not subject to that constraint.
How to fix it
Change line 847 to match the pattern used for avail_in/total_in:
// Before:
zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, @intCast(input.len)))
// After:
zlib_reader.list.ensureTotalCapacityPrecise(list_allocator, deflateBound(&zlib_reader.zlib, @truncate(input.len)))Step-by-step proof
- PR changes
uLong = c_ulong; on Windows LLP64,c_ulong = u32. deflateBoundC signature:uLong deflateBound(z_streamp strm, uLong sourceLen)— both 32-bit on Windows.input.lenisusize = u64on 64-bit platforms.@intCast(input.len)at line 847: ifinput.len > std.math.maxInt(u32), Zig's safety check fires → panic in debug.- Lines 817–818 use
@truncate(input.len)for the identical narrowing — no safety check, explicit truncation. - The inconsistency is the direct result of this PR adding a cast at line 847 but choosing the wrong cast builtin.
🔬 also observed by coderabbitai
|
Thanks for the update. Just upgraded bun yesterday and my ci pipeline failed for regression in size vs nodejs. Is there a chance that the compression ratio has changed between bun 1.3.12 and 1.3.13 ? /**
* Compress the given data and return it as a Uint8Array binary format
*
* ```typescript
* import { Compressor } from '@httpx/compress';
*
* const compressor = new Compressor('gzip');
* const longString = 'Hello, World! 🦆'.repeat(500_000);
* const compressedBinary = await compressor.Uint8Array(longString);
* ```
* @throws Error
*/
toUint8Array = async <T extends string | Uint8Array>(
data: T
): Promise<Uint8Array> => {
const readableStream = new ReadableStream({
start(controller) {
controller.enqueue(
typeof data === 'string'
? globalCache.utf8TextEncoder.encode(data)
: data
);
controller.close();
},
});
const compressedStream = readableStream.pipeThrough(
new CompressionStream(this.#algorithm)
);
return new Uint8Array(await new Response(compressedStream).arrayBuffer());
};
|
## What does this PR do? Replaces the cloudflare/zlib fork (last commit Oct 2023) with [zlib-ng](https://github.com/zlib-ng/zlib-ng) 2.3.3 in `ZLIB_COMPAT` mode. zlib-ng is actively maintained, ships in Node 24+ and Chromium, and provides runtime-dispatched SIMD across AVX-512/AVX2/SSE2/NEON/SVE/RVV for CRC32, adler32, longest-match, and chunk-copy. Supersedes oven-sh#16100, oven-sh#8529. ## Benchmarks Xeon Platinum 8375C (Ice Lake, AVX-512), linux-x64 release build vs system bun 1.3.13. Run with `bench/snippets/zlib-comprehensive.mjs` and `bench/snippets/zlib.mjs` (both included). | Operation | cloudflare | zlib-ng | Speedup | |---|---:|---:|---:| | `gzipSync` html-128K L1 | 275 µs | 107 µs | **2.59x** | | `gzipSync` html-1M L1 | 2.23 ms | 892 µs | **2.50x** | | `gzipSync` json-128K L6 | 897 µs | 483 µs | **1.86x** | | `deflate` 123K L6 (async) | 373 µs | 68 µs | **5.48x** | | `gunzipSync` html-1M | 561 µs | 522 µs | 1.07x | | `gunzipSync` binary-128K | 31.6 µs | 26.7 µs | 1.18x | | `createGzip` stream L1 1M | 3.76 ms | 2.68 ms | **1.40x** | | `createGunzip` stream 1M | 1.24 ms | 1.18 ms | 1.05x | | `fetch()` 11KB gzip decode | 42.9 µs | 41.6 µs | parity | | `gzipSync` 13B (init overhead) | 5.04 µs | 7.12 µs | 0.71x | The streaming-inflate regression that blocked oven-sh#16100 (Jan 2025, zlib-ng pre-2.2) **does not reproduce** on 2.3.3. The only downside is ~2µs higher per-stream init cost from larger state structs, amortized away on payloads ≥4KB. Compression ratio at level=6 is +0.4% vs cloudflare (different match-finding heuristics). Negligible. ## Security hardening Built with `-DWITH_INFLATE_STRICT=ON`. zlib-ng commit `340f2f6e` moved `inflateBack()`'s distance-too-far-back check behind a default-off `#ifdef`; upstream zlib has it unconditional. Bun doesn't call `inflateBack()`, but this hardens against heap OOB reads on malicious raw-deflate with `windowBits<15` for anything else linking the same lib, at zero cost to `inflate()` proper. ## Why pin to 2.3.3 (not develop) Two regressions landed on zlib-ng `develop` after 2.3.3 that are **not** present at this commit (documented in `zlib.ts`): - `172b8544` — inverted `COPY` guard disables Chorba CRC32 fast-path on PCLMULQDQ-only x64 - `e5129cfe` — `deflateBound()` hits `__builtin_unreachable()` after `Z_FINISH` Re-audit before bumping past 2.3.3. ## Build system changes zlib-ng generates `zlib.h` at cmake-configure time into the **build** dir (it doesn't exist in source). This required: - `provides.includes` → `depBuildDir(cfg, "zlib")` instead of source dir - libarchive's `-I` → build dir - `fetchDeps` now resolves to the cross-dep's **build outputs** (lib files) instead of just the source `.ref` stamp, so libarchive's configure waits for zlib's configure to have run. `resolveDep()` takes a map of previously-resolved deps. Drops 4 cloudflare-specific vendor patches. ## How did you verify your code works? - [x] linux-x64 release build: `bun run build:release` clean → smoke test passes - [x] `test/js/node/zlib/zlib.test.js`: **376 pass**, 0 fail (release build) - [x] `bun bd test test/js/node/zlib/`: deflate/gzip/inflate tests pass (1 unrelated brotli timeout in debug — `createBrotliCompress` slowness, untouched by this PR) - [x] Build-graph ordering verified: `build.ninja` shows libarchive configure has `deps/zlib/libz.a` as order-only input - [x] `bunx tsc --noEmit -p scripts/build/tsconfig.json` clean - [ ] Windows (lib name → `zlibstatic`) — needs CI - [ ] aarch64/musl — needs CI 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: root <root@ip-10-0-2-234.us-west-2.compute.internal> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
## What does this PR do? Replaces the cloudflare/zlib fork (last commit Oct 2023) with [zlib-ng](https://github.com/zlib-ng/zlib-ng) 2.3.3 in `ZLIB_COMPAT` mode. zlib-ng is actively maintained, ships in Node 24+ and Chromium, and provides runtime-dispatched SIMD across AVX-512/AVX2/SSE2/NEON/SVE/RVV for CRC32, adler32, longest-match, and chunk-copy. Supersedes oven-sh#16100, oven-sh#8529. ## Benchmarks Xeon Platinum 8375C (Ice Lake, AVX-512), linux-x64 release build vs system bun 1.3.13. Run with `bench/snippets/zlib-comprehensive.mjs` and `bench/snippets/zlib.mjs` (both included). | Operation | cloudflare | zlib-ng | Speedup | |---|---:|---:|---:| | `gzipSync` html-128K L1 | 275 µs | 107 µs | **2.59x** | | `gzipSync` html-1M L1 | 2.23 ms | 892 µs | **2.50x** | | `gzipSync` json-128K L6 | 897 µs | 483 µs | **1.86x** | | `deflate` 123K L6 (async) | 373 µs | 68 µs | **5.48x** | | `gunzipSync` html-1M | 561 µs | 522 µs | 1.07x | | `gunzipSync` binary-128K | 31.6 µs | 26.7 µs | 1.18x | | `createGzip` stream L1 1M | 3.76 ms | 2.68 ms | **1.40x** | | `createGunzip` stream 1M | 1.24 ms | 1.18 ms | 1.05x | | `fetch()` 11KB gzip decode | 42.9 µs | 41.6 µs | parity | | `gzipSync` 13B (init overhead) | 5.04 µs | 7.12 µs | 0.71x | The streaming-inflate regression that blocked oven-sh#16100 (Jan 2025, zlib-ng pre-2.2) **does not reproduce** on 2.3.3. The only downside is ~2µs higher per-stream init cost from larger state structs, amortized away on payloads ≥4KB. Compression ratio at level=6 is +0.4% vs cloudflare (different match-finding heuristics). Negligible. ## Security hardening Built with `-DWITH_INFLATE_STRICT=ON`. zlib-ng commit `340f2f6e` moved `inflateBack()`'s distance-too-far-back check behind a default-off `#ifdef`; upstream zlib has it unconditional. Bun doesn't call `inflateBack()`, but this hardens against heap OOB reads on malicious raw-deflate with `windowBits<15` for anything else linking the same lib, at zero cost to `inflate()` proper. ## Why pin to 2.3.3 (not develop) Two regressions landed on zlib-ng `develop` after 2.3.3 that are **not** present at this commit (documented in `zlib.ts`): - `172b8544` — inverted `COPY` guard disables Chorba CRC32 fast-path on PCLMULQDQ-only x64 - `e5129cfe` — `deflateBound()` hits `__builtin_unreachable()` after `Z_FINISH` Re-audit before bumping past 2.3.3. ## Build system changes zlib-ng generates `zlib.h` at cmake-configure time into the **build** dir (it doesn't exist in source). This required: - `provides.includes` → `depBuildDir(cfg, "zlib")` instead of source dir - libarchive's `-I` → build dir - `fetchDeps` now resolves to the cross-dep's **build outputs** (lib files) instead of just the source `.ref` stamp, so libarchive's configure waits for zlib's configure to have run. `resolveDep()` takes a map of previously-resolved deps. Drops 4 cloudflare-specific vendor patches. ## How did you verify your code works? - [x] linux-x64 release build: `bun run build:release` clean → smoke test passes - [x] `test/js/node/zlib/zlib.test.js`: **376 pass**, 0 fail (release build) - [x] `bun bd test test/js/node/zlib/`: deflate/gzip/inflate tests pass (1 unrelated brotli timeout in debug — `createBrotliCompress` slowness, untouched by this PR) - [x] Build-graph ordering verified: `build.ninja` shows libarchive configure has `deps/zlib/libz.a` as order-only input - [x] `bunx tsc --noEmit -p scripts/build/tsconfig.json` clean - [ ] Windows (lib name → `zlibstatic`) — needs CI - [ ] aarch64/musl — needs CI 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: root <root@ip-10-0-2-234.us-west-2.compute.internal> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
3 participants
What does this PR do?
Replaces the cloudflare/zlib fork (last commit Oct 2023) with zlib-ng 2.3.3 in
ZLIB_COMPATmode. zlib-ng is actively maintained, ships in Node 24+ and Chromium, and provides runtime-dispatched SIMD across AVX-512/AVX2/SSE2/NEON/SVE/RVV for CRC32, adler32, longest-match, and chunk-copy.Supersedes #16100, #8529.
Benchmarks
Xeon Platinum 8375C (Ice Lake, AVX-512), linux-x64 release build vs system bun 1.3.13. Run with
bench/snippets/zlib-comprehensive.mjsandbench/snippets/zlib.mjs(both included).gzipSynchtml-128K L1gzipSynchtml-1M L1gzipSyncjson-128K L6deflate123K L6 (async)gunzipSynchtml-1MgunzipSyncbinary-128KcreateGzipstream L1 1McreateGunzipstream 1Mfetch()11KB gzip decodegzipSync13B (init overhead)The streaming-inflate regression that blocked #16100 (Jan 2025, zlib-ng pre-2.2) does not reproduce on 2.3.3. The only downside is ~2µs higher per-stream init cost from larger state structs, amortized away on payloads ≥4KB.
Compression ratio at level=6 is +0.4% vs cloudflare (different match-finding heuristics). Negligible.
Security hardening
Built with
-DWITH_INFLATE_STRICT=ON. zlib-ng commit340f2f6emovedinflateBack()'s distance-too-far-back check behind a default-off#ifdef; upstream zlib has it unconditional. Bun doesn't callinflateBack(), but this hardens against heap OOB reads on malicious raw-deflate withwindowBits<15for anything else linking the same lib, at zero cost toinflate()proper.Why pin to 2.3.3 (not develop)
Two regressions landed on zlib-ng
developafter 2.3.3 that are not present at this commit (documented inzlib.ts):172b8544— invertedCOPYguard disables Chorba CRC32 fast-path on PCLMULQDQ-only x64e5129cfe—deflateBound()hits__builtin_unreachable()afterZ_FINISHRe-audit before bumping past 2.3.3.
Build system changes
zlib-ng generates
zlib.hat cmake-configure time into the build dir (it doesn't exist in source). This required:provides.includes→depBuildDir(cfg, "zlib")instead of source dir-I→ build dirfetchDepsnow resolves to the cross-dep's build outputs (lib files) instead of just the source.refstamp, so libarchive's configure waits for zlib's configure to have run.resolveDep()takes a map of previously-resolved deps.Drops 4 cloudflare-specific vendor patches.
How did you verify your code works?
bun run build:releaseclean → smoke test passestest/js/node/zlib/zlib.test.js: 376 pass, 0 fail (release build)bun bd test test/js/node/zlib/: deflate/gzip/inflate tests pass (1 unrelated brotli timeout in debug —createBrotliCompressslowness, untouched by this PR)build.ninjashows libarchive configure hasdeps/zlib/libz.aas order-only inputbunx tsc --noEmit -p scripts/build/tsconfig.jsoncleanzlibstatic) — needs CI🤖 Generated with Claude Code