Skip to content

Conversation

jhofstee
Copy link
Contributor

@jhofstee jhofstee commented Apr 2, 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

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-bot nodejs-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
Member

targos commented Apr 2, 2025

/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/

Copy link

codecov bot commented Apr 2, 2025

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 lines Coverage Δ
src/node_zlib.cc 78.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.

Copy link
Member

@lemire lemire left a comment

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
Contributor Author

jhofstee commented Apr 3, 2025

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.

@Flarna Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 9, 2025
@nodejs-github-bot nodejs-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.

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 9, 2025
@nodejs-github-bot
Copy link
Collaborator

@richardlau richardlau 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-bot nodejs-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.

@richardlau richardlau 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-bot nodejs-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 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
@ghost ghost 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 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 26, 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>
Sign up for free to 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-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants