GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||
GeezerVison is showing github.com — zlib: fix pointer alignment by jhofstee · Pull Request #57727 · nodejs/node Conversation
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. I don't think there is any additional test needed, since existing test will crash node already.
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page. All reactions
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
Member
All reactions
Sorry, something went wrong. Uh oh!
There was an error while loading. Please reload this page.
3 of 6 tasks
codebytere
approved these changes
3 tasks done
|
Uh oh!
There was an error while loading. Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
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
🚀 New features to boost your workflow:
|
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
tniessen
approved these changes
Apr 2, 2025
lemire
approved these changes
Apr 2, 2025
lemire
left a comment
Member
There was a problem hiding this 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.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Contributor
Author
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Flarna
reviewed
Apr 4, 2025
Uh oh!
There was an error while loading. Please reload this page.
Flarna
approved these changes
Apr 9, 2025
This comment was marked as outdated.
This comment was marked as outdated.
Collaborator
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]>
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 <[email protected]>
codebytere
added a commit
to electron/electron
that referenced
this pull request
Jun 6, 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 <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Daniel Lemire <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]>
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 <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.