diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 9750c4f1453713..cc907c8c9eae68 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -52,6 +52,7 @@ const pathModule = require('path'); const { promisify } = require('internal/util'); const kHandle = Symbol('kHandle'); +const kFd = Symbol('kFd'); const { kUsePromises } = binding; const getDirectoryEntriesPromise = promisify(getDirents); @@ -59,6 +60,7 @@ const getDirectoryEntriesPromise = promisify(getDirents); class FileHandle { constructor(filehandle) { this[kHandle] = filehandle; + this[kFd] = filehandle.fd; } getAsyncId() { @@ -66,7 +68,7 @@ class FileHandle { } get fd() { - return this[kHandle].fd; + return this[kFd]; } appendFile(data, options) { @@ -122,6 +124,7 @@ class FileHandle { } close = () => { + this[kFd] = -1; return this[kHandle].close(); } } diff --git a/src/node_file.cc b/src/node_file.cc index 23af6abd1e510b..f59cbf1fbb6cdf 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -53,7 +53,6 @@ namespace fs { using v8::Array; using v8::Context; -using v8::DontDelete; using v8::EscapableHandleScope; using v8::Function; using v8::FunctionCallbackInfo; @@ -68,8 +67,6 @@ using v8::Number; using v8::Object; using v8::ObjectTemplate; using v8::Promise; -using v8::PropertyAttribute; -using v8::ReadOnly; using v8::String; using v8::Symbol; using v8::Uint32; @@ -138,15 +135,6 @@ FileHandle* FileHandle::New(Environment* env, int fd, Local obj) { .ToLocal(&obj)) { return nullptr; } - PropertyAttribute attr = - static_cast(ReadOnly | DontDelete); - if (obj->DefineOwnProperty(env->context(), - env->fd_string(), - Integer::New(env->isolate(), fd), - attr) - .IsNothing()) { - return nullptr; - } return new FileHandle(env, obj, fd); } @@ -190,12 +178,13 @@ inline void FileHandle::Close() { uv_fs_t req; int ret = uv_fs_close(env()->event_loop(), &req, fd_, nullptr); uv_fs_req_cleanup(&req); - AfterClose(); struct err_detail { int ret; int fd; }; err_detail detail { ret, fd_ }; + AfterClose(); + if (ret < 0) { // Do not unref this env()->SetImmediate([detail](Environment* env) { @@ -340,6 +329,7 @@ void FileHandle::ReleaseFD(const FunctionCallbackInfo& args) { void FileHandle::AfterClose() { closing_ = false; closed_ = true; + fd_ = -1; if (reading_ && !persistent().IsEmpty()) EmitRead(UV_EOF); } diff --git a/src/node_file.h b/src/node_file.h index 1042baaf8f736b..0574aa8f6c079d 100644 --- a/src/node_file.h +++ b/src/node_file.h @@ -205,7 +205,7 @@ class FileHandle final : public AsyncWrap, public StreamBase { static void New(const v8::FunctionCallbackInfo& args); - int fd() const { return fd_; } + int GetFD() override { return fd_; } // Will asynchronously close the FD and return a Promise that will // be resolved once closing is complete. diff --git a/test/parallel/test-fs-filehandle-use-after-close.js b/test/parallel/test-fs-filehandle-use-after-close.js new file mode 100644 index 00000000000000..7b99e41cd02284 --- /dev/null +++ b/test/parallel/test-fs-filehandle-use-after-close.js @@ -0,0 +1,25 @@ +'use strict'; +const common = require('../common'); +const assert = require('assert'); +const fs = require('fs').promises; + +(async () => { + const filehandle = await fs.open(__filename); + + assert.notStrictEqual(filehandle.fd, -1); + await filehandle.close(); + assert.strictEqual(filehandle.fd, -1); + + // Open another file handle first. This would typically receive the fd + // that `filehandle` previously used. In earlier versions of Node.js, the + // .stat() call would then succeed because it still used the original fd; + // See https://github.com/nodejs/node/issues/31361 for more details. + const otherFilehandle = await fs.open(process.execPath); + + assert.rejects(() => filehandle.stat(), { + code: 'EBADF', + syscall: 'fstat' + }); + + await otherFilehandle.close(); +})().then(common.mustCall());