fs: unset FileHandle fd after close

addaleax · codebytere · commit 32ac1be372b7 · 2020-02-17T10:28:00.000-08:00
- 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>
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -52,21 +52,23 @@ const pathModule = require('path');
 const { promisify } = require('internal/util');
 
 const kHandle = Symbol('kHandle');
+const kFd = Symbol('kFd');
 const { kUsePromises } = binding;
 
 const getDirectoryEntriesPromise = promisify(getDirents);
 
 class FileHandle {
   constructor(filehandle) {
     this[kHandle] = filehandle;
+    this[kFd] = filehandle.fd;
   }
 
   getAsyncId() {
     return this[kHandle].getAsyncId();
   }
 
   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
@@ -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<Object> obj) {
                             .ToLocal(&obj)) {
     return nullptr;
   }
-  PropertyAttribute attr =
-      static_cast<PropertyAttribute>(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<Value>& 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
@@ -205,7 +205,7 @@ class FileHandle final : public AsyncWrap, public StreamBase {
 
   static void New(const v8::FunctionCallbackInfo<v8::Value>& 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
@@ -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());

Original file line number	Diff line number	Diff line change
`@@ -52,21 +52,23 @@ const pathModule = require('path');`
`52`	`52`	`const { promisify } = require('internal/util');`
`53`	`53`
`54`	`54`	`const kHandle = Symbol('kHandle');`
	`55`	`+const kFd = Symbol('kFd');`
`55`	`56`	`const { kUsePromises } = binding;`
`56`	`57`
`57`	`58`	`const getDirectoryEntriesPromise = promisify(getDirents);`
`58`	`59`
`59`	`60`	`class FileHandle {`
`60`	`61`	`constructor(filehandle) {`
`61`	`62`	`this[kHandle] = filehandle;`
	`63`	`+ this[kFd] = filehandle.fd;`
`62`	`64`	`}`
`63`	`65`
`64`	`66`	`getAsyncId() {`
`65`	`67`	`return this[kHandle].getAsyncId();`
`66`	`68`	`}`
`67`	`69`
`68`	`70`	`get fd() {`
`69`		`- return this[kHandle].fd;`
	`71`	`+ return this[kFd];`
`70`	`72`	`}`
`71`	`73`
`72`	`74`	`appendFile(data, options) {`
`@@ -122,6 +124,7 @@ class FileHandle {`
`122`	`124`	`}`
`123`	`125`
`124`	`126`	`close = () => {`
	`127`	`+ this[kFd] = -1;`
`125`	`128`	`return this[kHandle].close();`
`126`	`129`	`}`
`127`	`130`	`}`