fs: fix file descriptor leak in sync functions

Fixes #3202. This is a back-port of commit 4e290e4.
nodejs · May 3, 2012 · df2c5fa · df2c5fa
1 parent 47d6a94
commit df2c5fa
Show file tree

Hide file tree

Showing 2 changed files with 81 additions and 15 deletions.
diff --git a/lib/fs.js b/lib/fs.js
@@ -131,17 +131,19 @@ fs.readFileSync = function(path, encoding) {
   var nread = 0;
   var lastRead = 0;
 
-  do {
-    if (lastRead) {
-      buffer._bytesRead = lastRead;
-      nread += lastRead;
-      buffers.push(buffer);
-    }
-    var buffer = new Buffer(4048);
-    lastRead = fs.readSync(fd, buffer, 0, buffer.length, null);
-  } while (lastRead > 0);
-
-  fs.closeSync(fd);
+  try {
+    do {
+      if (lastRead) {
+        buffer._bytesRead = lastRead;
+        nread += lastRead;
+        buffers.push(buffer);
+      }
+      var buffer = new Buffer(4048);
+      lastRead = fs.readSync(fd, buffer, 0, buffer.length, null);
+    } while (lastRead > 0);
+  } finally {
+    fs.closeSync(fd);
+  }
 
   if (buffers.length > 1) {
     var offset = 0;
@@ -629,11 +631,13 @@ fs.writeFileSync = function(path, data, encoding) {
   }
   var written = 0;
   var length = data.length;
-  //writeSync(fd, buffer, offset, length, position)
-  while (written < length) {
-    written += fs.writeSync(fd, data, written, length - written, written);
+  try {
+    while (written < length) {
+      written += fs.writeSync(fd, data, written, length - written, written);
+    }
+  } finally {
+    fs.closeSync(fd);
   }
-  fs.closeSync(fd);
 };
 
 

diff --git a/test/simple/test-fs-sync-fd-leak.js b/test/simple/test-fs-sync-fd-leak.js
@@ -0,0 +1,62 @@
+// 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');
+
+// ensure that (read|write|append)FileSync() closes the file descriptor
+fs.openSync = function() {
+  return 42;
+};
+fs.closeSync = function(fd) {
+  assert.equal(fd, 42);
+  close_called++;
+};
+fs.readSync = function() {
+  throw new Error('BAM');
+};
+fs.writeSync = function() {
+  throw new Error('BAM');
+};
+
+ensureThrows(function() {
+  fs.readFileSync('dummy');
+});
+ensureThrows(function() {
+  fs.writeFileSync('dummy', 'xxx');
+});
+
+var close_called = 0;
+function ensureThrows(cb) {
+  var got_exception = false;
+
+  close_called = 0;
+  try {
+    cb();
+  } catch (e) {
+    assert.equal(e.message, 'BAM');
+    got_exception = true;
+  }
+
+  assert.equal(close_called, 1);
+  assert.equal(got_exception, true);
+}