Skip to content

Commit

Permalink
fs: don't throw in read if buffer too big
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
evanlucas authored and rvagg committed Oct 26, 2015
1 parent 9d8d752 commit 2bb1475
Show file tree
Hide file tree
Showing 2 changed files with 74 additions and 3 deletions.
19 changes: 16 additions & 3 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
};
}

Expand All @@ -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;
Expand Down
58 changes: 58 additions & 0 deletions test/parallel/test-fs-read-buffer-tostring-fail.js
Original file line number Diff line number Diff line change
@@ -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;
});

0 comments on commit 2bb1475

Please sign in to comment.