From f9447b71a6b458adbb265f8a5fd38ebf41bc97a4 Mon Sep 17 00:00:00 2001 From: Nitzan Uziely Date: Fri, 14 May 2021 02:26:20 +0300 Subject: [PATCH] fs: fix rmsync error swallowing fix rmsync swallowing errors instead of throwing them. fixes: https://github.com/nodejs/node/issues/38683 fixes: https://github.com/nodejs/node/issues/34580 PR-URL: https://github.com/nodejs/node/pull/38684 Fixes: https://github.com/nodejs/node/issues/38683 Fixes: https://github.com/nodejs/node/issues/34580 Reviewed-By: Antoine du Hamel Reviewed-By: Rich Trott Reviewed-By: Darshan Sen Reviewed-By: James M Snell --- lib/internal/fs/rimraf.js | 17 +- .../test-fs-mkdir-recursive-eaccess.js | 5 +- test/parallel/test-fs-open-no-close.js | 21 +- ...est-fs-promises-file-handle-read-worker.js | 22 +- .../test-fs-promises-file-handle-readFile.js | 17 +- .../test-fs-promises-file-handle-writeFile.js | 18 +- test/parallel/test-fs-promises.js | 237 +++++++++--------- test/parallel/test-fs-read-stream-pos.js | 24 +- test/parallel/test-fs-rm.js | 79 ++++++ 9 files changed, 286 insertions(+), 154 deletions(-) diff --git a/lib/internal/fs/rimraf.js b/lib/internal/fs/rimraf.js index 675c2448c4568c..84f32dc6bb99db 100644 --- a/lib/internal/fs/rimraf.js +++ b/lib/internal/fs/rimraf.js @@ -219,6 +219,11 @@ function _unlinkSync(path, options) { i < tries && options.retryDelay > 0) { sleep(i * options.retryDelay); + } else if (err.code === 'ENOENT') { + // The file is already gone. + return; + } else if (i === tries) { + throw err; } } } @@ -231,8 +236,9 @@ function _rmdirSync(path, options, originalErr) { } catch (err) { if (err.code === 'ENOENT') return; - if (err.code === 'ENOTDIR') - throw originalErr; + if (err.code === 'ENOTDIR') { + throw originalErr || err; + } if (notEmptyErrorCodes.has(err.code)) { // Removing failed. Try removing all children and then retrying the @@ -259,10 +265,17 @@ function _rmdirSync(path, options, originalErr) { i < tries && options.retryDelay > 0) { sleep(i * options.retryDelay); + } else if (err.code === 'ENOENT') { + // The file is already gone. + return; + } else if (i === tries) { + throw err; } } } } + + throw originalErr || err; } } diff --git a/test/parallel/test-fs-mkdir-recursive-eaccess.js b/test/parallel/test-fs-mkdir-recursive-eaccess.js index d0b3b2b950cf7b..c07f8cbd1bf4e5 100644 --- a/test/parallel/test-fs-mkdir-recursive-eaccess.js +++ b/test/parallel/test-fs-mkdir-recursive-eaccess.js @@ -26,8 +26,7 @@ function makeDirectoryReadOnly(dir) { let accessErrorCode = 'EACCES'; if (common.isWindows) { accessErrorCode = 'EPERM'; - execSync(`icacls ${dir} /inheritance:r`); - execSync(`icacls ${dir} /deny "everyone":W`); + execSync(`icacls ${dir} /deny "everyone:(OI)(CI)(DE,DC,AD,WD)"`); } else { fs.chmodSync(dir, '444'); } @@ -36,7 +35,7 @@ function makeDirectoryReadOnly(dir) { function makeDirectoryWritable(dir) { if (common.isWindows) { - execSync(`icacls ${dir} /grant "everyone":W`); + execSync(`icacls ${dir} /remove:d "everyone"`); } } diff --git a/test/parallel/test-fs-open-no-close.js b/test/parallel/test-fs-open-no-close.js index 5e432dd11d8084..41b58f5ef797d7 100644 --- a/test/parallel/test-fs-open-no-close.js +++ b/test/parallel/test-fs-open-no-close.js @@ -15,10 +15,17 @@ const debuglog = (arg) => { const tmpdir = require('../common/tmpdir'); tmpdir.refresh(); -{ - fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => { - debuglog('fs open() callback'); - assert.ifError(err); - })); - debuglog('waiting for callback'); -} +let openFd; + +fs.open(`${tmpdir.path}/dummy`, 'wx+', common.mustCall((err, fd) => { + debuglog('fs open() callback'); + assert.ifError(err); + openFd = fd; +})); +debuglog('waiting for callback'); + +process.on('beforeExit', common.mustCall(() => { + if (openFd) { + fs.closeSync(openFd); + } +})); diff --git a/test/parallel/test-fs-promises-file-handle-read-worker.js b/test/parallel/test-fs-promises-file-handle-read-worker.js index ccf3338d14506a..a1b7fbd2dfbe15 100644 --- a/test/parallel/test-fs-promises-file-handle-read-worker.js +++ b/test/parallel/test-fs-promises-file-handle-read-worker.js @@ -19,16 +19,20 @@ if (isMainThread || !workerData) { transferList: [handle] }); }); - fs.promises.open(file, 'r').then((handle) => { - fs.createReadStream(null, { fd: handle }); - assert.throws(() => { - new Worker(__filename, { - workerData: { handle }, - transferList: [handle] + fs.promises.open(file, 'r').then(async (handle) => { + try { + fs.createReadStream(null, { fd: handle }); + assert.throws(() => { + new Worker(__filename, { + workerData: { handle }, + transferList: [handle] + }); + }, { + code: 25, }); - }, { - code: 25, - }); + } finally { + await handle.close(); + } }); } else { let output = ''; diff --git a/test/parallel/test-fs-promises-file-handle-readFile.js b/test/parallel/test-fs-promises-file-handle-readFile.js index fc28dd23328df7..880b97d70713e1 100644 --- a/test/parallel/test-fs-promises-file-handle-readFile.js +++ b/test/parallel/test-fs-promises-file-handle-readFile.js @@ -56,13 +56,16 @@ async function doReadAndCancel() { { const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt'); const fileHandle = await open(filePathForHandle, 'w+'); - const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8'); - fs.writeFileSync(filePathForHandle, buffer); - const signal = AbortSignal.abort(); - await assert.rejects(readFile(fileHandle, { signal }), { - name: 'AbortError' - }); - await fileHandle.close(); + try { + const buffer = Buffer.from('Dogs running'.repeat(10000), 'utf8'); + fs.writeFileSync(filePathForHandle, buffer); + const signal = AbortSignal.abort(); + await assert.rejects(readFile(fileHandle, { signal }), { + name: 'AbortError' + }); + } finally { + await fileHandle.close(); + } } // Signal aborted on first tick diff --git a/test/parallel/test-fs-promises-file-handle-writeFile.js b/test/parallel/test-fs-promises-file-handle-writeFile.js index 7ad1beb4bcdd7b..44f049f55a9944 100644 --- a/test/parallel/test-fs-promises-file-handle-writeFile.js +++ b/test/parallel/test-fs-promises-file-handle-writeFile.js @@ -30,13 +30,17 @@ async function validateWriteFile() { async function doWriteAndCancel() { const filePathForHandle = path.resolve(tmpDir, 'dogs-running.txt'); const fileHandle = await open(filePathForHandle, 'w+'); - const buffer = Buffer.from('dogs running'.repeat(512 * 1024), 'utf8'); - const controller = new AbortController(); - const { signal } = controller; - process.nextTick(() => controller.abort()); - await assert.rejects(writeFile(fileHandle, buffer, { signal }), { - name: 'AbortError' - }); + try { + const buffer = Buffer.from('dogs running'.repeat(512 * 1024), 'utf8'); + const controller = new AbortController(); + const { signal } = controller; + process.nextTick(() => controller.abort()); + await assert.rejects(writeFile(fileHandle, buffer, { signal }), { + name: 'AbortError' + }); + } finally { + await fileHandle.close(); + } } validateWriteFile() diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index f0140084732056..5da1e55ffd2586 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -90,6 +90,18 @@ async function getHandle(dest) { return open(dest, 'r+'); } +async function executeOnHandle(dest, func) { + let handle; + try { + handle = await getHandle(dest); + await func(handle); + } finally { + if (handle) { + await handle.close(); + } + } +} + { async function doTest() { tmpdir.refresh(); @@ -98,140 +110,138 @@ async function getHandle(dest) { // handle is object { - const handle = await getHandle(dest); - assert.strictEqual(typeof handle, 'object'); - await handle.close(); + await executeOnHandle(dest, async (handle) => { + assert.strictEqual(typeof handle, 'object'); + }); } // file stats { - const handle = await getHandle(dest); - let stats = await handle.stat(); - verifyStatObject(stats); - assert.strictEqual(stats.size, 35); + await executeOnHandle(dest, async (handle) => { + let stats = await handle.stat(); + verifyStatObject(stats); + assert.strictEqual(stats.size, 35); - await handle.truncate(1); + await handle.truncate(1); - stats = await handle.stat(); - verifyStatObject(stats); - assert.strictEqual(stats.size, 1); + stats = await handle.stat(); + verifyStatObject(stats); + assert.strictEqual(stats.size, 1); - stats = await stat(dest); - verifyStatObject(stats); + stats = await stat(dest); + verifyStatObject(stats); - stats = await handle.stat(); - verifyStatObject(stats); + stats = await handle.stat(); + verifyStatObject(stats); - await handle.datasync(); - await handle.sync(); - await handle.close(); + await handle.datasync(); + await handle.sync(); + }); } // Test fs.read promises when length to read is zero bytes { const dest = path.resolve(tmpDir, 'test1.js'); - const handle = await getHandle(dest); - const buf = Buffer.from('DAWGS WIN'); - const bufLen = buf.length; - await handle.write(buf); - const ret = await handle.read(Buffer.alloc(bufLen), 0, 0, 0); - assert.strictEqual(ret.bytesRead, 0); - - await unlink(dest); - await handle.close(); + await executeOnHandle(dest, async (handle) => { + const buf = Buffer.from('DAWGS WIN'); + const bufLen = buf.length; + await handle.write(buf); + const ret = await handle.read(Buffer.alloc(bufLen), 0, 0, 0); + assert.strictEqual(ret.bytesRead, 0); + + await unlink(dest); + }); } // Use fallback buffer allocation when input not buffer { - const handle = await getHandle(dest); - const ret = await handle.read(0, 0, 0, 0); - assert.strictEqual(ret.buffer.length, 16384); + await executeOnHandle(dest, async (handle) => { + const ret = await handle.read(0, 0, 0, 0); + assert.strictEqual(ret.buffer.length, 16384); + }); } // Bytes written to file match buffer { - const handle = await getHandle(dest); - const buf = Buffer.from('hello fsPromises'); - const bufLen = buf.length; - await handle.write(buf); - const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0); - assert.strictEqual(ret.bytesRead, bufLen); - assert.deepStrictEqual(ret.buffer, buf); - await handle.close(); + await executeOnHandle(dest, async (handle) => { + const buf = Buffer.from('hello fsPromises'); + const bufLen = buf.length; + await handle.write(buf); + const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0); + assert.strictEqual(ret.bytesRead, bufLen); + assert.deepStrictEqual(ret.buffer, buf); + }); } // Truncate file to specified length { - const handle = await getHandle(dest); - const buf = Buffer.from('hello FileHandle'); - const bufLen = buf.length; - await handle.write(buf, 0, bufLen, 0); - const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0); - assert.strictEqual(ret.bytesRead, bufLen); - assert.deepStrictEqual(ret.buffer, buf); - await truncate(dest, 5); - assert.deepStrictEqual((await readFile(dest)).toString(), 'hello'); - await handle.close(); + await executeOnHandle(dest, async (handle) => { + const buf = Buffer.from('hello FileHandle'); + const bufLen = buf.length; + await handle.write(buf, 0, bufLen, 0); + const ret = await handle.read(Buffer.alloc(bufLen), 0, bufLen, 0); + assert.strictEqual(ret.bytesRead, bufLen); + assert.deepStrictEqual(ret.buffer, buf); + await truncate(dest, 5); + assert.deepStrictEqual((await readFile(dest)).toString(), 'hello'); + }); } // Invalid change of ownership { - const handle = await getHandle(dest); + await executeOnHandle(dest, async (handle) => { + await chmod(dest, 0o666); + await handle.chmod(0o666); - await chmod(dest, 0o666); - await handle.chmod(0o666); + await chmod(dest, (0o10777)); + await handle.chmod(0o10777); - await chmod(dest, (0o10777)); - await handle.chmod(0o10777); - - if (!common.isWindows) { - await chown(dest, process.getuid(), process.getgid()); - await handle.chown(process.getuid(), process.getgid()); - } - - assert.rejects( - async () => { - await chown(dest, 1, -2); - }, - { - code: 'ERR_OUT_OF_RANGE', - name: 'RangeError', - message: 'The value of "gid" is out of range. ' + - 'It must be >= -1 && <= 4294967295. Received -2' - }); - - assert.rejects( - async () => { - await handle.chown(1, -2); - }, - { - code: 'ERR_OUT_OF_RANGE', - name: 'RangeError', - message: 'The value of "gid" is out of range. ' + - 'It must be >= -1 && <= 4294967295. Received -2' - }); + if (!common.isWindows) { + await chown(dest, process.getuid(), process.getgid()); + await handle.chown(process.getuid(), process.getgid()); + } - await handle.close(); + await assert.rejects( + async () => { + await chown(dest, 1, -2); + }, + { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError', + message: 'The value of "gid" is out of range. ' + + 'It must be >= -1 && <= 4294967295. Received -2' + }); + + await assert.rejects( + async () => { + await handle.chown(1, -2); + }, + { + code: 'ERR_OUT_OF_RANGE', + name: 'RangeError', + message: 'The value of "gid" is out of range. ' + + 'It must be >= -1 && <= 4294967295. Received -2' + }); + }); } // Set modification times { - const handle = await getHandle(dest); - - await utimes(dest, new Date(), new Date()); - - try { - await handle.utimes(new Date(), new Date()); - } catch (err) { - // Some systems do not have futimes. If there is an error, - // expect it to be ENOSYS - common.expectsError({ - code: 'ENOSYS', - name: 'Error' - })(err); - } - - await handle.close(); + await executeOnHandle(dest, async (handle) => { + + await utimes(dest, new Date(), new Date()); + + try { + await handle.utimes(new Date(), new Date()); + } catch (err) { + // Some systems do not have futimes. If there is an error, + // expect it to be ENOSYS + common.expectsError({ + code: 'ENOSYS', + name: 'Error' + })(err); + } + }); } // Set modification times with lutimes @@ -438,29 +448,28 @@ async function getHandle(dest) { // Regression test for https://github.com/nodejs/node/issues/38168 { - const handle = await getHandle(dest); - - assert.rejects( - async () => handle.write('abc', 0, 'hex'), - { - code: 'ERR_INVALID_ARG_VALUE', - message: /'encoding' is invalid for data of length 3/ - } - ); + await executeOnHandle(dest, async (handle) => { + await assert.rejects( + async () => handle.write('abc', 0, 'hex'), + { + code: 'ERR_INVALID_ARG_VALUE', + message: /'encoding' is invalid for data of length 3/ + } + ); - const ret = await handle.write('abcd', 0, 'hex'); - assert.strictEqual(ret.bytesWritten, 2); - await handle.close(); + const ret = await handle.write('abcd', 0, 'hex'); + assert.strictEqual(ret.bytesWritten, 2); + }); } // Test prototype methods calling with contexts other than FileHandle { - const handle = await getHandle(dest); - assert.rejects(() => handle.stat.call({}), { - code: 'ERR_INTERNAL_ASSERTION', - message: /handle must be an instance of FileHandle/ + await executeOnHandle(dest, async (handle) => { + await assert.rejects(() => handle.stat.call({}), { + code: 'ERR_INTERNAL_ASSERTION', + message: /handle must be an instance of FileHandle/ + }); }); - await handle.close(); } } diff --git a/test/parallel/test-fs-read-stream-pos.js b/test/parallel/test-fs-read-stream-pos.js index c9470cb23ddeb6..c67c2e8a81fc47 100644 --- a/test/parallel/test-fs-read-stream-pos.js +++ b/test/parallel/test-fs-read-stream-pos.js @@ -16,7 +16,7 @@ fs.writeFileSync(file, ''); let counter = 0; -setInterval(() => { +const writeInterval = setInterval(() => { counter = counter + 1; const line = `hello at ${counter}\n`; fs.writeFileSync(file, line, { flag: 'a' }); @@ -28,7 +28,7 @@ let isLow = false; let cur = 0; let stream; -setInterval(() => { +const readInterval = setInterval(() => { if (stream) return; stream = fs.createReadStream(file, { @@ -49,7 +49,7 @@ setInterval(() => { return false; }); assert.strictEqual(brokenLines.length, 0); - process.exit(); + exitTest(); return; } if (chunk.length !== hwm) { @@ -64,6 +64,20 @@ setInterval(() => { }, 10); // Time longer than 90 seconds to exit safely -setTimeout(() => { - process.exit(); +const endTimer = setTimeout(() => { + exitTest(); }, 90000); + +const exitTest = () => { + clearInterval(readInterval); + clearInterval(writeInterval); + clearTimeout(endTimer); + if (stream && !stream.destroyed) { + stream.on('close', () => { + process.exit(); + }); + stream.destroy(); + } else { + process.exit(); + } +}; diff --git a/test/parallel/test-fs-rm.js b/test/parallel/test-fs-rm.js index 5bb5d2de553eee..3fdfc8426248ac 100644 --- a/test/parallel/test-fs-rm.js +++ b/test/parallel/test-fs-rm.js @@ -5,6 +5,8 @@ const tmpdir = require('../common/tmpdir'); const assert = require('assert'); const fs = require('fs'); const path = require('path'); +const { execSync } = require('child_process'); + const { validateRmOptionsSync } = require('internal/fs/utils'); tmpdir.refresh(); @@ -280,3 +282,80 @@ function removeAsync(dir) { message: /^The value of "options\.maxRetries" is out of range\./ }); } + +{ + // IBMi has a different access permission mechanism + // This test should not be run as `root` + if (!common.isIBMi && (common.isWindows || process.getuid() !== 0)) { + function makeDirectoryReadOnly(dir, mode) { + let accessErrorCode = 'EACCES'; + if (common.isWindows) { + accessErrorCode = 'EPERM'; + execSync(`icacls ${dir} /deny "everyone:(OI)(CI)(DE,DC)"`); + } else { + fs.chmodSync(dir, mode); + } + return accessErrorCode; + } + + function makeDirectoryWritable(dir) { + if (fs.existsSync(dir)) { + if (common.isWindows) { + execSync(`icacls ${dir} /remove:d "everyone"`); + } else { + fs.chmodSync(dir, 0o777); + } + } + } + + { + // Check that deleting a file that cannot be accessed using rmsync throws + // https://github.com/nodejs/node/issues/38683 + const dirname = nextDirPath(); + const filePath = path.join(dirname, 'text.txt'); + try { + fs.mkdirSync(dirname, { recursive: true }); + fs.writeFileSync(filePath, 'hello'); + const code = makeDirectoryReadOnly(dirname, 0o444); + assert.throws(() => { + fs.rmSync(filePath, { force: true }); + }, { + code, + name: 'Error', + }); + } finally { + makeDirectoryWritable(dirname); + } + } + + { + // Check endless recursion. + // https://github.com/nodejs/node/issues/34580 + const dirname = nextDirPath(); + fs.mkdirSync(dirname, { recursive: true }); + const root = fs.mkdtempSync(path.join(dirname, 'fs-')); + const middle = path.join(root, 'middle'); + fs.mkdirSync(middle); + fs.mkdirSync(path.join(middle, 'leaf')); // Make `middle` non-empty + try { + const code = makeDirectoryReadOnly(middle, 0o555); + try { + assert.throws(() => { + fs.rmSync(root, { recursive: true }); + }, { + code, + name: 'Error', + }); + } catch (err) { + // Only fail the test if the folder was not deleted. + // as in some cases rmSync succesfully deletes read-only folders. + if (fs.existsSync(root)) { + throw err; + } + } + } finally { + makeDirectoryWritable(middle); + } + } + } +}