Skip to content

Commit

Permalink
Fix file descriptor leak on sync errors
Browse files Browse the repository at this point in the history
  • Loading branch information
iarna committed Jan 23, 2019
1 parent 8ea1b4a commit 3365a4d
Showing 1 changed file with 22 additions and 18 deletions.
40 changes: 22 additions & 18 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -180,27 +180,30 @@ function writeFileSync (filename, data, options) {
}
var tmpfile = getTmpname(filename)

try {
if (!options.mode || !options.chown) {
// Either mode or chown is not explicitly set
// Default behavior is to copy it from original file
try {
var stats = fs.statSync(filename)
options = Object.assign({}, options)
if (!options.mode) {
options.mode = stats.mode
}
if (!options.chown && process.getuid) {
options.chown = { uid: stats.uid, gid: stats.gid }
}
} catch (ex) {
// ignore stat errors
if (!options.mode || !options.chown) {
// Either mode or chown is not explicitly set
// Default behavior is to copy it from original file
try {
var stats = fs.statSync(filename)
options = Object.assign({}, options)
if (!options.mode) {
options.mode = stats.mode
}
if (!options.chown && process.getuid) {
options.chown = { uid: stats.uid, gid: stats.gid }
}
} catch (ex) {
// ignore stat errors
}
}

var fd
var cleanup = cleanupOnExit(tmpfile)
var removeOnExitHandler = onExit(cleanup)

try {

var cleanup = cleanupOnExit(tmpfile)
var removeOnExitHandler = onExit(cleanup)
var fd = fs.openSync(tmpfile, 'w', options.mode)
fd = fs.openSync(tmpfile, 'w', options.mode)
if (Buffer.isBuffer(data)) {
fs.writeSync(fd, data, 0, data.length, 0)
} else if (data != null) {
Expand All @@ -215,6 +218,7 @@ function writeFileSync (filename, data, options) {
fs.renameSync(tmpfile, filename)
removeOnExitHandler()
} catch (err) {
if (fd) fs.closeSync(fd)

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 27, 2019

Contributor

@iarna I wonder if this is wrong? Getting lots of

    Failure message: EBADF: bad file descriptor, close
      at Object.closeSync (D:/a/1/jest/node_modules/graceful-fs/graceful-fs.js:52:27)

which I think is probably from this?

This comment has been minimized.

Copy link
@iarna

iarna Jan 28, 2019

Author Contributor

It seems likely.... we should wrap that and not step on whatever the underlying error was.

This comment has been minimized.

Copy link
@lukeapage

lukeapage Jan 29, 2019

Contributor

Is it because fd May have already been closed?

This comment has been minimized.

Copy link
@SimenB

SimenB Jan 29, 2019

Contributor

just a try-catch (with empty catch) around it makes sense, I think

removeOnExitHandler()
cleanup()
throw err
Expand Down

0 comments on commit 3365a4d

Please # to comment.