Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: account for buffer alloc failure in readFile #16219

Closed
wants to merge 3 commits into from
Closed

fs: account for buffer alloc failure in readFile #16219

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

Ref: #15362

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs

@addaleax addaleax added fs Issues and PRs related to the fs subsystem / file system. lts-watch-v6.x labels Oct 15, 2017
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 15, 2017
@addaleax
Copy link
Member Author

addaleax commented Oct 15, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-commit/13157/
CI with V8 6.2: https://ci.nodejs.org/job/node-test-commit/13158/

@addaleax addaleax force-pushed the fs-readfile-buffer-alloc-fail branch from 405e2bf to 6334528 Compare October 15, 2017 11:44
bnoordhuis
bnoordhuis reviewed Oct 15, 2017
View reviewed changes
lib/fs.js Outdated
if (context.encoding) {
return tryToString(buffer, context.encoding, callback);
if (context.encoding) {
return tryToString(buffer, context.encoding, callback);
Copy link
Member

@bnoordhuis bnoordhuis Oct 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

retryToString() should be moved outside the try/catch block, it has a try/catch block of its own.

edit: what I mean is, tryToString() calls callback if buf.toString() fails. If callback throws and you catch the exception here, it's invoked again, this time with its own exception.

Copy link
Member Author

Choose a reason for hiding this comment

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

right, thanks for pointing this out – I’ve just eliminated tryToString altogether

@addaleax addaleax mentioned this pull request Oct 16, 2017
Closed
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Oct 19, 2017
@addaleax @BridgeAR
fs: account for buffer alloc failure in readFile
5cde451 
PR-URL: nodejs#16219
Refs: nodejs#15362
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@BridgeAR
Copy link
Member

Landed in 5cde451

@BridgeAR BridgeAR closed this Oct 19, 2017
@addaleax addaleax deleted the fs-readfile-buffer-alloc-fail branch October 22, 2017 18:05
MylesBorins pushed a commit that referenced this pull request Oct 23, 2017
@addaleax @MylesBorins
fs: account for buffer alloc failure in readFile
62f802a 
PR-URL: #16219
Refs: #15362
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
addaleax added a commit to ayojs/ayo that referenced this pull request Oct 26, 2017
@addaleax
fs: account for buffer alloc failure in readFile
9191c90 
PR-URL: nodejs/node#16219
Refs: nodejs/node#15362
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

addaleax added a commit to ayojs/ayo that referenced this pull request Dec 7, 2017
@addaleax
fs: account for buffer alloc failure in readFile
e16cde4 
PR-URL: nodejs/node#16219
Refs: nodejs/node#15362
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

@cjihrig cjihrig cjihrig approved these changes

@evanlucas evanlucas evanlucas approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@addaleax @BridgeAR @MylesBorins @bnoordhuis @jasnell @evanlucas @cjihrig @gireeshpunathil @nodejs-github-bot