Possible undefined behavior on buffer's CallbackInfo

[mmarchini]

If I build Node.js with ASAN and I run cctest filtering by ``EnvironmentTest.BufferWithFreeCallbackIsDetached`, I get the following memory leak:

$ ./out/Debug/cctest --gtest_filter=EnvironmentTest.BufferWithFreeCallbackIsDetached
Running main() from ../../test/cctest/gtest/gtest_main.cc
Note: Google Test filter = EnvironmentTest.BufferWithFreeCallbackIsDetached
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from EnvironmentTest
[ RUN      ] EnvironmentTest.BufferWithFreeCallbackIsDetached
[       OK ] EnvironmentTest.BufferWithFreeCallbackIsDetached (204 ms)
[----------] 1 test from EnvironmentTest (204 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test suite ran. (206 ms total)
[  PASSED  ] 1 test.

=================================================================
==8724==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 40 byte(s) in 1 object(s) allocated from:
    #0 0xe00b5d in operator new(unsigned long) (/nodejs/out/Debug/cctest+0xe00b5d)
    #1 0x1e9c710 in node::Buffer::(anonymous namespace)::CallbackInfo::New(node::Environment*, v8::Local<v8::ArrayBuffer>, void (*)(char*, void*), char*, void*) /nodejs/out/Debug/../../src/node_buffer.cc:117:10
    #2 0x1e9c324 in node::Buffer::New(node::Environment*, char*, unsigned long, void (*)(char*, void*), void*) /nodejs/out/Debug/../../src/node_buffer.cc:433:3
    #3 0x1e9b7b8 in node::Buffer::New(v8::Isolate*, char*, unsigned long, void (*)(char*, void*), void*) /nodejs/out/Debug/../../src/node_buffer.cc:398:7
    #4 0x1c6c364 in EnvironmentTest_BufferWithFreeCallbackIsDetached_Test::TestBody() /nodejs/out/Debug/../../test/cctest/test_environment.cc:228:39
    #5 0x1bf9cfe in void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:3913:10
    #6 0x1bc0743 in void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*) /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:3968:12
    #7 0x1b85209 in testing::Test::Run() /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:3988:5
    #8 0x1b86c09 in testing::TestInfo::Run() /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:4164:11
    #9 0x1b87c1f in testing::TestSuite::Run() /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:4294:28
    #10 0x1b9d7fa in testing::internal::UnitTestImpl::RunAllTests() /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:6752:44
    #11 0x1c024c9 in bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:3913:10
    #12 0x1bc6663 in bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*) /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:3968:12
    #13 0x1b9cc06 in testing::UnitTest::Run() /nodejs/out/Debug/../../test/cctest/gtest/gtest-all.cc:6340:10
    #14 0x1c0e8d0 in RUN_ALL_TESTS() /nodejs/out/Debug/../../test/cctest/gtest/gtest.h:14896:46
    #15 0x1c0e816 in main /nodejs/out/Debug/../../test/cctest/gtest/gtest_main.cc:45:10
    #16 0x7f377ed4e1e2 in __libc_start_main /build/glibc-4WA41p/glibc-2.30/csu/../csu/libc-start.c:308:16

SUMMARY: AddressSanitizer: 40 byte(s) leaked in 1 allocation(s).

I tried to fix this by running a GC at the end of the test:

diff --git a/test/cctest/test_environment.cc b/test/cctest/test_environment.cc
index 90c5cff5e0..6a30eec2a8 100644
--- a/test/cctest/test_environment.cc
+++ b/test/cctest/test_environment.cc
@@ -214,30 +214,34 @@ TEST_F(EnvironmentTest, SetImmediateCleanup) {
 static char hello[] = "hello";
 
 TEST_F(EnvironmentTest, BufferWithFreeCallbackIsDetached) {
-  // Test that a Buffer allocated with a free callback is detached after
-  // its callback has been called.
-  const v8::HandleScope handle_scope(isolate_);
-  const Argv argv;
-
   int callback_calls = 0;
-
-  v8::Local<v8::ArrayBuffer> ab;
   {
-    Env env {handle_scope, argv};
-    v8::Local<v8::Object> buf_obj = node::Buffer::New(
-        isolate_,
-        hello,
-        sizeof(hello),
-        [](char* data, void* hint) {
-          CHECK_EQ(data, hello);
-          ++*static_cast<int*>(hint);
-        },
-        &callback_calls).ToLocalChecked();
-    CHECK(buf_obj->IsUint8Array());
-    ab = buf_obj.As<v8::Uint8Array>()->Buffer();
-    CHECK_EQ(ab->ByteLength(), sizeof(hello));
-  }
+    // Test that a Buffer allocated with a free callback is detached after
+    // its callback has been called.
+    const v8::HandleScope handle_scope(isolate_);
+    const Argv argv;
 
-  CHECK_EQ(callback_calls, 1);
-  CHECK_EQ(ab->ByteLength(), 0);
+
+    v8::Local<v8::ArrayBuffer> ab;
+    {
+      Env env {handle_scope, argv};
+      v8::Local<v8::Object> buf_obj = node::Buffer::New(
+          isolate_,
+          hello,
+          sizeof(hello),
+          [](char* data, void* hint) {
+            CHECK_EQ(data, hello);
+            ++*static_cast<int*>(hint);
+          },
+          &callback_calls).ToLocalChecked();
+      CHECK(buf_obj->IsUint8Array());
+      ab = buf_obj.As<v8::Uint8Array>()->Buffer();
+      CHECK_EQ(ab->ByteLength(), sizeof(hello));
+    }
+
+    CHECK_EQ(callback_calls, 1);
+    CHECK_EQ(ab->ByteLength(), 0);
+  }
+  v8::V8::SetFlagsFromString("--expose-gc");
+  isolate_->RequestGarbageCollectionForTesting(v8::Isolate::kFullGarbageCollection);
 }

Except now we start to get a undefined behavior, because GC will delete the object, triggering the CallbackInfo::~CallbackInfo, which will try to remove the cleanup hook from our env, but since we already freed the Environemnt, it will not work (and since this is an ASAN build, not we get heap-use-after-free).

I think this is a legitimate (although unlikely) undefined behavior (not for node, but for embedders), but I'm not sure how to fix it. Or maybe I'm misunderstanding the test/Buffer/CallbackInfo code (which is very likely).

[addaleax]

@mmarchini Thank you for being so thorough! 💙 I think this was just me mixing up the WeakCallback() overloads, one deletes the CallbackInfo and one doesn’t, and the cleanup hook called the one that doesn’t.

Fix is in #32405 :)