Skip to content

Commit

Permalink
src: re-add Realloc() shrink after reading stream data
Browse files Browse the repository at this point in the history
This would otherwise keep a lot of unused memory lying around,
and in particular add up to a page per chunk of memory overhead
for network reads, potentially opening a DoS vector if the resulting
`Buffer` objects are kept around indefinitely (e.g. stored in a list
and not concatenated until the socket finishes).

This fixes CVE-2018-7164.

Refs: https://github.com/nodejs-private/security/issues/186
Refs: 7c4b09b
PR-URL: https://github.com/nodejs-private/node-private/pull/128
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
  • Loading branch information
addaleax authored and evanlucas committed Jun 12, 2018
1 parent e87bf62 commit 9ba8ed1
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 1 deletion.
3 changes: 2 additions & 1 deletion src/stream_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,9 @@ void EmitToJSStreamListener::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
}

CHECK_LE(static_cast<size_t>(nread), buf.len);
char* base = Realloc(buf.base, nread);

Local<Object> obj = Buffer::New(env, buf.base, nread).ToLocalChecked();
Local<Object> obj = Buffer::New(env, base, nread).ToLocalChecked();
stream->CallJSOnreadMethod(nread, obj);
}

Expand Down
41 changes: 41 additions & 0 deletions test/sequential/test-net-bytes-per-incoming-chunk-overhead.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Flags: --expose-gc
'use strict';

const common = require('../common');
const assert = require('assert');
const net = require('net');

// Tests that, when receiving small chunks, we do not keep the full length
// of the original allocation for the libuv read call in memory.

let client;
let baseRSS;
const receivedChunks = [];
const N = 250000;

const server = net.createServer(common.mustCall((socket) => {
baseRSS = process.memoryUsage().rss;

socket.setNoDelay(true);
socket.on('data', (chunk) => {
receivedChunks.push(chunk);
if (receivedChunks.length < N) {
client.write('a');
} else {
client.end();
server.close();
}
});
})).listen(0, common.mustCall(() => {
client = net.connect(server.address().port);
client.setNoDelay(true);
client.write('hello!');
}));

process.on('exit', () => {
global.gc();
const bytesPerChunk =
(process.memoryUsage().rss - baseRSS) / receivedChunks.length;
// We should always have less than one page (usually ~ 4 kB) per chunk.
assert(bytesPerChunk < 512, `measured ${bytesPerChunk} bytes per chunk`);
});

0 comments on commit 9ba8ed1

Please sign in to comment.