From 7077e7768c73ea6c273a9f2fd4abd43d9b229071 Mon Sep 17 00:00:00 2001 From: Bruno Jouhier Date: Sat, 7 Feb 2015 19:27:20 +0100 Subject: [PATCH 1/2] fs: properly handle fd passed to truncate() Currently, fs.truncate() silently fails when a file descriptor is passed as the first argument. This commit changes this behavior to properly call fs.ftruncate(). PR-URL: https://github.com/joyent/node/pull/9161 Reviewed-By: Colin Ihrig Reviewed-By: Timothy J Fontaine Reviewed-By: Ben Noordhuis Conflicts: lib/fs.js --- doc/api/fs.markdown | 3 ++- lib/fs.js | 4 +--- test/parallel/test-fs-truncate-fd.js | 25 +++++++++++++++++++++++++ 3 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 test/parallel/test-fs-truncate-fd.js diff --git a/doc/api/fs.markdown b/doc/api/fs.markdown index fe78082cfe57c3..9b824b105c371e 100644 --- a/doc/api/fs.markdown +++ b/doc/api/fs.markdown @@ -106,7 +106,8 @@ Synchronous ftruncate(2). ## fs.truncate(path, len, callback) Asynchronous truncate(2). No arguments other than a possible exception are -given to the completion callback. +given to the completion callback. A file descriptor can also be passed as the +first argument. In this case, `fs.ftruncate()` is called. ## fs.truncateSync(path, len) diff --git a/lib/fs.js b/lib/fs.js index 18332c7840c8b2..5d8d3696908e82 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -679,9 +679,7 @@ fs.renameSync = function(oldPath, newPath) { fs.truncate = function(path, len, callback) { if (typeof path === 'number') { - var req = new FSReqWrap(); - req.oncomplete = callback; - return fs.ftruncate(path, len, req); + return fs.ftruncate(path, len, callback); } if (typeof len === 'function') { callback = len; diff --git a/test/parallel/test-fs-truncate-fd.js b/test/parallel/test-fs-truncate-fd.js new file mode 100644 index 00000000000000..6e9628db867371 --- /dev/null +++ b/test/parallel/test-fs-truncate-fd.js @@ -0,0 +1,25 @@ +var common = require('../common'); +var assert = require('assert'); +var path = require('path'); +var fs = require('fs'); +var tmp = common.tmpDir; +if (!fs.existsSync(tmp)) + fs.mkdirSync(tmp); +var filename = path.resolve(tmp, 'truncate-file.txt'); + +var success = 0; + +fs.writeFileSync(filename, 'hello world', 'utf8'); +var fd = fs.openSync(filename, 'r+'); +fs.truncate(fd, 5, function(err) { + assert.ok(!err); + assert.equal(fs.readFileSync(filename, 'utf8'), 'hello'); + success++; +}); + +process.on('exit', function() { + fs.closeSync(fd); + fs.unlinkSync(filename); + assert.equal(success, 1); + console.log('ok'); +}); From 3d1e49e906f265b46a6c21496c8017038f1dba6c Mon Sep 17 00:00:00 2001 From: cjihrig Date: Wed, 18 Feb 2015 12:55:13 -0500 Subject: [PATCH 2/2] fs: add type checking to makeCallback() This commit adds proper type checking to makeCallback(). Anything other than undefined or a function will throw. PR-URL: https://github.com/iojs/io.js/pull/866 Reviewed-By: Ben Noordhuis --- lib/fs.js | 6 +++++- test/parallel/test-fs-make-callback.js | 27 ++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 test/parallel/test-fs-make-callback.js diff --git a/lib/fs.js b/lib/fs.js index 5d8d3696908e82..63348fc2483a7c 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -65,10 +65,14 @@ function maybeCallback(cb) { // for callbacks that are passed to the binding layer, callbacks that are // invoked from JS already run in the proper scope. function makeCallback(cb) { - if (typeof cb !== 'function') { + if (cb === undefined) { return rethrow(); } + if (typeof cb !== 'function') { + throw new TypeError('callback must be a function'); + } + return function() { return cb.apply(null, arguments); }; diff --git a/test/parallel/test-fs-make-callback.js b/test/parallel/test-fs-make-callback.js new file mode 100644 index 00000000000000..91990015b8220c --- /dev/null +++ b/test/parallel/test-fs-make-callback.js @@ -0,0 +1,27 @@ +var common = require('../common'); +var assert = require('assert'); +var fs = require('fs'); + +function test(cb) { + return function() { + // fs.stat() calls makeCallback() on its second argument + fs.stat(__filename, cb); + }; +} + +// Verify the case where a callback function is provided +assert.doesNotThrow(test(function() {})); + +// Passing undefined calls rethrow() internally, which is fine +assert.doesNotThrow(test(undefined)); + +// Anything else should throw +assert.throws(test(null)); +assert.throws(test(true)); +assert.throws(test(false)); +assert.throws(test(1)); +assert.throws(test(0)); +assert.throws(test('foo')); +assert.throws(test(/foo/)); +assert.throws(test([])); +assert.throws(test({}));