Skip to content

src: fix build with --shared-brotli - #66895

Open
lazerg wants to merge 3 commits into
nodejs:mainfrom
lazerg:fix/issue-66894-brotli-block-switch
Open

lazerg wants to merge 3 commits into
nodejs:mainfrom
lazerg:fix/issue-66894-brotli-block-switch

Conversation

@lazerg

@lazerg lazerg commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

BROTLI_DECODER_ERROR_FORMAT_BLOCK_SWITCH isn't in any brotli release yet (1.2.0 is the latest), so building with --shared-brotli fails since 7ba22c2. It's an enum value and the bundled brotli still reports 1.2.0, so there's nothing to #ifdef on. This only exposes the constant when brotli is bundled and skips the matching test otherwise.

I built with --shared-brotli against Homebrew brotli 1.2.0 and ran parallel/test-zlib* (90 tests, all pass). The GN change is untested, I had no gn locally.

Fixes: #66894

Assisted-by: Claude Code
Signed-off-by: lazerg <lazerg2@gmail.com>
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to Node.js builds or CI infrastructure. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies. labels Oct 21, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Caution

AgentScan found account activity patterns that may be consistent with automation. This is a heuristic, not proof that this pull request was opened by an agent or violates policy. AI-assisted contributions are permitted, but automated tooling must not open pull requests without advance approval, and contributors must personally understand, test, verify, and take responsibility for every submitted change. See the AgentScan analysis, AI use policy, and automation policy for additional context.

Comment thread src/node_zlib.cc Outdated
Comment thread node.gypi Outdated
Comment on lines +255 to +256
}, {
'defines': [ 'NODE_SHARED_BROTLI' ],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
}, {
'defines': [ 'NODE_SHARED_BROTLI' ],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

✅ Done

Comment thread unofficial.gni Outdated
Comment thread test/parallel/test-zlib-invalid-input.js Outdated
@aduh95

aduh95 commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Can you remove the cherry-pick to validate it does fix this build and still pass the tests

brotli = pkgs.brotli.overrideAttrs (old: {
patches = (old.patches or [ ]) ++ [
(pkgs.fetchpatch2 {
url = "https://github.com/google/brotli/commit/0ed4268a4401abcb4472096340477cf4af7d1a2b.patch?full_index=1";
hash = "sha256-dVpnYI2JLDEt2VQkbI6RnbpdcQSMB5bTQ6TJp5CLnyw=";
})
];
});

@codecov

codecov Bot commented Oct 21, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.58%. Comparing base (7ba22c2) to head (c47b864).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #66895   +/-   ##
=======================================
  Coverage   90.58%   90.58%           
=======================================
  Files         797      797           
  Lines      278434   278465   +31     
  Branches    53531    53531           
=======================================
+ Hits       252216   252251   +35     
+ Misses      16571    16570    -1     
+ Partials     9647     9644    -3     
Files with missing lines Coverage Δ
src/node_zlib.cc 79.86% <100.00%> (-0.44%) ⬇️

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ziyao233

Copy link
Copy Markdown
Contributor

Since commit introducing this regression is related to a CVE, I wonder would such workaround breaks the fix?

@0x9fff00

Copy link
Copy Markdown

Brotli 1.2.1 has been released with a backport of the fix for CVE-2026-111497 which added BROTLI_DECODER_ERROR_FORMAT_BLOCK_SWITCH.

Since commit introducing this regression is related to a CVE, I wonder would such workaround breaks the fix?

Yes. If --shared-brotli is used, Brotli 1.2.1 is required in order to fix CVE-2026-111497 because in that case the vendored copy of Brotli in Node.js is not used.

Assisted-by: Claude Code
Signed-off-by: lazerg <lazerg2@gmail.com>
Assisted-by: Claude Code
Signed-off-by: lazerg <lazerg2@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor
Platform Number of requisites Proportion of new derivations
x86_64-linux 544 -> 541 => +-3 0 / 541 = 0.0%
aarch64-linux 545 -> 542 => +-3 0 / 542 = 0.0%
x86_64-darwin 457 -> 454 => +-3 0 / 454 = 0.0%
aarch64-darwin 470 -> 467 => +-3 0 / 467 = 0.0%
Total 2016 -> 2004 => +-12 0 / 2004 = 0.0%
Changelog
@@ -120 +119,0 @@
-/nix/store/kminssh6dm17s3f6cqszaxwpyyhm6i88-brotli-1.2.0 (aarch64-darwin)
@@ -122,2 +120,0 @@
-/nix/store/6aaijr2v6rlgailsir2l1q9j3k72flxx-brotli-1.2.0 (aarch64-linux)
-/nix/store/895yl6jdl10mkffq8qrgimy2i7snla3q-brotli-1.2.0 (x86_64-darwin)
@@ -125 +121,0 @@
-/nix/store/2nyhgrln2i4rw0rdijyqh0nn333ws3nx-brotli-1.2.0 (x86_64-linux)
@@ -128,2 +123,0 @@
-/nix/store/c4y22jmyh7wjhbdidbnhz661jpg9kaim-brotli-1.2.0-dev (aarch64-darwin)
-/nix/store/44lj6r7v54yfzcd7p6l6n76g7ma7y633-brotli-1.2.0-dev (aarch64-linux)
@@ -132 +125,0 @@
-/nix/store/af449dhklypcj2kild1fv4q9dy2bmjdv-brotli-1.2.0-dev (x86_64-darwin)
@@ -134 +126,0 @@
-/nix/store/ainnzs18blgfd5f3n21wnq0m4j5ryf6j-brotli-1.2.0-dev (x86_64-linux)
@@ -136 +127,0 @@
-/nix/store/kl3m09q74kf4g4mfk9whw65w8dw0jg3a-brotli-1.2.0-lib (aarch64-darwin)
@@ -138 +128,0 @@
-/nix/store/r5vy3djdp4qpddj4bsw8hqcl9asyp1h2-brotli-1.2.0-lib (aarch64-linux)
@@ -140 +129,0 @@
-/nix/store/yvkvvnyvnaw08bl9gvnkbk33m339v4ff-brotli-1.2.0-lib (x86_64-darwin)
@@ -142 +130,0 @@
-/nix/store/f667bn2vz2vv1hf4ayjc49r3w967ivhr-brotli-1.2.0-lib (x86_64-linux)

@lazerg

lazerg commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

@aduh95 ✅ Done

I removed the Nix cherry-pick of the brotli patch (c47b864). The shared-libraries CI now builds against unpatched brotli, so it checks the fix by itself.

I did not use your #ifdef suggestion for src/node_zlib.cc. BROTLI_DECODER_ERROR_FORMAT_BLOCK_SWITCH is an enum value, and #ifdef cannot see an enum value. The constant would be missing from every build. The constants now come from BROTLI_DECODER_ERROR_CODES_LIST in the brotli header (67111ab).

Local result: all 90 parallel/test-zlib* tests pass with system brotli 1.2.1. With brotli 1.2.0, the block-switch test is skipped.

@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Oct 22, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Oct 22, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95 aduh95 added commit-queue PRs queued for automated landing through the Commit Queue. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x labels Oct 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agentscan:community-flagged author ready PRs with CI started, the required approvals, and no outstanding review comments. build Issues and PRs related to Node.js builds or CI infrastructure. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue PRs queued for automated landing through the Commit Queue. lts-watch-v22.x PRs that may need to be released in v22.x lts-watch-v24.x PRs that may need to be released in v24.x needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

03923858417d ("src: expose BROTLI_DECODER_ERROR_FORMAT_BLOCK_SWITCH") breaks build with shared, latest stable brotli

7 participants