Skip to content

zlib: fix pointer alignment - #57727

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jhofstee:fix-brotli-crash
Apr 9, 2025
Merged

zlib: fix pointer alignment#57727
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
jhofstee:fix-brotli-crash

Conversation

@jhofstee

Copy link
Copy Markdown
Contributor

The function AllocForBrotli prefixes the allocated memory with its size, and returns a pointer to the region after it. This pointer can however no longer be suitably aligned. Correct this by allocating the maximum of the the size of the size_t and the max alignment.

On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for some NEON instructions. When Brotli is compiled with optimizations enabled newer GCC versions will use the NEON instructions and trigger a bus error killing node.

see google/brotli#1159

I don't think there is any additional test needed, since existing test will crash node already.
Not sure about the notable-change label.

The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
@nodejs-github-botnodejs-github-bot added 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 subsystem. labels Apr 2, 2025
@targos

Copy link
Copy Markdown
Member

/cc @nodejs/cpp-reviewers

I can't review this, but I'm very interested as it might fix crashes that we have with the latest V8 update: https://ci.nodejs.org/job/node-test-commit-arm/57862/

@targostargos mentioned this pull request Apr 2, 2025
6 tasks
@codecov

codecovBot commented Apr 2, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.24%. Comparing base (1c2d98d) to head (152058a).
Report is 85 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57727 +/- ##
==========================================
+ Coverage 90.22% 90.24% +0.01% 
==========================================
Files 630 630 Lines 185073 185075 +2 Branches 36222 36222 ==========================================
+ Hits 166990 167017 +27 + Misses 11044 11034 -10 + Partials 7039 7024 -15 
Files with missing linesCoverage Δ
src/node_zlib.cc78.80% <100.00%> (+0.43%)⬆️

... and 24 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.

@lemirelemire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe that alignof(std::max_align_t)should always be as large assizeof(size_t)` so the code might be unnecessarily complicated, but it has the benefit of being clear, so I recommend merging as-is.

@jhofstee

Copy link
Copy Markdown
ContributorAuthor

I believe that alignof(std::max_align_t)should always be as large assizeof(size_t)` so the code might be unnecessarily complicated, but it has the benefit of being clear, so I recommend merging as-is.

Actually I initially wrote that, only using alignof(std::max_align_t), but that might be confusing to read, since it is casted to a <size_t*> thereafter and a reader might be led to believe it is wrong and put a size_t back. In practice the alignment is often, if not always, twice the sizeof(size_t) and guaranteed by the standard not to be less if I am not mistaken. But like this, the code clearly states it intend, make room for a size_t, while keeping the data after it suitable aligned. So it is clearer like this if you ask me and the compiler will remove the Max for us.

Comment threadsrc/node_zlib.cc
@FlarnaFlarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 9, 2025
@nodejs-github-bot

This comment was marked as outdated.

@FlarnaFlarna added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@richardlaurichardlau added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 9, 2025
@nodejs-github-botnodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 9, 2025
@nodejs-github-bot

This comment was marked as outdated.

@richardlaurichardlau added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Apr 9, 2025
@richardlau

This comment was marked as outdated.

