Skip to content

Commit 4c2d9a6

Browse files
committed
buffer: do not leak memory if buffer is too big
A recent pull request changed this method to throw when the buffer was too big, but this meant that the `free` finalizer would never get called, leading to a memory leak. Included is a test which provokes this behavior using `v8.serialize`. Technically the behavior is allocator-dependent, but I suspect any reasonable allocator will choose to free (or at the very least, reuse) the 1 GiB memory. Refs: nodejs#40243
1 parent 0484022 commit 4c2d9a6

File tree

2 files changed

+33
-0
lines changed

2 files changed

+33
-0
lines changed

src/node_buffer.cc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -497,6 +497,7 @@ MaybeLocal<Object> New(Environment* env,
497497
if (length > kMaxLength) {
498498
Isolate* isolate(env->isolate());
499499
isolate->ThrowException(ERR_BUFFER_TOO_LARGE(isolate));
500+
free(data);
500501
return Local<Object>();
501502
}
502503
}
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
'use strict';
2+
const common = require('../common');
3+
4+
// On IBMi, the rss memory always returns zero
5+
if (common.isIBMi)
6+
common.skip('On IBMi, the rss memory always returns zero');
7+
8+
const v8 = require('v8');
9+
const { kMaxLength } = require('buffer');
10+
11+
const PADDING_STRING = 'a'.repeat(3000);
12+
13+
const i = 22;
14+
const toSerialize = {};
15+
16+
for (let j = 0; j < 2 ** i; j++) {
17+
toSerialize[j] = PADDING_STRING;
18+
}
19+
20+
const assert = require('assert');
21+
22+
const rssBefore = process.memoryUsage.rss();
23+
24+
// Fail to serialize a few times.
25+
const expectedError = { code: 'ERR_BUFFER_TOO_LARGE' };
26+
assert.throws(() => v8.serialize(toSerialize), expectedError);
27+
assert.throws(() => v8.serialize(toSerialize), expectedError);
28+
assert.throws(() => v8.serialize(toSerialize), expectedError);
29+
30+
// Check that (at least some of) the memory got freed.
31+
const rssAfter = process.memoryUsage.rss();
32+
assert(rssAfter - rssBefore <= 2 * kMaxLength);

0 commit comments

Comments
 (0)