fs: guard against undefined behavior by ronag · Pull Request #34746 · nodejs/node · GitHub

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.

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

@nodejs/fs

@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.

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 👍

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

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>

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>

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

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

This can currently be triggered when posting a closing FileHandle. Refs: nodejs#34746 (comment)

Landed in a0f87aa

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>

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>

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>

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>

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>

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>

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>

fs: guard against undefined behavior

ceb22f5

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 requested review from addaleax and jasnell August 12, 2020 10:32

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Aug 12, 2020

ronag added 2 commits August 12, 2020 12:33

fixup

e1380b9

fixup

dc1dcff

ronag force-pushed the safe-file-handle branch 8 times, most recently from 03ee3d5 to bc670bd Compare August 12, 2020 11:11

fixup

0bed9de

ronag force-pushed the safe-file-handle branch 2 times, most recently from 6169687 to 107f508 Compare August 12, 2020 11:15

fixup: ERR_FS_FILE_CLOSED

6a3c7bb

ronag force-pushed the safe-file-handle branch 4 times, most recently from 4a3099a to 711653a Compare August 12, 2020 21:05

fixup

fd5ca60

ronag force-pushed the safe-file-handle branch from 711653a to fd5ca60 Compare August 12, 2020 21:24

addaleax approved these changes Aug 12, 2020

View reviewed changes

ronag added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 12, 2020

jasnell reviewed Aug 13, 2020

View reviewed changes

Comment thread lib/internal/fs/promises.js

jasnell reviewed Aug 13, 2020

View reviewed changes

Comment thread test/parallel/test-worker-message-port-transfer-filehandle.js Outdated

Update test/parallel/test-worker-message-port-transfer-filehandle.js

c1f4245

Co-authored-by: James M Snell <jasnell@gmail.com>

fixup: docs

8ebe12d

jasnell reviewed Aug 13, 2020

View reviewed changes

Comment thread lib/internal/fs/promises.js

Apply suggestions from code review

dcd5ddb

Co-authored-by: James M Snell <jasnell@gmail.com>

ronag added 2 commits August 13, 2020 23:32

fixup: docs

b384f08

fixup: linting

69cc10b

jasnell approved these changes Aug 13, 2020

View reviewed changes

addaleax added a commit to addaleax/node that referenced this pull request Aug 13, 2020

worker: do not crash when JSTransferable lists untransferable value

bf7d850

This can currently be triggered when posting a closing FileHandle. Refs: nodejs#34746 (comment)

addaleax mentioned this pull request Aug 13, 2020

worker: do not crash when JSTransferable lists untransferable value #34766

Closed

3 tasks

ronag closed this Aug 14, 2020

addaleax mentioned this pull request Aug 17, 2020

[v14.x] worker: do not crash when JSTransferable lists untransferable value #34814

Closed

danielleadams mentioned this pull request Aug 20, 2020

V14.9.0 proposal #34852

Merged

fs: guard against undefined behavior by ronag · Pull Request #34746 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: guard against undefined behavior - #34746

fs: guard against undefined behavior#34746
ronag wants to merge 13 commits into
nodejs:masterfrom
nxtedition:safe-file-handle

ronag commented Aug 12, 2020

Uh oh!

nodejs-github-bot commented Aug 12, 2020

Uh oh!

ronag commented Aug 12, 2020

Uh oh!

jasnell commented Aug 12, 2020 •
edited
Loading

Uh oh!

Uh oh!

addaleax commented Aug 13, 2020

Uh oh!

nodejs-github-bot commented Aug 13, 2020

Uh oh!

Uh oh!

Uh oh!

jasnell commented Aug 13, 2020

Uh oh!

ronag commented Aug 13, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 13, 2020

Uh oh!

nodejs-github-bot commented Aug 13, 2020 •
edited by ronag
Loading

Uh oh!

Uh oh!

ronag commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fs: guard against undefined behavior by ronag · Pull Request #34746 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: guard against undefined behavior - #34746

fs: guard against undefined behavior#34746ronag wants to merge 13 commits intonodejs:masternodejs/node:masterfrom nxtedition:safe-file-handlenxtedition/node:safe-file-handleCopy head branch name to clipboard

Conversation

ronag commented Aug 12, 2020

Checklist

Uh oh!

nodejs-github-bot commented Aug 12, 2020

Uh oh!

ronag commented Aug 12, 2020

Uh oh!

jasnell commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Aug 13, 2020

Uh oh!

nodejs-github-bot commented Aug 13, 2020

Uh oh!

Uh oh!

Uh oh!

jasnell commented Aug 13, 2020

Uh oh!

ronag commented Aug 13, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 13, 2020

Uh oh!

nodejs-github-bot commented Aug 13, 2020 • edited by ronag Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Aug 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fs: guard against undefined behavior#34746
ronag wants to merge 13 commits into
nodejs:masterfrom
nxtedition:safe-file-handle

jasnell commented Aug 12, 2020 •
edited
Loading

nodejs-github-bot commented Aug 13, 2020 •
edited by ronag
Loading