From 4c2d9a63ba9568b3605d66b9ba3d9fa580591f1a Mon Sep 17 00:00:00 2001 From: Keyhan Vakil Date: Fri, 22 Jul 2022 02:11:45 +0000 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/40243 --- src/node_buffer.cc | 1 + .../test-v8-serialize-buffer-too-large.js | 32 +++++++++++++++++++ 2 files changed, 33 insertions(+) create mode 100755 test/pummel/test-v8-serialize-buffer-too-large.js diff --git a/src/node_buffer.cc b/src/node_buffer.cc index 5b2186feb8c707..aec97f15e2c809 100644 --- a/src/node_buffer.cc +++ b/src/node_buffer.cc @@ -497,6 +497,7 @@ MaybeLocal New(Environment* env, if (length > kMaxLength) { Isolate* isolate(env->isolate()); isolate->ThrowException(ERR_BUFFER_TOO_LARGE(isolate)); + free(data); return Local(); } } diff --git a/test/pummel/test-v8-serialize-buffer-too-large.js b/test/pummel/test-v8-serialize-buffer-too-large.js new file mode 100755 index 00000000000000..3498b9b18ad6df --- /dev/null +++ b/test/pummel/test-v8-serialize-buffer-too-large.js @@ -0,0 +1,32 @@ +'use strict'; +const common = require('../common'); + +// On IBMi, the rss memory always returns zero +if (common.isIBMi) + common.skip('On IBMi, the rss memory always returns zero'); + +const v8 = require('v8'); +const { kMaxLength } = require('buffer'); + +const PADDING_STRING = 'a'.repeat(3000); + +const i = 22; +const toSerialize = {}; + +for (let j = 0; j < 2 ** i; j++) { + toSerialize[j] = PADDING_STRING; +} + +const assert = require('assert'); + +const rssBefore = process.memoryUsage.rss(); + +// Fail to serialize a few times. +const expectedError = { code: 'ERR_BUFFER_TOO_LARGE' }; +assert.throws(() => v8.serialize(toSerialize), expectedError); +assert.throws(() => v8.serialize(toSerialize), expectedError); +assert.throws(() => v8.serialize(toSerialize), expectedError); + +// Check that (at least some of) the memory got freed. +const rssAfter = process.memoryUsage.rss(); +assert(rssAfter - rssBefore <= 2 * kMaxLength);