Browse Source

chore: fix memory leak in `v8.serialize()` (#37021)

chore: fix memory leak in v8.serialize()
Shelley Vohr 2 years ago
parent
commit
c6b9340b89

+ 1 - 1
patches/node/fixup_for_wc_98-compat-extra-semi.patch

@@ -7,7 +7,7 @@ Wc++98-compat-extra-semi is turned on for Electron so this
 patch fixes that error in node.
 
 diff --git a/src/node_serdes.cc b/src/node_serdes.cc
-index 0cd76078218433b46c17f350e3ba6073987438cf..ba13061b6aa7fd8f877aa456db9d352a847e682a 100644
+index 97917c91c06dc47dfa6be2c194944cdc93e6bd7f..177390a24eb6490b128e22c104014e80f338c9d9 100644
 --- a/src/node_serdes.cc
 +++ b/src/node_serdes.cc
 @@ -32,7 +32,7 @@ namespace serdes {

+ 25 - 10
patches/node/support_v8_sandboxed_pointers.patch

@@ -155,7 +155,7 @@ index efbdbeabf5a6afb658cbdc7888f94952e55f4f71..8b37639361e8902d7e1481071d3ec24b
  
    // Delegate to V8's allocator for compatibility with the V8 memory cage.
 diff --git a/src/node_serdes.cc b/src/node_serdes.cc
-index 45a16d9de43703c2115dde85c9faae3a04be2a88..0cd76078218433b46c17f350e3ba6073987438cf 100644
+index 45a16d9de43703c2115dde85c9faae3a04be2a88..97917c91c06dc47dfa6be2c194944cdc93e6bd7f 100644
 --- a/src/node_serdes.cc
 +++ b/src/node_serdes.cc
 @@ -29,6 +29,11 @@ using v8::ValueSerializer;
@@ -219,17 +219,32 @@ index 45a16d9de43703c2115dde85c9faae3a04be2a88..0cd76078218433b46c17f350e3ba6073
  Maybe<bool> SerializerContext::WriteHostObject(Isolate* isolate,
                                                 Local<Object> input) {
    MaybeLocal<Value> ret;
-@@ -211,7 +240,12 @@ void SerializerContext::ReleaseBuffer(const FunctionCallbackInfo<Value>& args) {
+@@ -209,9 +238,14 @@ void SerializerContext::ReleaseBuffer(const FunctionCallbackInfo<Value>& args) {
+   // Note: Both ValueSerializer and this Buffer::New() variant use malloc()
+   // as the underlying allocator.
    std::pair<uint8_t*, size_t> ret = ctx->serializer_.Release();
-   auto buf = Buffer::New(ctx->env(),
-                          reinterpret_cast<char*>(ret.first),
+-  auto buf = Buffer::New(ctx->env(),
+-                         reinterpret_cast<char*>(ret.first),
 -                         ret.second);
-+                         ret.second,
-+                         [](char* data, void* hint){
-+                           if (data)
-+                             GetAllocator()->Free(data, reinterpret_cast<size_t>(hint));
-+                         },
-+                         reinterpret_cast<void*>(ctx->last_length_));
++  std::unique_ptr<v8::BackingStore> bs =
++      v8::ArrayBuffer::NewBackingStore(reinterpret_cast<char*>(ret.first), ret.second,
++        [](void* data, size_t length, void* deleter_data) {
++          if (data) GetAllocator()->Free(reinterpret_cast<char*>(data), length);
++        }, nullptr);
++  Local<ArrayBuffer> ab = v8::ArrayBuffer::New(ctx->env()->isolate(), std::move(bs));
++
++  auto buf = Buffer::New(ctx->env(), ab, 0, ret.second);
  
    if (!buf.IsEmpty()) {
      args.GetReturnValue().Set(buf.ToLocalChecked());
+diff --git a/test/parallel/test-v8-serialize-leak.js b/test/parallel/test-v8-serialize-leak.js
+index a90c398adcdaf30491a0fecdcf00895038d62e69..f5b8a1430ad2033eae06ca0157af2fb51d3f36a5 100644
+--- a/test/parallel/test-v8-serialize-leak.js
++++ b/test/parallel/test-v8-serialize-leak.js
+@@ -23,5 +23,5 @@ const after = process.memoryUsage.rss();
+ if (process.config.variables.asan) {
+   assert(after < before * 10, `asan: before=${before} after=${after}`);
+ } else {
+-  assert(after < before * 2, `before=${before} after=${after}`);
++  assert(after < before * 3, `before=${before} after=${after}`);
+ }

+ 0 - 1
script/node-disabled-tests.json

@@ -137,7 +137,6 @@
   "parallel/test-worker-debug",
   "parallel/test-worker-init-failure",
   "parallel/test-worker-stdio",
-  "parallel/test-v8-serialize-leak",
   "parallel/test-zlib-unused-weak",
   "report/test-report-fatalerror-oomerror-set",
   "report/test-report-fatalerror-oomerror-directory",