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: async fstat after close does not cause EBADF #31361

Closed
bengl opened this issue Jan 14, 2020 · 2 comments
Closed

fs: async fstat after close does not cause EBADF #31361

bengl opened this issue Jan 14, 2020 · 2 comments
Labels
fs Issues and PRs related to the fs subsystem / file system. question Issues that look for answers.

Comments

@bengl
Copy link
Member

bengl commented Jan 14, 2020

  • Version: >= 11.7.0
  • Platform: Darwin $computername 18.7.0 Darwin Kernel Version 18.7.0: Tue Aug 20 16:57:14 PDT 2019; root:xnu-4903.271.2~2/RELEASE_X86_64 x86_64 i386 MacBookPro15,2 Darwin
  • Subsystem: fs

In versions of node greater than or equal to v11.6.0, calling fstat or equivalent (but not fstatSync) on an already-closed file descriptor will not produce an error as it should, but only if something happens in between the close and the fstat (I'm not yet sure if it matters what that is, but I'm using a console.log in my examples). This happen with both the regular fs.fstat function and also with FileHandle#stat.

Either of these two examples will demonstrate the issue:

const fs = require('fs').promises;

(async () => {
  const filehandle = await fs.open(__filename);
  await filehandle.close();
  // console.log('hello world'); // Uncomment this line and EBADF never happens
  await filehandle.stat();
  console.log('we should never get here due to EBADF');
})();
// Note that rewriting this without async/await results in the same thing
const fs = require('fs');

fs.open(__filename, (err, fd) => {
  if (err) throw err;
  fs.close(fd, (err) => {
    if (err) throw err;
    // console.log('hello world'); // Uncomment this line and EBADF never happens
    fs.fstat(fd, (err) => {
      if (err) throw err;
      console.log('we should never get here due to EBADF');
    });
  });
});

The expected behavior here is that we get an EBADF when fstat is called. This happens as it should with these examples as-is. If you uncomment the "hello world" line in either example, it proceeds without error, which is the incorrect behavior.

I was not able to reproduce this with fstatSync. I've narrow this down to having first occurred in Node v11.7.70, and it seems to affect all versions after that, including all releases of Node 12 and 13.

@bengl bengl changed the title fs: async stat after close does not cause EBADF fs: async fstat after close does not cause EBADF Jan 14, 2020
@bengl bengl added the fs Issues and PRs related to the fs subsystem / file system. label Jan 14, 2020
@addaleax
Copy link
Member

@bengl The console.log() call lazily loads process.stdout which in turn initializes a libuv TTY handle, which re-opens the TTY file descriptor. This happens to receive the same fd number as the previously opened file, thus making the fstat() call succeed.

I don’t think there’s much we can do about that, besides maybe setting the fd associated with a file handle to -1 or null in the FileHandle/promise case.

You can tell the difference by adding an extra console.log() call before the script starts – that way the EBADF error shows up as expected.

@addaleax addaleax added the question Issues that look for answers. label Jan 15, 2020
addaleax added a commit to addaleax/node that referenced this issue Jan 16, 2020
- Do not set the fd as a property on the native object.
- Use the already-existent `GetFD()` method to pass the
  fd from C++ to JS.
- Cache the fd in JS to avoid repeated accesses to the
  C++ getter.
- Set the fd to `-1` after close, thus reliably making
  subsequent calls using the `FileHandle` return `EBADF`.

Fixes: nodejs#31361
@addaleax
Copy link
Member

addaleax commented Jan 18, 2020

Landed Sorry, fixed in 7864c53

addaleax added a commit that referenced this issue Jan 18, 2020
- Do not set the fd as a property on the native object.
- Use the already-existent `GetFD()` method to pass the
  fd from C++ to JS.
- Cache the fd in JS to avoid repeated accesses to the
  C++ getter.
- Set the fd to `-1` after close, thus reliably making
  subsequent calls using the `FileHandle` return `EBADF`.

Fixes: #31361

PR-URL: #31389
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Feb 17, 2020
- Do not set the fd as a property on the native object.
- Use the already-existent `GetFD()` method to pass the
  fd from C++ to JS.
- Cache the fd in JS to avoid repeated accesses to the
  C++ getter.
- Set the fd to `-1` after close, thus reliably making
  subsequent calls using the `FileHandle` return `EBADF`.

Fixes: #31361

PR-URL: #31389
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 14, 2020
- Do not set the fd as a property on the native object.
- Use the already-existent `GetFD()` method to pass the
  fd from C++ to JS.
- Cache the fd in JS to avoid repeated accesses to the
  C++ getter.
- Set the fd to `-1` after close, thus reliably making
  subsequent calls using the `FileHandle` return `EBADF`.

Fixes: #31361

PR-URL: #31389
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
codebytere pushed a commit that referenced this issue Mar 17, 2020
- Do not set the fd as a property on the native object.
- Use the already-existent `GetFD()` method to pass the
  fd from C++ to JS.
- Cache the fd in JS to avoid repeated accesses to the
  C++ getter.
- Set the fd to `-1` after close, thus reliably making
  subsequent calls using the `FileHandle` return `EBADF`.

Fixes: #31361

PR-URL: #31389
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
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. question Issues that look for answers.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants