From 47d6a946562b7825579211b0d617aee32bdf6f74 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 2 May 2012 04:24:38 +0200 Subject: [PATCH] fs: fix ReadStream / WriteStream double close bug * Calling fs.ReadStream.destroy() or fs.WriteStream.destroy() twice would close the file descriptor twice. That's bad because the file descriptor may have been repurposed in the mean time. * A bad value check in fs.ReadStream.prototype.destroy() would prevent a stream created with fs.createReadStream({fd:0}) from getting closed. --- lib/fs.js | 16 +++--- test/simple/test-fs-stream-double-close.js | 61 ++++++++++++++++++++++ 2 files changed, 71 insertions(+), 6 deletions(-) create mode 100644 test/simple/test-fs-stream-double-close.js diff --git a/lib/fs.js b/lib/fs.js index a62e083293..09fbca83fa 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -1167,6 +1167,8 @@ ReadStream.prototype._emitData = function(d) { ReadStream.prototype.destroy = function(cb) { var self = this; + + if (!this.readable) return; this.readable = false; function close() { @@ -1182,10 +1184,10 @@ ReadStream.prototype.destroy = function(cb) { }); } - if (this.fd) { - close(); - } else { + if (this.fd === null) { this.addListener('open', close); + } else { + close(); } }; @@ -1367,6 +1369,8 @@ WriteStream.prototype.end = function(data, encoding, cb) { WriteStream.prototype.destroy = function(cb) { var self = this; + + if (!this.writable) return; this.writable = false; function close() { @@ -1382,10 +1386,10 @@ WriteStream.prototype.destroy = function(cb) { }); } - if (this.fd) { - close(); - } else { + if (this.fd === null) { this.addListener('open', close); + } else { + close(); } }; diff --git a/test/simple/test-fs-stream-double-close.js b/test/simple/test-fs-stream-double-close.js new file mode 100644 index 0000000000..89386b6e64 --- /dev/null +++ b/test/simple/test-fs-stream-double-close.js @@ -0,0 +1,61 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); +var fs = require('fs'); + +test1(fs.createReadStream(__filename)); +test2(fs.createReadStream(__filename)); +test3(fs.createReadStream(__filename)); + +test1(fs.createWriteStream(common.tmpDir + '/dummy1')); +test2(fs.createWriteStream(common.tmpDir + '/dummy2')); +test3(fs.createWriteStream(common.tmpDir + '/dummy3')); + +function test1(stream) { + stream.destroy(); + stream.destroy(); +} + +function test2(stream) { + stream.destroy(); + stream.on('open', function(fd) { + stream.destroy(); + open_cb_called++; + }); + process.on('exit', function() { + assert.equal(open_cb_called, 1); + }); + var open_cb_called = 0; +} + +function test3(stream) { + stream.on('open', function(fd) { + stream.destroy(); + stream.destroy(); + open_cb_called++; + }); + process.on('exit', function() { + assert.equal(open_cb_called, 1); + }); + var open_cb_called = 0; +}