From 32ac1be372b7299316bc6e60a83edd4effbc33fa Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 16 Jan 2020 22:12:07 +0100
Subject: [PATCH] fs: unset FileHandle fd after close

- 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: https://github.com/nodejs/node/issues/31361

PR-URL: https://github.com/nodejs/node/pull/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>
---
 lib/internal/fs/promises.js                   |  5 +++-
 src/node_file.cc                              | 16 +++---------
 src/node_file.h                               |  2 +-
 .../test-fs-filehandle-use-after-close.js     | 25 +++++++++++++++++++
 4 files changed, 33 insertions(+), 15 deletions(-)
 create mode 100644 test/parallel/test-fs-filehandle-use-after-close.js

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 65e4fdb63c7ecb..d999338adcf789 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 c130712f97dc7f..fbab726afb4fa0 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<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
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<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
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());