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.

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

fs: fix validation of negative offset to avoid abort #38421

Closed

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 26, 2021

Signed-off-by: James M Snell [email protected]
Fixes: #24640

@github-actions github-actions bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Apr 26, 2021
@nodejs-github-bot

This comment has been minimized.

@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Apr 26, 2021
@nodejs-github-bot

This comment has been minimized.

@Linkgoron
Copy link
Member

Linkgoron commented Apr 26, 2021

I think that the real issue is only in write as negative offset is already checked in validateOffsetLengthRead, however there is no negative check in validateOffsetLengthWrite.

Note that the check for length is also broken in write, the following aborts for me as well - so maybe it makes sense to fix it in this PR as well:

fs.open('./tst/some-file.file', 'w+', (err, fd) => {
  fs.write(fd, Buffer.from('text'), 0, -10, (err) => {
    console.log(err);
  });
})

@jasnell jasnell removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2021
@jasnell
Copy link
Member Author

jasnell commented Apr 27, 2021

I think that the real issue is only in write as negative offset is already checked

Yeah, I modified the reads here just for consistency.

so maybe it makes sense to fix it in this PR as well

Sigh. Yeah, missed that. Will add that tomorrow. done!

@jasnell jasnell force-pushed the fix-fs-write-abort-on-negative-offset branch from bce3d67 to 837137f Compare April 27, 2021 02:50
@nodejs-github-bot
Copy link
Collaborator

lib/internal/fs/utils.js Outdated Show resolved Hide resolved
Copy link
Contributor

@RaisinTen RaisinTen left a comment

Choose a reason for hiding this comment

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

Would it make sense to also move validateInteger(offset, 'offset', 0); into validateOffsetLengthRead and validateOffsetLengthWrite? That way, we could have just one validation for each field.

Co-authored-by: linkgoron <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Apr 27, 2021

Would it make sense to also move validateInteger(offset, 'offset', 0); into validateOffsetLengthRead and validateOffsetLengthWrite? That way, we could have just one validation for each field.

Perhaps, but let's save that for a separate PR.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 27, 2021

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 27, 2021
@jasnell
Copy link
Member Author

jasnell commented Apr 28, 2021

Landed in 4af15df

@jasnell jasnell closed this Apr 28, 2021
jasnell added a commit that referenced this pull request Apr 28, 2021
Fixes: #24640
Signed-off-by: James M Snell <[email protected]>

PR-URL: #38421
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Apr 29, 2021
Fixes: #24640
Signed-off-by: James M Snell <[email protected]>

PR-URL: #38421
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
@targos targos mentioned this pull request May 3, 2021
targos pushed a commit that referenced this pull request May 30, 2021
Fixes: #24640
Signed-off-by: James M Snell <[email protected]>

PR-URL: #38421
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
Fixes: #24640
Signed-off-by: James M Snell <[email protected]>

PR-URL: #38421
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
Fixes: #24640
Signed-off-by: James M Snell <[email protected]>

PR-URL: #38421
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
targos pushed a commit that referenced this pull request Jun 11, 2021
Fixes: #24640
Signed-off-by: James M Snell <[email protected]>

PR-URL: #38421
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Nitzan Uziely <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

Regression v10.x node_file.cc:1713: Assertion `(off) <= (buffer_length)' failed.
5 participants