From fb6df3bfac8ca38a7012eabf0563d7a799ce1acc Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 19 Dec 2019 19:00:45 +0100 Subject: [PATCH] fs: validate the input data to be of expected types The input was not validated so far and that caused unwanted side effects. E.g., `undefined` became the string `'undefined'`. It was expected to fail or to end up as empty string. Now all input is validated to be either some type of array buffer view or a string. That way it's always clear what the user intents. PR-URL: https://github.com/nodejs/node/pull/31030 Fixes: https://github.com/nodejs/node/issues/31025 Reviewed-By: Rich Trott Reviewed-By: Yongsheng Zhang Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Gireesh Punathil --- doc/api/fs.md | 46 ++++++++++++++- lib/fs.js | 47 ++++++++------- lib/internal/fs/promises.js | 22 +++---- lib/internal/fs/utils.js | 11 ++++ test/parallel/test-fs-append-file-sync.js | 11 +++- test/parallel/test-fs-append-file.js | 59 ++++--------------- test/parallel/test-fs-buffertype-writesync.js | 23 +++----- .../test-fs-promises-file-handle-write.js | 9 ++- test/parallel/test-fs-promises.js | 4 +- test/parallel/test-fs-write-file.js | 21 ------- test/parallel/test-fs-write-string-coerce.js | 28 --------- test/parallel/test-fs-write.js | 17 ++++++ 12 files changed, 148 insertions(+), 150 deletions(-) delete mode 100644 test/parallel/test-fs-write-string-coerce.js diff --git a/doc/api/fs.md b/doc/api/fs.md index 356436034e2d58..ad45c7b6865e9d 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -3897,6 +3897,10 @@ This happens when: * `buffer` {Buffer|Uint8Array} @@ -4518,6 +4547,11 @@ the end of the file. #### `filehandle.write(string[, position[, encoding]])` * `string` {string} @@ -4526,7 +4560,7 @@ added: v10.0.0 * Returns: {Promise} Write `string` to the file. If `string` is not a string, then -the value will be coerced to one. +an exception will be thrown. The `Promise` is resolved with an object containing a `bytesWritten` property identifying the number of bytes written, and a `buffer` property containing @@ -4549,6 +4583,11 @@ the end of the file. #### `filehandle.writeFile(data, options)` * `data` {string|Buffer|Uint8Array} @@ -5137,6 +5176,11 @@ The `atime` and `mtime` arguments follow these rules: ### `fsPromises.writeFile(file, data[, options])` * `file` {string|Buffer|URL|FileHandle} filename or `FileHandle` diff --git a/lib/fs.js b/lib/fs.js index f0879fce41fa36..a85b8ab29ebd1a 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -92,6 +92,7 @@ const { validateOffsetLengthWrite, validatePath, validateRmdirOptions, + validateStringAfterArrayBufferView, warnOnNonPortableTemplate } = require('internal/fs/utils'); const { @@ -548,9 +549,6 @@ function write(fd, buffer, offset, length, position, callback) { validateInt32(fd, 'fd', 0); - const req = new FSReqCallback(); - req.oncomplete = wrapper; - if (isArrayBufferView(buffer)) { callback = maybeCallback(callback || position || length || offset); if (offset == null || typeof offset === 'function') { @@ -563,11 +561,14 @@ function write(fd, buffer, offset, length, position, callback) { if (typeof position !== 'number') position = null; validateOffsetLengthWrite(offset, length, buffer.byteLength); + + const req = new FSReqCallback(); + req.oncomplete = wrapper; return binding.writeBuffer(fd, buffer, offset, length, position, req); } - if (typeof buffer !== 'string') - buffer += ''; + validateStringAfterArrayBufferView(buffer, 'buffer'); + if (typeof position !== 'function') { if (typeof offset === 'function') { position = offset; @@ -578,6 +579,9 @@ function write(fd, buffer, offset, length, position, callback) { length = 'utf8'; } callback = maybeCallback(position); + + const req = new FSReqCallback(); + req.oncomplete = wrapper; return binding.writeString(fd, buffer, offset, length, req); } @@ -606,8 +610,8 @@ function writeSync(fd, buffer, offset, length, position) { result = binding.writeBuffer(fd, buffer, offset, length, position, undefined, ctx); } else { - if (typeof buffer !== 'string') - buffer += ''; + validateStringAfterArrayBufferView(buffer, 'buffer'); + if (offset === undefined) offset = null; result = binding.writeString(fd, buffer, offset, length, @@ -1277,8 +1281,14 @@ function writeFile(path, data, options, callback) { options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); const flag = options.flag || 'w'; + if (!isArrayBufferView(data)) { + validateStringAfterArrayBufferView(data, 'data'); + data = Buffer.from(data, options.encoding || 'utf8'); + } + if (isFd(path)) { - writeFd(path, true); + const isUserFd = true; + writeAll(path, isUserFd, data, 0, data.byteLength, null, callback); return; } @@ -1286,29 +1296,26 @@ function writeFile(path, data, options, callback) { if (openErr) { callback(openErr); } else { - writeFd(fd, false); + const isUserFd = false; + const position = /a/.test(flag) ? null : 0; + writeAll(fd, isUserFd, data, 0, data.byteLength, position, callback); } }); - - function writeFd(fd, isUserFd) { - const buffer = isArrayBufferView(data) ? - data : Buffer.from('' + data, options.encoding || 'utf8'); - const position = (/a/.test(flag) || isUserFd) ? null : 0; - - writeAll(fd, isUserFd, buffer, 0, buffer.byteLength, position, callback); - } } function writeFileSync(path, data, options) { options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); + + if (!isArrayBufferView(data)) { + validateStringAfterArrayBufferView(data, 'data'); + data = Buffer.from(data, options.encoding || 'utf8'); + } + const flag = options.flag || 'w'; const isUserFd = isFd(path); // File descriptor ownership const fd = isUserFd ? path : fs.openSync(path, flag, options.mode); - if (!isArrayBufferView(data)) { - data = Buffer.from('' + data, options.encoding || 'utf8'); - } let offset = 0; let length = data.byteLength; let position = (/a/.test(flag) || isUserFd) ? null : 0; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index e0c4e2ca4a49e7..6517a5ef4be056 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -26,7 +26,7 @@ const { ERR_INVALID_ARG_VALUE, ERR_METHOD_NOT_IMPLEMENTED } = require('internal/errors').codes; -const { isUint8Array } = require('internal/util/types'); +const { isArrayBufferView } = require('internal/util/types'); const { rimrafPromises } = require('internal/fs/rimraf'); const { copyObject, @@ -44,6 +44,7 @@ const { validateOffsetLengthRead, validateOffsetLengthWrite, validateRmdirOptions, + validateStringAfterArrayBufferView, warnOnNonPortableTemplate } = require('internal/fs/utils'); const { opendir } = require('internal/fs/dir'); @@ -140,16 +141,18 @@ function validateFileHandle(handle) { } async function writeFileHandle(filehandle, data, options) { - let buffer = isUint8Array(data) ? - data : Buffer.from('' + data, options.encoding || 'utf8'); - let remaining = buffer.length; + if (!isArrayBufferView(data)) { + validateStringAfterArrayBufferView(data, 'data'); + data = Buffer.from(data, options.encoding || 'utf8'); + } + let remaining = data.length; if (remaining === 0) return; do { const { bytesWritten } = - await write(filehandle, buffer, 0, - MathMin(16384, buffer.length)); + await write(filehandle, data, 0, + MathMin(16384, data.length)); remaining -= bytesWritten; - buffer = buffer.slice(bytesWritten); + data = data.slice(bytesWritten); } while (remaining > 0); } @@ -260,7 +263,7 @@ async function write(handle, buffer, offset, length, position) { if (buffer.length === 0) return { bytesWritten: 0, buffer }; - if (isUint8Array(buffer)) { + if (isArrayBufferView(buffer)) { if (offset == null) { offset = 0; } else { @@ -277,8 +280,7 @@ async function write(handle, buffer, offset, length, position) { return { bytesWritten, buffer }; } - if (typeof buffer !== 'string') - buffer += ''; + validateStringAfterArrayBufferView(buffer, 'buffer'); const bytesWritten = (await binding.writeString(handle.fd, buffer, offset, length, kUsePromises)) || 0; return { bytesWritten, buffer }; diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 18d8e07d78e3cb..763a940f6e98a7 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -651,6 +651,16 @@ const getValidMode = hideStackFrames((mode, type) => { 'mode', `an integer >= ${min} && <= ${max}`, mode); }); +const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => { + if (typeof buffer !== 'string') { + throw new ERR_INVALID_ARG_TYPE( + name, + ['string', 'Buffer', 'TypedArray', 'DataView'], + buffer + ); + } +}); + module.exports = { assertEncoding, BigIntStats, // for testing @@ -675,5 +685,6 @@ module.exports = { validateOffsetLengthWrite, validatePath, validateRmdirOptions, + validateStringAfterArrayBufferView, warnOnNonPortableTemplate }; diff --git a/test/parallel/test-fs-append-file-sync.js b/test/parallel/test-fs-append-file-sync.js index 521776d77900ab..9914863674b0b2 100644 --- a/test/parallel/test-fs-append-file-sync.js +++ b/test/parallel/test-fs-append-file-sync.js @@ -70,11 +70,18 @@ const fileData3 = fs.readFileSync(filename3); assert.strictEqual(buf.length + currentFileData.length, fileData3.length); -// Test that appendFile accepts numbers. const filename4 = join(tmpdir.path, 'append-sync4.txt'); fs.writeFileSync(filename4, currentFileData, { mode: m }); -fs.appendFileSync(filename4, num, { mode: m }); +[ + true, false, 0, 1, Infinity, () => {}, {}, [], undefined, null +].forEach((value) => { + assert.throws( + () => fs.appendFileSync(filename4, value, { mode: m }), + { message: /data/, code: 'ERR_INVALID_ARG_TYPE' } + ); +}); +fs.appendFileSync(filename4, `${num}`, { mode: m }); // Windows permissions aren't Unix. if (!common.isWindows) { diff --git a/test/parallel/test-fs-append-file.js b/test/parallel/test-fs-append-file.js index 90fe8d9277a23e..8220d01ca1b2fe 100644 --- a/test/parallel/test-fs-append-file.js +++ b/test/parallel/test-fs-append-file.js @@ -29,7 +29,6 @@ const tmpdir = require('../common/tmpdir'); const currentFileData = 'ABCD'; -const n = 220; const s = '南越国是前203年至前111年存在于岭南地区的一个国家，国都位于番禺，疆域包括今天中国的广东、' + '广西两省区的大部份地区，福建省、湖南、贵州、云南的一小部份地区和越南的北部。' + '南越国是秦朝灭亡后，由南海郡尉赵佗于前203年起兵兼并桂林郡和象郡后建立。' + @@ -130,51 +129,19 @@ const throwNextTick = (e) => { process.nextTick(() => { throw e; }); }; .catch(throwNextTick); } -// Test that appendFile accepts numbers (callback API). -{ - const filename = join(tmpdir.path, 'append-numbers.txt'); - fs.writeFileSync(filename, currentFileData); - - const m = 0o600; - fs.appendFile(filename, n, { mode: m }, common.mustCall((e) => { - assert.ifError(e); - - // Windows permissions aren't Unix. - if (!common.isWindows) { - const st = fs.statSync(filename); - assert.strictEqual(st.mode & 0o700, m); - } - - fs.readFile(filename, common.mustCall((e, buffer) => { - assert.ifError(e); - assert.strictEqual(Buffer.byteLength(String(n)) + currentFileData.length, - buffer.length); - })); - })); -} - -// Test that appendFile accepts numbers (promises API). -{ - const filename = join(tmpdir.path, 'append-numbers-promises.txt'); - fs.writeFileSync(filename, currentFileData); - - const m = 0o600; - fs.promises.appendFile(filename, n, { mode: m }) - .then(common.mustCall(() => { - // Windows permissions aren't Unix. - if (!common.isWindows) { - const st = fs.statSync(filename); - assert.strictEqual(st.mode & 0o700, m); - } - - return fs.promises.readFile(filename); - })) - .then((buffer) => { - assert.strictEqual(Buffer.byteLength(String(n)) + currentFileData.length, - buffer.length); - }) - .catch(throwNextTick); -} +// Test that appendFile does not accept numbers (callback API). +[false, 5, {}, [], null, undefined].forEach((data) => { + const errObj = { + code: 'ERR_INVALID_ARG_TYPE', + message: /"data"|"buffer"/ + }; + assert.throws( + () => fs.appendFile('foobar', data, common.mustNotCall()), + errObj + ); + assert.throws(() => fs.appendFileSync('foobar', data), errObj); + assert.rejects(fs.promises.appendFile('foobar', data), errObj); +}); // Test that appendFile accepts file descriptors (callback API). { diff --git a/test/parallel/test-fs-buffertype-writesync.js b/test/parallel/test-fs-buffertype-writesync.js index 042552a46f97b3..fca5b944970836 100644 --- a/test/parallel/test-fs-buffertype-writesync.js +++ b/test/parallel/test-fs-buffertype-writesync.js @@ -1,23 +1,16 @@ 'use strict'; require('../common'); -// This test ensures that writeSync does support inputs which -// are then correctly converted into string buffers. +// This test ensures that writeSync throws for invalid data input. const assert = require('assert'); const fs = require('fs'); -const path = require('path'); -const tmpdir = require('../common/tmpdir'); - -const filePath = path.join(tmpdir.path, 'test_buffer_type'); -const v = [true, false, 0, 1, Infinity, () => {}, {}, [], undefined, null]; - -tmpdir.refresh(); - -v.forEach((value) => { - const fd = fs.openSync(filePath, 'w'); - fs.writeSync(fd, value); - assert.strictEqual(fs.readFileSync(filePath).toString(), String(value)); - fs.closeSync(fd); +[ + true, false, 0, 1, Infinity, () => {}, {}, [], undefined, null +].forEach((value) => { + assert.throws( + () => fs.writeSync(1, value), + { message: /"buffer"/, code: 'ERR_INVALID_ARG_TYPE' } + ); }); diff --git a/test/parallel/test-fs-promises-file-handle-write.js b/test/parallel/test-fs-promises-file-handle-write.js index 60840e1e5304d8..dcff311b3d3a33 100644 --- a/test/parallel/test-fs-promises-file-handle-write.js +++ b/test/parallel/test-fs-promises-file-handle-write.js @@ -55,13 +55,12 @@ async function validateNonStringValuesWrite() { const fileHandle = await open(filePathForHandle, 'w+'); const nonStringValues = [123, {}, new Map()]; for (const nonStringValue of nonStringValues) { - await fileHandle.write(nonStringValue); + await assert.rejects( + fileHandle.write(nonStringValue), + { message: /"buffer"/, code: 'ERR_INVALID_ARG_TYPE' } + ); } - const readFileData = fs.readFileSync(filePathForHandle); - const expected = ['123', '[object Object]', '[object Map]'].join(''); - assert.deepStrictEqual(Buffer.from(expected, 'utf8'), readFileData); - await fileHandle.close(); } diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 08b86ea4ed41f0..f23df685eac072 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -324,7 +324,7 @@ async function getHandle(dest) { { const dir = path.join(tmpDir, nextdir(), nextdir()); await mkdir(path.dirname(dir)); - await writeFile(dir); + await writeFile(dir, ''); assert.rejects( mkdir(dir, { recursive: true }), { @@ -341,7 +341,7 @@ async function getHandle(dest) { const file = path.join(tmpDir, nextdir(), nextdir()); const dir = path.join(file, nextdir(), nextdir()); await mkdir(path.dirname(file)); - await writeFile(file); + await writeFile(file, ''); assert.rejects( mkdir(dir, { recursive: true }), { diff --git a/test/parallel/test-fs-write-file.js b/test/parallel/test-fs-write-file.js index 97f29c081bcd7c..74aab1423d3a68 100644 --- a/test/parallel/test-fs-write-file.js +++ b/test/parallel/test-fs-write-file.js @@ -30,7 +30,6 @@ tmpdir.refresh(); const filename = join(tmpdir.path, 'test.txt'); -const n = 220; const s = '南越国是前203年至前111年存在于岭南地区的一个国家，国都位于番禺，疆域包括今天中国的广东、' + '广西两省区的大部份地区，福建省、湖南、贵州、云南的一小部份地区和越南的北部。' + '南越国是秦朝灭亡后，由南海郡尉赵佗于前203年起兵兼并桂林郡和象郡后建立。' + @@ -62,26 +61,6 @@ fs.writeFile(filename2, buf, common.mustCall(function(e) { })); })); -// Test that writeFile accepts numbers. -const filename3 = join(tmpdir.path, 'test3.txt'); - -const m = 0o600; -fs.writeFile(filename3, n, { mode: m }, common.mustCall(function(e) { - assert.ifError(e); - - // Windows permissions aren't Unix. - if (!common.isWindows) { - const st = fs.statSync(filename3); - assert.strictEqual(st.mode & 0o700, m); - } - - fs.readFile(filename3, common.mustCall(function(e, buffer) { - assert.ifError(e); - - assert.strictEqual(Buffer.byteLength(String(n)), buffer.length); - })); -})); - // Test that writeFile accepts file descriptors. const filename4 = join(tmpdir.path, 'test4.txt'); diff --git a/test/parallel/test-fs-write-string-coerce.js b/test/parallel/test-fs-write-string-coerce.js deleted file mode 100644 index b9ac82aec026c5..00000000000000 --- a/test/parallel/test-fs-write-string-coerce.js +++ /dev/null @@ -1,28 +0,0 @@ -'use strict'; -const common = require('../common'); -const assert = require('assert'); -const path = require('path'); -const fs = require('fs'); - -const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); - -const fn = path.join(tmpdir.path, 'write-string-coerce.txt'); -const data = true; -const expected = String(data); - -fs.open(fn, 'w', 0o644, common.mustCall(function(err, fd) { - assert.ifError(err); - console.log('open done'); - fs.write(fd, data, 0, 'utf8', common.mustCall(function(err, written) { - console.log('write done'); - assert.ifError(err); - assert.strictEqual(written, Buffer.byteLength(expected)); - fs.closeSync(fd); - const found = fs.readFileSync(fn, 'utf8'); - console.log(`expected: "${expected}"`); - console.log(`found: "${found}"`); - fs.unlinkSync(fn); - assert.strictEqual(found, expected); - })); -})); diff --git a/test/parallel/test-fs-write.js b/test/parallel/test-fs-write.js index 39a0f3720ee76a..bdbbad9cb834d4 100644 --- a/test/parallel/test-fs-write.js +++ b/test/parallel/test-fs-write.js @@ -150,3 +150,20 @@ fs.open(fn3, 'w', 0o644, common.mustCall((err, fd) => { } ); }); + +[false, 5, {}, [], null, undefined].forEach((data) => { + assert.throws( + () => fs.write(1, data, common.mustNotCall()), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /"buffer"/ + } + ); + assert.throws( + () => fs.writeSync(1, data), + { + code: 'ERR_INVALID_ARG_TYPE', + message: /"buffer"/ + } + ); +});