fs: fix file descriptor leak in sync functions

Fixes #3202.

lib/fs.js

@@ -159,17 +159,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;
@@ -680,11 +682,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);
 };
 
 fs.appendFile = function(path, data, encoding_, callback) {
@@ -708,20 +712,14 @@ fs.appendFileSync = function(path, data, encoding) {
   var position = null;
   var length = data.length;
 
-  while (written < length) {
-    try {
+  try {
+    while (written < length) {
       written += fs.writeSync(fd, data, written, length - written, position);
-    } catch (e) {
-      try {
-        fs.closeSync(fd);
-      } catch (e) {
-        // swallow exception
-      }
-      throw e;
+      position += written; // XXX not safe with multiple concurrent writers?
     }
-    position += written;
+  } finally {
+    fs.closeSync(fd);
   }
-  fs.closeSync(fd);
 };
 
 function errnoException(errorno, syscall) {

test/simple/test-fs-sync-fd-leak.js

@@ -0,0 +1,65 @@
+// 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');
+});
+ensureThrows(function() {
+  fs.appendFileSync('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);
+}