Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid race condition by using fs.open and fs.fstat, also allow file descriptor to be passed #125

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Prev Previous commit
Next Next commit
Ensure fd is closed in the following cases:
1. The `res` ends before it is passed to `send().pipe()`
2. The `res` ends after pipe but before the `open` event
jcready committed Feb 2, 2017
commit ff711b0a3fbdd4adbf67d658c6583aa653d6aab2
53 changes: 40 additions & 13 deletions index.js
Original file line number Diff line number Diff line change
@@ -24,6 +24,7 @@ var fs = require('fs')
var mime = require('mime')
var ms = require('ms')
var onFinished = require('on-finished')
var isFinished = onFinished.isFinished
var parseRange = require('range-parser')
var path = require('path')
var statuses = require('statuses')
@@ -181,6 +182,8 @@ function SendStream (req, path, options) {
? opts.fd
: null

this.autoClose = opts.autoClose !== false

if (!this._root && opts.from) {
this.from(opts.from)
}
@@ -476,7 +479,35 @@ SendStream.prototype.redirect = function redirect (path) {
}

/**
* Pipe to `res.
* End the stream.
*
* @private
*/

SendStream.prototype.end = function end () {
this.send = this.close
if (this._stream) this._stream.destroy()
if (this.autoClose) this.close()
}

/**
* Close the file descriptor.
*
* @private
*/

SendStream.prototype.close = function close () {
if (typeof this.fd !== 'number') return
var self = this
fs.close(this.fd, function (err) { /* istanbul ignore next */
if (err && !BAD_FD_REGEXP.test(err.code)) return self.onFileSystemError(err)
self.fd = null
self.emit('close')
})
}

/**
* Pipe to `res`.
*
* @param {Stream} res
* @return {Stream} res
@@ -492,18 +523,12 @@ SendStream.prototype.pipe = function pipe (res) {
this.res = res

// response finished, done with the fd
onFinished(res, function onfinished () {
var autoClose = self.options.autoClose !== false
if (self._stream) self._stream.destroy()
if (typeof self.fd === 'number' && autoClose) {
fs.close(self.fd, function (err) {
/* istanbul ignore next */
if (err && !BAD_FD_REGEXP.test(err.code)) return self.onFileSystemError(err)
self.fd = null
self.emit('close')
})
}
})
if (isFinished(res)) {
this.end()
return res
}

onFinished(res, this.end.bind(this))

if (typeof this.fd === 'number') {
fs.fstat(this.fd, function (err, stat) {
@@ -608,6 +633,8 @@ SendStream.prototype.send = function send (path, stat) {
var ranges = req.headers.range
var offset = options.start || 0

if (isFinished(res)) return this.end()

if (res._header) {
// impossible to send now
this.headersAlreadySent()
36 changes: 35 additions & 1 deletion test/send.js
Original file line number Diff line number Diff line change
@@ -24,6 +24,7 @@ var app = http.createServer(function (req, res) {
.pipe(res)
})

var noop = Function.prototype
var fsOpen = fs.open
var fsClose = fs.close
var fds = { opened: 0, closed: 0 }
@@ -934,10 +935,43 @@ describe('send(file, options)', function () {
.get('/anything')
.expect(500, done)
})

it('should close the file descriptor if the response ends before pipe', function (done) {
fs.open(path.join(fixtures, '/name.txt'), 'r', function (err, fd) {
if (err) return done(err)
var app = http.createServer(function (req, res) {
res.end()
send(req, req.url, req.url === '/fd' ? {fd: fd} : {root: 'test/fixtures'})
.on('close', done)
.pipe(res)
})

request(app)
.get('/nums')
.expect(200, function () {
request(app)
.get('/fd')
.expect(200, noop)
})
})
})

it('should close the file descriptor if the response ends immediately after pipe', function (done) {
var app = http.createServer(function (req, res) {
send(req, req.url, {root: 'test/fixtures'})
.on('close', done)
.pipe(res)
res.end()
})

request(app)
.get('/name.txt')
.expect(200, noop)
})
})

describe('autoClose', function () {
it('should prevent the file descriptor from being closed automatically', function (done) {
it('should prevent the file descriptor from being closed automatically if set to `false`', function (done) {
var resource = '/nums'
fs.open(path.join(fixtures, resource), 'r', function (err, fd) {
if (err) return done(err)