fs: don't throw in read if buffer too big

evanlucas · evanlucas · commit 57359cd1e450 · 2015-10-25T21:24:12.000-05:00
If the resulting buffer.toString() call in fs.read throws, catch the error and pass it back in the callback. This issue only presents itself when fs.read is called using the legacy string interface: fs.read(fd, length, position, encoding, callback) PR-URL: #3503 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
diff --git a/lib/fs.js b/lib/fs.js
@@ -599,10 +599,13 @@ fs.read = function(fd, buffer, offset, length, position, callback) {
 
     callback = function(err, bytesRead) {
       if (!cb) return;
+      if (err) return cb(err);
 
-      var str = (bytesRead > 0) ? buffer.toString(encoding, 0, bytesRead) : '';
-
-      (cb)(err, str, bytesRead);
+      if (bytesRead > 0) {
+        tryToStringWithEnd(buffer, encoding, bytesRead, cb);
+      } else {
+        (cb)(err, '', bytesRead);
+      }
     };
   }
 
@@ -617,6 +620,16 @@ fs.read = function(fd, buffer, offset, length, position, callback) {
   binding.read(fd, buffer, offset, length, position, req);
 };
 
+function tryToStringWithEnd(buf, encoding, end, callback) {
+  var e;
+  try {
+    buf = buf.toString(encoding, 0, end);
+  } catch (err) {
+    e = err;
+  }
+  callback(e, buf, end);
+}
+
 fs.readSync = function(fd, buffer, offset, length, position) {
   var legacy = false;
   var encoding;
diff --git a/test/parallel/test-fs-read-buffer-tostring-fail.js b/test/parallel/test-fs-read-buffer-tostring-fail.js
@@ -0,0 +1,58 @@
+'use strict';
+
+const common = require('../common');
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const Buffer = require('buffer').Buffer;
+const kStringMaxLength = process.binding('buffer').kStringMaxLength;
+const kMaxLength = process.binding('buffer').kMaxLength;
+
+var fd;
+
+common.refreshTmpDir();
+
+const file = path.join(common.tmpDir, 'toobig2.txt');
+const stream = fs.createWriteStream(file, {
+  flags: 'a'
+});
+
+const size = kStringMaxLength / 200;
+const a = new Buffer(size).fill('a');
+
+for (var i = 0; i < 201; i++) {
+  stream.write(a);
+}
+
+stream.end();
+stream.on('finish', common.mustCall(function() {
+  fd = fs.openSync(file, 'r');
+  fs.read(fd, kStringMaxLength + 1, 0, 'utf8', common.mustCall(function(err) {
+    assert.ok(err instanceof Error);
+    assert.strictEqual('toString failed', err.message);
+  }));
+}));
+
+function destroy() {
+  try {
+    // Make sure we close fd and unlink the file
+    fs.closeSync(fd);
+    fs.unlinkSync(file);
+  } catch (err) {
+    // it may not exist
+  }
+}
+
+process.on('exit', destroy);
+
+process.on('SIGINT', function() {
+  destroy();
+  process.exit();
+});
+
+// To make sure we don't leave a very large file
+// on test machines in the event this test fails.
+process.on('uncaughtException', function(err) {
+  destroy();
+  throw err;
+});
