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: guard against undefined behavior #34746

Closed
wants to merge 13 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Aug 12, 2020

Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

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

Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.
@ronag ronag requested review from jasnell and addaleax August 12, 2020 10:32
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 12, 2020
@ronag ronag force-pushed the safe-file-handle branch 8 times, most recently from 03ee3d5 to bc670bd Compare August 12, 2020 11:11
@ronag ronag force-pushed the safe-file-handle branch 2 times, most recently from 6169687 to 107f508 Compare August 12, 2020 11:15
@ronag ronag force-pushed the safe-file-handle branch 4 times, most recently from 4a3099a to 711653a Compare August 12, 2020 21:05
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 12, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Aug 12, 2020

@nodejs/fs

@jasnell
Copy link
Member

jasnell commented Aug 12, 2020

@addaleax ... This brought an edge case to mind that I'm not sure how we're handling...

What should happen in the following case

'use strict';

const fs = require('fs');
const { Worker, workerData, isMainThread } = require('worker_threads');

if (isMainThread) {
  async function foo() {
    const fh = await fs.promises.open(__filename);
    const buffer = Buffer.alloc(1000);

    // Not awaiting...
    const p = fh.read(buffer, 0, 1000);

    // Transferring the FileHandle while there's potentially
    // an outstanding async operation on it.
    const w = new Worker(__filename, {
      workerData: fh,
      transferList: [fh]
    });

    // Then awaiting...
    await p;
    return buffer.toString();
  }
  foo().then(console.log);
} else {
  workerData.close();
}

First immediate thought is that we should throw if attempting to transfer a FileHandle that has pending operations.

@addaleax
Copy link
Member

First immediate thought is that we should throw if attempting to transfer a FileHandle that has pending operations.

Yeah, I agree – that sounds like the best approach to me 👍

@nodejs-github-bot
Copy link
Collaborator

@jasnell
Copy link
Member

jasnell commented Aug 13, 2020

It would likely be good to add a comment to the fs.md documentation for FileHandle.close() that pending operations will be completed.

Co-authored-by: James M Snell <jasnell@gmail.com>
@ronag
Copy link
Member Author

ronag commented Aug 13, 2020

It would likely be good to add a comment to the fs.md documentation for FileHandle.close() that pending operations will be completed.

Fixed

Co-authored-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 13, 2020

addaleax added a commit to addaleax/node that referenced this pull request Aug 13, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: nodejs#34746 (comment)
@ronag
Copy link
Member Author

ronag commented Aug 14, 2020

Landed in a0f87aa

@ronag ronag closed this Aug 14, 2020
ronag added a commit that referenced this pull request Aug 14, 2020
Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

PR-URL: #34746
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Aug 17, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: #34746 (comment)

PR-URL: #34766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Aug 17, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: nodejs#34746 (comment)

PR-URL: nodejs#34766
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

PR-URL: #34746
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 18, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: #34746 (comment)

PR-URL: #34766
Backport-PR-URL: #34814
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
Calling close on a file description which is currently in use is
undefined behavior due to implementation details in libuv. Add
a guard against this when using FileHandle.

PR-URL: #34746
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: #34746 (comment)

PR-URL: #34766
Backport-PR-URL: #34814
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 27, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: #34746 (comment)

PR-URL: #34766
Backport-PR-URL: #34814
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit that referenced this pull request Sep 28, 2020
This can currently be triggered when posting a closing FileHandle.

Refs: #34746 (comment)

PR-URL: #34766
Backport-PR-URL: #34814
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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.

5 participants