Skip to content

buffer: truncate instead of throw when writing beyond buffer - #54524

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:buffer-write-length-oob
Aug 27, 2024
Merged

buffer: truncate instead of throw when writing beyond buffer#54524
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:buffer-write-length-oob

Conversation

@ronag

@ronagronag commented Aug 23, 2024

Copy link
Copy Markdown
Member

Fixes: #54523

@nodejs-github-botnodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Aug 23, 2024
@ronag
ronagforce-pushed the buffer-write-length-oob branch from e785336 to 9c112d9CompareAugust 23, 2024 13:44
@avivkeller

avivkeller commented Aug 23, 2024•
edited
Loading

Copy link
Copy Markdown
Member

@ronag I marked #54523 as a duplicate of #54521 and #54518, does this issue fix one of those too? (It looks like it might fix #54518, right?)

avivkeller
avivkeller reviewed Aug 23, 2024
View reviewed changes
Comment threadtest/parallel/test-buffer-write.js Outdated
ronag
ronag commented Aug 23, 2024
View reviewed changes
Comment threadtest/parallel/test-buffer-write.js Outdated
@ronag

ronag commented Aug 23, 2024

Copy link
Copy Markdown
MemberAuthor

#54521 is not related to this

@ronag
ronagforce-pushed the buffer-write-length-oob branch from 615fe78 to ff17b6bCompareAugust 23, 2024 14:05
@ronagronag added the fast-track PRs that do not need to wait for 48 hours to land. label Aug 23, 2024
@github-actions

github-actionsBot commented Aug 23, 2024

Copy link
Copy Markdown
Contributor

Fast-track has been requested by @ronag. Please 👍 to approve.

@ronag
ronagforce-pushed the buffer-write-length-oob branch from ff17b6b to e94a7f2CompareAugust 23, 2024 14:07
@ronag
ronagforce-pushed the buffer-write-length-oob branch from e94a7f2 to f6f2334CompareAugust 23, 2024 14:07
@ronag
ronagforce-pushed the buffer-write-length-oob branch from f6f2334 to 9b0a26aCompareAugust 23, 2024 14:08
@avivkeller

avivkeller commented Aug 23, 2024

Copy link
Copy Markdown
Member

#54521 is not related to this

But does it fix #54518?

@avivkelleravivkeller mentioned this pull request Aug 23, 2024
Closed
@ronag

ronag commented Aug 23, 2024

Copy link
Copy Markdown
MemberAuthor

#54521 is not related to this

But does it fix #54518?

I think so.

Ethan-Arrowood
Ethan-Arrowood approved these changes Aug 23, 2024
View reviewed changes

@Ethan-ArrowoodEthan-Arrowood left a comment

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.

I'm okay with this, but could it be considered a major, breaking change?

@ronag

ronag commented Aug 23, 2024

Copy link
Copy Markdown
MemberAuthor

It's unbrraking.

@codecov

codecovBot commented Aug 23, 2024•
edited
Loading

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 87.34%. Comparing base (628469c) to head (fcb2906).
Report is 340 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/buffer.js33.33%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #54524 +/- ##
==========================================
- Coverage 87.34% 87.34% -0.01% 
==========================================
Files 649 649 Lines 182576 182626 +50 Branches 35036 35040 +4 ==========================================
+ Hits 159467 159507 +40 - Misses 16381 16390 +9 - Partials 6728 6729 +1
Files with missing linesCoverage Δ
lib/internal/buffer.js97.41% <33.33%> (+0.26%)⬆️

... and 37 files with indirect coverage changes

@jerome-benoit

jerome-benoit commented Aug 23, 2024•
edited
Loading

Copy link
Copy Markdown
Contributor

It's unbrraking.

Of course it is: the semantic of dependent APIs is changed. But the initial upper bound check introduction should not have go without being flagged as a breaking change.

@avivkelleravivkeller mentioned this pull request Aug 26, 2024
Closed
@nodejs-github-bot

nodejs-github-bot commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/61523/

@ronagronag added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Aug 27, 2024
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 27, 2024
@nodejs-github-bot
nodejs-github-bot merged commit dd8eeec into nodejs:mainAug 27, 2024
@nodejs-github-bot

nodejs-github-bot commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

Landed in dd8eeec

haines added a commit to cerbos/cerbos-sdk-javascript that referenced this pull request Aug 27, 2024
@haines
Revert to Node.js 22.6.0
209de9b
nodejs/node#54524
Signed-off-by: Andrew Haines <haines@cerbos.dev>
haines added a commit to cerbos/cerbos-sdk-javascript that referenced this pull request Aug 27, 2024
@renovate@haines
chore(deps-dev): update development dependencies (#993)
c3c0690
* chore(deps-dev): update development dependencies
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Revert to Node.js 22.6.0
nodejs/node#54524
Signed-off-by: Andrew Haines <haines@cerbos.dev>
---------
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Andrew Haines <haines@cerbos.dev>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Andrew Haines <haines@cerbos.dev>
ronag added a commit to nxtedition/node that referenced this pull request Aug 27, 2024
@ronag
buffer: throw when writing beyond buffer"
2edea24
This reverts commit dd8eeec.
PR-URL: nodejs#54588
Refs: nodejs#54524
@Wazbat

Wazbat commented Aug 27, 2024

Copy link
Copy Markdown

I'm unfamiliar with the node release process. Is there a rough estimate on when this will be released?

@RafaelGSS

RafaelGSS commented Aug 27, 2024

Copy link
Copy Markdown
Member

I'm unfamiliar with the node release process. Is there a rough estimate on when this will be released?

It should go out tomorrow. Follow #54560

@owmo-devowmo-dev mentioned this pull request Aug 27, 2024
Closed
RafaelGSS pushed a commit that referenced this pull request Aug 30, 2024
@ronag@RafaelGSS
buffer: truncate instead of throw when writing beyond buffer
ac178b0
Fixes: #54523Fixes: #54518
PR-URL: #54524
Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
haines added a commit to cerbos/cerbos-sdk-javascript that referenced this pull request Sep 2, 2024
@haines
Revert to Node.js 22.6.0
f15712b
nodejs/node#54524
Signed-off-by: Andrew Haines <haines@cerbos.dev>
haines added a commit to cerbos/cerbos-sdk-javascript that referenced this pull request Sep 2, 2024
@renovate@haines
chore(deps-dev): update development dependencies (#996)
2730b40
* chore(deps-dev): update development dependencies
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Fix lint errors
Signed-off-by: Andrew Haines <haines@cerbos.dev>
* Revert to Node.js 22.6.0
nodejs/node#54524
Signed-off-by: Andrew Haines <haines@cerbos.dev>
---------
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Andrew Haines <haines@cerbos.dev>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Andrew Haines <haines@cerbos.dev>
@wavedeckwavedeck mentioned this pull request Sep 6, 2024
Closed
jasnell pushed a commit that referenced this pull request Sep 28, 2024
@ronag@jasnell
buffer: throw when writing beyond buffer"
3800d60
This reverts commit dd8eeec.
PR-URL: #54588
Refs: #54524
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bugarelabugarela mentioned this pull request Nov 11, 2024
Closed
This was referenced Jun 26, 2025
Closed
Closed
@dcodeIOdcodeIO mentioned this pull request May 8, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@avivkelleravivkelleravivkeller left review comments
@mcollinamcollinamcollina approved these changes
@addaleaxaddaleaxaddaleax approved these changes
@BridgeARBridgeARBridgeAR approved these changes
@Ethan-ArrowoodEthan-ArrowoodEthan-Arrowood approved these changes
@RafaelGSSRafaelGSSRafaelGSS approved these changes
@targostargosAwaiting requested review from targos
@benjamingrbenjamingrAwaiting requested review from benjamingr

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.bufferIssues and PRs related to the buffer subsystem.fast-trackPRs that do not need to wait for 48 hours to land.needs-ciPRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

latest release error - RangeError: "length" is outside of buffer bounds

12 participants

@ronag@avivkeller@jerome-benoit@nodejs-github-bot@mcollina@Wazbat@RafaelGSS@addaleax@benjamingr@BridgeAR@Ethan-Arrowood@targos