From dd95fb7eb6e744297f15bb44f621405ef896ef9d Mon Sep 17 00:00:00 2001
From: Nitzan Uziely <nitzan@testim.io>
Date: Tue, 16 Feb 2021 15:18:51 +0200
Subject: [PATCH] fs: fix pre-aborted writeFile AbortSignal file leak

Fix an issue in writeFile where a file is opened, and not closed
if the abort signal is aborted after the file was opened
but before writing began.
---
 lib/internal/fs/promises.js                 | 11 +++++
 test/parallel/test-fs-promises-writefile.js | 51 +++++++++++++++++++--
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 6c39a13349d27b8..8ffc92ff45c909b 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -66,6 +66,7 @@ const {
 const { opendir } = require('internal/fs/dir');
 const {
   parseFileMode,
+  validateAbortSignal,
   validateBoolean,
   validateBuffer,
   validateInteger,
@@ -668,11 +669,17 @@ async function writeFile(path, data, options) {
     data = Buffer.from(data, options.encoding || 'utf8');
   }
 
+  validateAbortSignal(options.signal);
   if (path instanceof FileHandle)
     return writeFileHandle(path, data, options.signal);
 
+  if (options.signal?.aborted) {
+    throw lazyDOMException('The operation was aborted', 'AbortError');
+  }
+
   const fd = await open(path, flag, options.mode);
   if (options.signal?.aborted) {
+    await fd.close();
     throw lazyDOMException('The operation was aborted', 'AbortError');
   }
   return PromisePrototypeFinally(writeFileHandle(fd, data), fd.close);
@@ -692,6 +699,10 @@ async function readFile(path, options) {
   if (path instanceof FileHandle)
     return readFileHandle(path, options);
 
+  if (options.signal?.aborted) {
+    throw lazyDOMException('The operation was aborted', 'AbortError');
+  }
+
   const fd = await open(path, flag, 0o666);
   return PromisePrototypeFinally(readFileHandle(fd, options), fd.close);
 }
diff --git a/test/parallel/test-fs-promises-writefile.js b/test/parallel/test-fs-promises-writefile.js
index 7fbe12dda4dc2da..6976a85da1000a5 100644
--- a/test/parallel/test-fs-promises-writefile.js
+++ b/test/parallel/test-fs-promises-writefile.js
@@ -2,6 +2,8 @@
 
 const common = require('../common');
 const fs = require('fs');
+const os = require('os');
+const cp = require('child_process');
 const fsPromises = fs.promises;
 const path = require('path');
 const tmpdir = require('../common/tmpdir');
@@ -11,7 +13,9 @@ const tmpDir = tmpdir.path;
 tmpdir.refresh();
 
 const dest = path.resolve(tmpDir, 'tmp.txt');
-const otherDest = path.resolve(tmpDir, 'tmp-2.txt');
+const otherDest2 = path.resolve(tmpDir, 'tmp-2.txt');
+const otherDest3 = path.resolve(tmpDir, 'tmp-3.txt');
+const otherDest4 = path.resolve(tmpDir, 'tmp-4.txt');
 const buffer = Buffer.from('abc'.repeat(1000));
 const buffer2 = Buffer.from('xyz'.repeat(1000));
 
@@ -25,9 +29,33 @@ async function doWriteWithCancel() {
   const controller = new AbortController();
   const { signal } = controller;
   process.nextTick(() => controller.abort());
-  assert.rejects(fsPromises.writeFile(otherDest, buffer, { signal }), {
+  assert.rejects(fsPromises.writeFile(otherDest2, buffer, { signal }), {
     name: 'AbortError'
-  });
+  }).then(common.mustCall(()=> {
+    checkIfFileIsOpen(otherDest2);
+  }));
+}
+
+async function doWriteWithCancelPreSync() {
+  const controller = new AbortController();
+  const { signal } = controller;
+  controller.abort();
+  assert.rejects(fsPromises.writeFile(otherDest3, buffer, { signal }), {
+    name: 'AbortError'
+  }).then(common.mustCall(()=> {
+    checkIfFileIsOpen(otherDest3);
+  }));
+}
+
+async function doWriteWithCancelPostSync() {
+  const controller = new AbortController();
+  const { signal } = controller;
+  controller.abort();
+  assert.rejects(fsPromises.writeFile(otherDest4, buffer, { signal }), {
+    name: 'AbortError'
+  }).then(common.mustCall(()=> {
+    checkIfFileIsOpen(otherDest4);
+  }));
 }
 
 async function doAppend() {
@@ -55,4 +83,21 @@ doWrite()
   .then(doAppend)
   .then(doRead)
   .then(doReadWithEncoding)
+  .then(doWriteWithCancelPreSync)
+  .then(doWriteWithCancelPostSync)
   .then(common.mustCall());
+
+function checkIfFileIsOpen(fileName) {
+  const platform = os.platform();
+  if (platform === 'linux' || platform === 'darwin') {
+    // Tried other ways like rm/stat, but that didn't work.
+    cp.exec(`lsof -p ${process.pid}`, common.mustCall((err, value) => {
+      if (err) {
+        assert.ifError(err);
+        return;
+      }
+      assert.ok(!value.toString().includes(fileName),
+                `${fileName} is still open, but should be closed`);
+    }));
+  }
+}