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: validate the input data before opening file #31731

Closed
wants to merge 2 commits into from
Closed

fs: validate the input data before opening file #31731

wants to merge 2 commits into from

Conversation

ZYSzys
Copy link
Member

@ZYSzys ZYSzys commented Feb 11, 2020

Now running python tools/test.py test/parallel/test-fs-append-file in our master branch, there was a file called foobar generated, which was caused by this line:

node/test/parallel/test-fs-append-file.js

Line 143 in fb437c4

assert.rejects(fs.promises.appendFile('foobar', data), errObj);

This was because firstly we opened foobar file and then validating the input data in our fs.promise module.

So I think the more reasonable method is validating the input data first and then opening the file, to be consistent with fs module.

Refs: #31030

/cc @BridgeAR

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

@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 11, 2020
@nodejs-github-bot
Copy link
Collaborator

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

BridgeAR
BridgeAR approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

Thanks, this slipped through! LGTM % comment

test/parallel/test-fs-append-file.js Outdated Show resolved Hide resolved
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 11, 2020
@nodejs-github-bot
Copy link
Collaborator

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

ZYSzys added a commit that referenced this pull request Feb 13, 2020
@ZYSzys
fs: validate the input data before opening file
3e9302b 
PR-URL: #31731
Refs: #31030
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@ZYSzys
Copy link
Member Author

ZYSzys commented Feb 13, 2020

Landed in 3e9302b.

@ZYSzys ZYSzys closed this Feb 13, 2020
@ZYSzys ZYSzys deleted the fs-promise-validate-before-open branch February 13, 2020 13:48
@codebytere
Copy link
Member

@lundibundi should this be backported to v13.x? it'll need a manual if yes.

@lundibundi
Copy link
Member

/cc @ZYSzys ^

@ZYSzys
Copy link
Member Author

ZYSzys commented Feb 19, 2020

I think no, since this fixed the bug caused by #31030 (which is semver-major).

@BethGriggs BethGriggs mentioned this pull request Apr 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lundibundi lundibundi lundibundi approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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.
8 participants
@ZYSzys @nodejs-github-bot @codebytere @lundibundi @addaleax @lpinca @cjihrig @BridgeAR