From 559c2d5dc55bdfad31045ed8543cb4c396146a0a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jes=C3=BAs=20Legan=C3=A9s=20Combarro=20=22piranna?= Date: Fri, 6 Nov 2015 23:37:47 +0100 Subject: [PATCH] Fix: Improve stats logic to avoid problems when files not owned --- lib/dest/writeContents/index.js | 102 ++++++++++++++++++-- lib/dest/writeContents/writeBuffer.js | 16 +-- lib/dest/writeContents/writeStream.js | 26 ++--- lib/dest/writeContents/writeSymbolicLink.js | 2 +- lib/futimes.js | 27 ------ test/dest.js | 36 ++++--- test/spy.js | 2 + 7 files changed, 131 insertions(+), 80 deletions(-) delete mode 100644 lib/futimes.js diff --git a/lib/dest/writeContents/index.js b/lib/dest/writeContents/index.js index 81ef50ca..745d3bf1 100644 --- a/lib/dest/writeContents/index.js +++ b/lib/dest/writeContents/index.js @@ -1,11 +1,65 @@ 'use strict'; var fs = require('fs'); + var writeDir = require('./writeDir'); var writeStream = require('./writeStream'); var writeBuffer = require('./writeBuffer'); var writeSymbolicLink = require('./writeSymbolicLink'); + +function getMode(oldStat, newStat) { + if (typeof newStat.mode !== 'number') { + return; + } + + var currentMode = (oldStat.mode & parseInt('0777', 8)); + var expectedMode = (newStat.mode & parseInt('0777', 8)); + if (currentMode !== expectedMode) { + return expectedMode; + } +} + +function getUtimes(oldStat, newStat) { + // `futimes` only works if we own the file + if (oldStat.uid !== process.getuid()) { + return; + } + + // Given `mtime` is not valid, do nothing + var mtime = newStat.mtime; + if (!validDate(mtime)) { + return; + } + + // Given `atime` is not valid, assign a new one with current time + var atime = newStat.atime; + if (!validDate(atime)) { + atime = new Date(); + } + + return { atime: atime, mtime: mtime }; +} + + +// http://stackoverflow.com/a/10589791/586382 +function validDate(date) { + return date instanceof Date && !isNaN(date.valueOf()); +} + +function futimes(fd, oldStat, newStat, cb) { + setTimeout(function() { + var stat = getUtimes(oldStat, newStat); + if (!stat) { + return cb(); + } + + // Set file `atime` and `mtime` fields + fs.futimes(fd, stat.atime, stat.mtime, cb); + }, 0); +} + + function writeContents(writePath, file, cb) { // If directory then mkdirp it if (file.isDirectory()) { @@ -36,26 +90,54 @@ function writeContents(writePath, file, cb) { cb(err, file); } - function written(err) { + function stat(fd) { + function complete2(err1) { + fs.close(fd, function(err2) { + complete(err1 || err2); + }); + } + + fs.fstat(fd, function(err, st) { + if (err) { + return complete2(err); + } + + var stat = file.stat; + var mode = getMode(st, stat); + if (mode == null) { + return futimes(fd, st, stat, complete2); + } + fs.fchmod(fd, mode, function(error) { + if (error) { + return complete2(error); + } + + futimes(fd, st, stat, complete2); + }); + }); + } + + + function written(err, fd) { if (isErrorFatal(err)) { return complete(err); } - if (!file.stat || typeof file.stat.mode !== 'number' || file.symlink) { + if (!file.stat) { return complete(); } - fs.stat(writePath, function(err, st) { + if (typeof fd === 'number') { + return stat(fd); + } + + fs.open(writePath, 'r', function(err, fd) { if (err) { return complete(err); } - var currentMode = (st.mode & parseInt('0777', 8)); - var expectedMode = (file.stat.mode & parseInt('0777', 8)); - if (currentMode === expectedMode) { - return complete(); - } - fs.chmod(writePath, expectedMode, complete); + + stat(fd); }); } @@ -64,8 +146,8 @@ function writeContents(writePath, file, cb) { return false; } + // Handle scenario for file overwrite failures. if (err.code === 'EEXIST' && file.flag === 'wx') { - // Handle scenario for file overwrite failures. return false; // "These aren't the droids you're looking for" } diff --git a/lib/dest/writeContents/writeBuffer.js b/lib/dest/writeContents/writeBuffer.js index daf0321b..88790eac 100644 --- a/lib/dest/writeContents/writeBuffer.js +++ b/lib/dest/writeContents/writeBuffer.js @@ -2,8 +2,6 @@ var fs = require('graceful-fs'); -var futimes = require('../../futimes'); - function writeBuffer(writePath, file, cb) { var stat = file.stat; @@ -12,19 +10,9 @@ function writeBuffer(writePath, file, cb) { return cb(err); } - fs.write(fd, file.contents, 0, file.contents.length, 0, function(error) { - if (error) { - return complete(error); - } - futimes(fd, stat, complete); + fs.write(fd, file.contents, 0, file.contents.length, 0, function(err) { + cb(err, fd); }); - - // Cleanup - function complete(err1) { - fs.close(fd, function(err2) { - cb(err1 || err2); - }); - } }); } diff --git a/lib/dest/writeContents/writeStream.js b/lib/dest/writeContents/writeStream.js index 2cfda5f2..a22b0cc3 100644 --- a/lib/dest/writeContents/writeStream.js +++ b/lib/dest/writeContents/writeStream.js @@ -3,20 +3,28 @@ var fs = require('graceful-fs'); var streamFile = require('../../src/getContents/streamFile'); -var futimes = require('../../futimes'); function writeStream(writePath, file, cb) { var stat = file.stat; + var mode = stat.mode; + var outStream; var outFD; - fs.open(writePath, 'w', file.stat.mode, function(err, fd) { + fs.open(writePath, 'w', mode, function(err, fd) { if (err) { cb(err); } + var opt = { + mode: mode, + flag: file.flag, + autoClose: false, + fd: fd, + }; + outFD = fd; - outStream = fs.createWriteStream(null, { fd: fd }); + outStream = fs.createWriteStream(null, opt); file.contents.once('error', complete); file.contents.once('end', readStreamEnd); @@ -35,14 +43,8 @@ function writeStream(writePath, file, cb) { return complete(error); } - futimes(outFD, stat, function(error) { - if (error) { - return complete(error); - } - - // All finished with WriteStream, close and clean up - outStream.end(); - }); + // All finished with WriteStream, close and clean up + outStream.end(); }); } @@ -54,7 +56,7 @@ function writeStream(writePath, file, cb) { outStream.removeListener('error', complete); outStream.removeListener('finish', complete); } - cb(err); + cb(err, outFD); } } diff --git a/lib/dest/writeContents/writeSymbolicLink.js b/lib/dest/writeContents/writeSymbolicLink.js index fdd229ac..74f8071d 100644 --- a/lib/dest/writeContents/writeSymbolicLink.js +++ b/lib/dest/writeContents/writeSymbolicLink.js @@ -8,7 +8,7 @@ function writeSymbolicLink(writePath, file, cb) { return cb(err); } - cb(null, file); + cb(); }); } diff --git a/lib/futimes.js b/lib/futimes.js deleted file mode 100644 index 53ebeceb..00000000 --- a/lib/futimes.js +++ /dev/null @@ -1,27 +0,0 @@ -'use strict'; - -var fs = require('graceful-fs'); - -// http://stackoverflow.com/a/10589791/586382 -function validDate(date) { - return date instanceof Date && !isNaN(date.valueOf()); -} - -function futimes(fd, stat, cb) { - // Given `mtime` is not valid, do nothing - var mtime = stat.mtime; - if (!validDate(mtime)) { - return cb(); - } - - // Given `atime` is not valid, assign a new one with current time - var atime = stat.atime; - if (!validDate(atime)) { - atime = new Date(); - } - - // Set file `atime` and `mtime` fields - fs.futimes(fd, atime, mtime, cb); -} - -module.exports = futimes; diff --git a/test/dest.js b/test/dest.js index 6300f0bc..1039ed70 100644 --- a/test/dest.js +++ b/test/dest.js @@ -2,7 +2,8 @@ var spies = require('./spy'); var chmodSpy = spies.chmodSpy; -var statSpy = spies.statSpy; +var fchmodSpy = spies.fchmodSpy; +var fstatSpy = spies.fstatSpy; var vfs = require('../'); @@ -21,8 +22,9 @@ require('mocha'); var wipeOut = function() { this.timeout(20000); spies.setError('false'); - statSpy.reset(); + fstatSpy.reset(); chmodSpy.reset(); + fchmodSpy.reset(); del.sync(path.join(__dirname, './fixtures/highwatermark')); del.sync(path.join(__dirname, './out-fixtures/')); }; @@ -784,10 +786,6 @@ describe('dest stream', function() { }, }); - // Node.js uses `utime()`, so `fs.utimes()` has a resolution of 1 second - expectedAtime.setMilliseconds(0); - expectedMtime.setMilliseconds(0); - var buffered = []; var onEnd = function() { @@ -908,6 +906,7 @@ describe('dest stream', function() { var inputPath = path.join(__dirname, './fixtures/test.coffee'); var inputBase = path.join(__dirname, './fixtures/'); var expectedPath = path.join(__dirname, './out-fixtures/test.coffee'); + var expectedFd = 12; var expectedContents = fs.readFileSync(inputPath); var expectedBase = path.join(__dirname, './out-fixtures'); var expectedMode = parseInt('722', 8); @@ -926,7 +925,7 @@ describe('dest stream', function() { fs.closeSync(fs.openSync(expectedPath, 'w')); spies.setError(function(mod, fn) { - if (fn === 'stat' && arguments[2] === expectedPath) { + if (fn === 'fstat' && arguments[2] === expectedFd) { return new Error('stat error'); } }); @@ -943,6 +942,7 @@ describe('dest stream', function() { var inputPath = path.join(__dirname, './fixtures/test.coffee'); var inputBase = path.join(__dirname, './fixtures/'); var expectedPath = path.join(__dirname, './out-fixtures/test.coffee'); + var expectedFd = 12; var expectedContents = fs.readFileSync(inputPath); var expectedBase = path.join(__dirname, './out-fixtures'); var expectedMode = parseInt('722', 8); @@ -961,7 +961,7 @@ describe('dest stream', function() { fs.closeSync(fs.openSync(expectedPath, 'w')); spies.setError(function(mod, fn) { - if (fn === 'chmod' && arguments[2] === expectedPath) { + if (fn === 'fchmod' && arguments[2] === expectedFd) { return new Error('chmod error'); } }); @@ -978,6 +978,7 @@ describe('dest stream', function() { var inputPath = path.join(__dirname, './fixtures/test.coffee'); var inputBase = path.join(__dirname, './fixtures/'); var expectedPath = path.join(__dirname, './out-fixtures/test.coffee'); + var expectedFd = 12; var expectedContents = fs.readFileSync(inputPath); var expectedBase = path.join(__dirname, './out-fixtures'); var expectedMode = parseInt('722', 8); @@ -994,14 +995,14 @@ describe('dest stream', function() { var expectedCount = 0; spies.setError(function(mod, fn) { - if (fn === 'stat' && arguments[2] === expectedPath) { + if (fn === 'fstat' && arguments[2] === expectedFd) { expectedCount++; } }); var onEnd = function() { expectedCount.should.equal(1); - should(chmodSpy.called).be.not.ok; + should(fchmodSpy.called).be.not.ok; realMode(fs.lstatSync(expectedPath).mode).should.equal(expectedMode); done(); }; @@ -1010,8 +1011,8 @@ describe('dest stream', function() { fs.closeSync(fs.openSync(expectedPath, 'w')); fs.chmodSync(expectedPath, expectedMode); - statSpy.reset(); - chmodSpy.reset(); + fstatSpy.reset(); + fchmodSpy.reset(); var stream = vfs.dest('./out-fixtures/', { cwd: __dirname }); var buffered = []; @@ -1026,6 +1027,7 @@ describe('dest stream', function() { var inputPath = path.join(__dirname, './fixtures/test.coffee'); var inputBase = path.join(__dirname, './fixtures/'); var expectedPath = path.join(__dirname, './out-fixtures/test.coffee'); + var expectedFd = 12; var expectedContents = fs.readFileSync(inputPath); var expectedBase = path.join(__dirname, './out-fixtures'); var expectedMode = parseInt('3722', 8); @@ -1043,14 +1045,14 @@ describe('dest stream', function() { var expectedCount = 0; spies.setError(function(mod, fn) { - if (fn === 'stat' && arguments[2] === expectedPath) { + if (fn === 'fstat' && arguments[2] === expectedFd) { expectedCount++; } }); var onEnd = function() { expectedCount.should.equal(1); - should(chmodSpy.called).be.not.ok; + should(fchmodSpy.called).be.not.ok; done(); }; @@ -1058,8 +1060,8 @@ describe('dest stream', function() { fs.closeSync(fs.openSync(expectedPath, 'w')); fs.chmodSync(expectedPath, expectedMode); - statSpy.reset(); - chmodSpy.reset(); + fstatSpy.reset(); + fchmodSpy.reset(); var stream = vfs.dest('./out-fixtures/', { cwd: __dirname }); var buffered = []; @@ -1248,6 +1250,8 @@ describe('dest stream', function() { var stream = vfs.dest('./out-fixtures/', { cwd: __dirname }); var bufferStream = through.obj(dataWrap(buffered.push.bind(buffered)), onEnd); + + stream.on('error', done); stream.pipe(bufferStream); stream.write(inputFile); stream.end(); diff --git a/test/spy.js b/test/spy.js index e25c10a5..ef565af8 100644 --- a/test/spy.js +++ b/test/spy.js @@ -25,5 +25,7 @@ module.exports = { errorfn = fn; }, chmodSpy: maybeCallAsync(fs, 'chmod'), + fchmodSpy: maybeCallAsync(fs, 'fchmod'), statSpy: maybeCallAsync(fs, 'stat'), + fstatSpy: maybeCallAsync(fs, 'fstat'), };