@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025
aduh95 pushed a commit that referenced this pull request May 17, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
aduh95 pushed a commit that referenced this pull request May 19, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
codebytere added a commit to electron/electron that referenced this pull request May 23, 2025
codebytere added a commit to electron/electron that referenced this pull request May 26, 2025
codebytere added a commit to electron/electron that referenced this pull request May 29, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 2, 2025
jkleinsc pushed a commit to electron/electron that referenced this pull request Jun 2, 2025
* chore: bump node in DEPS to v22.16.0
* crypto: remove BoringSSL dh-primes addition
nodejs/node#57023
* tools: enable linter in test/fixtures/test\-runner/output
nodejs/node#57698
* src: improve thread safety of TaskQueue
nodejs/node#57910
* buffer: define global v8::CFunction objects as const
nodejs/node#57676
* src: disable abseil deadlock detection
nodejs/node#57582
* zlib: fix pointer alignment
nodejs/node#57727
* chore: fixup patch indices
* src: set default config as node.config.json
nodejs/node#57171
* src: update std::vector<v8::Local<T>> to use v8::LocalVector<T>
nodejs/node#57578
* test: disable chmod tests failing in Docker
nodejs/node#58326
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 3, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 4, 2025
* chore: bump node in DEPS to v22.16.0
* crypto: remove BoringSSL dh-primes addition
nodejs/node#57023
* tools: enable linter in test/fixtures/test\-runner/output
nodejs/node#57698
* src: improve thread safety of TaskQueue
nodejs/node#57910
* buffer: define global v8::CFunction objects as const
nodejs/node#57676
* src: disable abseil deadlock detection
nodejs/node#57582
* zlib: fix pointer alignment
nodejs/node#57727
* chore: fixup patch indices
* src: set default config as node.config.json
nodejs/node#57171
* src: update std::vector<v8::Local<T>> to use v8::LocalVector<T>
nodejs/node#57578
* test: disable chmod tests failing in Docker
nodejs/node#58326
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Jun 6, 2025
@ghostghost mentioned this pull request Jun 8, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 18, 2025
codebytere added a commit to electron/electron that referenced this pull request Jun 30, 2025
* chore: bump node in DEPS to v22.16.0
* crypto: remove BoringSSL dh-primes addition
nodejs/node#57023
* tools: enable linter in test/fixtures/test\-runner/output
nodejs/node#57698
* src: improve thread safety of TaskQueue
nodejs/node#57910
* buffer: define global v8::CFunction objects as const
nodejs/node#57676
* zlib: fix pointer alignment
nodejs/node#57727
* chore: fixup patch indices
* src: set default config as node.config.json
nodejs/node#57171
* src: update std::vector<v8::Local<T>> to use v8::LocalVector<T>
nodejs/node#57578
* test: disable chmod tests failing in Docker
nodejs/node#58326
* chore: fix out of date patch
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Aug 18, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
marco-ippolito pushed a commit that referenced this pull request Aug 20, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
marco-ippolito pushed a commit that referenced this pull request Aug 23, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
marco-ippolito pushed a commit that referenced this pull request Aug 25, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
marco-ippolito pushed a commit that referenced this pull request Aug 27, 2025
The function AllocForBrotli prefixes the allocated memory with its
size, and returns a pointer to the region after it. This pointer can
however no longer be suitably aligned. Correct this by allocating
the maximum of the the size of the size_t and the max alignment.
On Arm 32bits the size_t is 4 bytes long, but the alignment is 8 for
some NEON instructions. When Brotli is compiled with optimizations
enabled newer GCC versions will use the NEON instructions and trigger
a bus error killing node.
see google/brotli#1159
PR-URL: #57727
Reviewed-By: Shelley Vohr <shelley.vohr@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Daniel Lemire <daniel@lemire.me>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
* chore: bump node in DEPS to v22.16.0
* crypto: remove BoringSSL dh-primes addition
nodejs/node#57023
* tools: enable linter in test/fixtures/test\-runner/output
nodejs/node#57698
* src: improve thread safety of TaskQueue
nodejs/node#57910
* buffer: define global v8::CFunction objects as const
nodejs/node#57676
* src: disable abseil deadlock detection
nodejs/node#57582
* zlib: fix pointer alignment
nodejs/node#57727
* chore: fixup patch indices
* src: set default config as node.config.json
nodejs/node#57171
* src: update std::vector<v8::Local<T>> to use v8::LocalVector<T>
nodejs/node#57578
* test: disable chmod tests failing in Docker
nodejs/node#58326
---------
Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++Issues and PRs that require attention from people who are familiar with C++.needs-ciPRs that need a full CI run.zlibIssues and PRs related to the zlib subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@jhofstee@targos@nodejs-github-bot@richardlau@lemire@codebytere@tniessen@Flarna