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 pre-aborted writeFile AbortSignal file leak #37393

Merged
merged 1 commit into from
Feb 26, 2021

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Feb 16, 2021

This fixes an issue in promises.writeFile where a file is opened, and not closed if the abort signal is aborted before/during the file was opened.

A minor thing is that fd.close can still throw, I'm not sure if it's an issue or not.

In addition I also added validation for the AbortSignal, and a check where if the signal was already aborted, the file won't be opened at all.

  • [] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • 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 16, 2021
@aduh95
Copy link
Contributor

aduh95 commented Feb 16, 2021

Could you add a test? Also, do you know if it also happens on fs.promises.readFile?

@Linkgoron
Copy link
Member Author

Linkgoron commented Feb 16, 2021

Could you add a test?

Sure

do you know if it also happens on fs.promises.readFile?

It doesn't, however I could add the "optimization" that I added here (the file still opens, even if the signal is already aborted).

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

good catch

@benjamingr
Copy link
Member

(Please add a test)

@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch 2 times, most recently from 6e0162c to dd95fb7 Compare February 18, 2021 18:59
@Linkgoron
Copy link
Member Author

Linkgoron commented Feb 18, 2021

@aduh95 @benjamingr I've added tests. I'm not 100% happy about them, as it's hard for me to know if they'll work consistently on other platforms... However, on my machine they do work consistently on this branch, and fail consistently on master. I'd be happy to improve them if you have any pointers (The tests are essentially copy/pasted from my other fs PR, just changed for the promisified version).

EDIT: clearly doesn't work... I'll try to find another solution.

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

If making a robust test is too tricky, I think we should land this without one.

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch from dd95fb7 to 9778ea3 Compare February 20, 2021 18:03
@Linkgoron
Copy link
Member Author

removed test changes

@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch from 9778ea3 to 758d27a Compare February 20, 2021 18:13
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

lgtm without the test due to the circumstances.

@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch from 758d27a to edd2871 Compare February 20, 2021 20:51
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch 2 times, most recently from f828fa8 to 76892da Compare February 20, 2021 21:47
@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch from 76892da to 936160f Compare February 20, 2021 22:54
@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 20, 2021
@nodejs-github-bot
Copy link
Collaborator

@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch 2 times, most recently from f14faab to a95971f Compare February 23, 2021 02:05
@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch from a95971f to 6425b00 Compare February 23, 2021 13:05
@Linkgoron Linkgoron force-pushed the fs-write-file-abort-signal-leak branch from 6425b00 to 39e50ac Compare February 25, 2021 15:43
@nodejs-github-bot
Copy link
Collaborator

Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.

PR-URL: nodejs#37393
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 force-pushed the fs-write-file-abort-signal-leak branch from 39e50ac to c4180b7 Compare February 26, 2021 15:29
@aduh95
Copy link
Contributor

aduh95 commented Feb 26, 2021

Landed in c4180b7

@aduh95 aduh95 merged commit c4180b7 into nodejs:master Feb 26, 2021
@Linkgoron Linkgoron deleted the fs-write-file-abort-signal-leak branch February 26, 2021 16:24
targos pushed a commit that referenced this pull request Feb 28, 2021
Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.

PR-URL: #37393
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.

PR-URL: nodejs#37393
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.

PR-URL: nodejs#37393
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.

PR-URL: nodejs#37393
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Apr 30, 2021
Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.

PR-URL: #37393
Backport-PR-URL: #38386
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos targos added backported-to-v14.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v14.x labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants