Instrumenting nodejs with AddressSanitizer fails resulting of memory leaks

[zyscoder]

• Version: v12.16.0
• Platform: Linux vul337 4.15.0-91-generic new design of error handling #92-Ubuntu SMP Fri Feb 28 11:09:48 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
• Subsystem:

What steps will reproduce the bug?

When building nodejs with AddressSanitizer,

CC="/path/to/afl-clang" CXX="/path/to/afl-clang++" LINK="/path/to/afl-clang++ -fuse-ld=lld" CCFLAGS="-fsanitize=address" CXXFLAGS="-fsanitize=address" LDFLAGS="-fsanitize=address" make

the following command will be run, and memory leaks occur:

LD_LIBRARY_PATH=/path/to/node/out/Release/lib.host:/path/to/node/out/Release/lib.target:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH; cd ../.; mkdir -p /path/to/node/out/Release/obj/gen; "/path/to/node/out/Release/mkcodecache" "/path/to/node/out/Release/obj/gen/node_code_cache.cc"

How often does it reproduce? Is there a required condition?

No. This potential bug can always be reproduced.

What is the expected behavior?

The executable of 'mkcodecache' tries to generate the file of 'node_code_cache.cc' when building nodejs, and the building process fails due to memory leaks of 'mkcodecache'. This problem should be handled otherwise a nodejs instrumented by Address Sanitizer cannot be built successfully.

What do you see instead?

Multiple stack dumps of memory leaks:

==72826==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 16384 byte(s) in 4 object(s) allocated from:
    #0 0x3108ce1 in __interceptor_calloc (/path/to/node/out/Release/mkcodecache+0x3108ce1)
    #1 0x4821af9 in v8::internal::BasicMemoryChunk::BasicMemoryChunk(unsigned long, unsigned long, unsigned long) (/path/to/node/out/Release/mkcodecache+0x4821af9)
    #2 0x47f383b in v8::internal::MemoryChunk::Initialize(v8::internal::Heap*, unsigned long, unsigned long, unsigned long, unsigned long, v8::internal::Executability, v8::internal::Space*, v8::internal::VirtualMemory) (/path/to/node/out/Release/mkcodecache+0x47f383b)
    #3 0x47eb8ea in v8::internal::MemoryAllocator::AllocateChunk(unsigned long, unsigned long, v8::internal::Executability, v8::internal::Space*) (/path/to/node/out/Release/mkcodecache+0x47eb8ea)
    #4 0x47eca8d in v8::internal::Page* v8::internal::MemoryAllocator::AllocatePage<(v8::internal::MemoryAllocator::AllocationMode)1, v8::internal::SemiSpace>(unsigned long, v8::internal::SemiSpace*, v8::internal::Executability) (/path/to/node/out/Release/mkcodecache+0x47eca8d)
    #5 0x4801895 in v8::internal::SemiSpace::Commit() (/path/to/node/out/Release/mkcodecache+0x4801895)
    #6 0x480153c in v8::internal::NewSpace::NewSpace(v8::internal::Heap*, v8::PageAllocator*, unsigned long, unsigned long) (/path/to/node/out/Release/mkcodecache+0x480153c)
    #7 0x4687500 in v8::internal::Heap::SetUpSpaces() (/path/to/node/out/Release/mkcodecache+0x4687500)
    #8 0x456f8ca in v8::internal::Isolate::Init(v8::internal::ReadOnlyDeserializer*, v8::internal::StartupDeserializer*) (/path/to/node/out/Release/mkcodecache+0x456f8ca)
    #9 0x4571838 in v8::internal::Isolate::InitWithSnapshot(v8::internal::ReadOnlyDeserializer*, v8::internal::StartupDeserializer*) (/path/to/node/out/Release/mkcodecache+0x4571838)
    #10 0x543af7e in v8::internal::Snapshot::Initialize(v8::internal::Isolate*) (/path/to/node/out/Release/mkcodecache+0x543af7e)
    #11 0x41df7cb in v8::Isolate::Initialize(v8::Isolate*, v8::Isolate::CreateParams const&) (/path/to/node/out/Release/mkcodecache+0x41df7cb)
    #12 0x41dff9e in v8::Isolate::New(v8::Isolate::CreateParams const&) (/path/to/node/out/Release/mkcodecache+0x41dff9e)
    #13 0x347f816 in main (/path/to/node/out/Release/mkcodecache+0x347f816)
    #14 0x7f39921d8b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
...

SUMMARY: AddressSanitizer: 118344 byte(s) leaked in 1206 allocation(s).
node.target.mk:13: recipe for target '/path/to/node/out/Release/obj/gen/node_code_cache.cc' failed
make[1]: *** [/path/to/node/out/Release/obj/gen/node_code_cache.cc] Error 1
make[1]: *** Waiting for unfinished jobs....
rm c37f73ea17746a11dc6852c9156cd99eb8565d70.intermediate 16d0a06d018bbaab79792789d3c3aadd33ec98cf.intermediate 3fe54b56864e2ab8dadc5290dbe04ab6d00cf977.intermediate 0459621a7970293331ab29d45be116d6b911f492.intermediate
Makefile:101: recipe for target 'node' failed
make: *** [node] Error 2

Additional information

[addaleax]

--- a/tools/code_cache/mkcodecache.cc
+++ b/tools/code_cache/mkcodecache.cc
@@ -49,8 +49,8 @@ int main(int argc, char* argv[]) {
 
   // Create a new Isolate and make it the current one.
   Isolate::CreateParams create_params;
-  create_params.array_buffer_allocator =
-      ArrayBuffer::Allocator::NewDefaultAllocator();
+  create_params.array_buffer_allocator_shared.reset(
+      ArrayBuffer::Allocator::NewDefaultAllocator());
   Isolate* isolate = Isolate::New(create_params);
   {
     Isolate::Scope isolate_scope(isolate);
@@ -65,6 +65,7 @@ int main(int argc, char* argv[]) {
     out << cache;
     out.close();
   }
+  isolate->Dispose();
 
   v8::V8::ShutdownPlatform();
   return 0;

Does this help? If yes, feel free to open a PR with it.

[zyscoder]

Thank you for your timely reply. Let me have a try.

[zyscoder]

@addaleax Your patch works well!

[mmarchini]

The executable of 'mkcodecache' tries to generate the file of 'node_code_cache.cc' when building nodejs, and the building process fails due to memory leaks of 'mkcodecache'. This problem should be handled otherwise a nodejs instrumented by Address Sanitizer cannot be built successfully.

Interesting. We recently introduced ASAN checks on our CI (https://github.com/nodejs/node/actions?query=workflow%3Atest-asan), and so far it hasn't failed. It's worth noting we use ./configure --enable-asan && make instead of setting the flags via CC and family. I wonder what's different and if we need to change anything in our config to increase coverage (or if this issue is exclusive of v12). I don't think we disabled halt_on_error during the build phase, so mkcodecache should fail if there's a ASAN violation.

[zyscoder]

@addaleax The patch does work well on the master branch. But I found that in v12.16.0, 'CreateParams' has no member of 'array_buffer_allocator_shared'.

[zyscoder]

@mmarchini I tried to build v12.16.0 in your way and succeeded.

[mmarchini]

Ok, so something is different when building with the flag and setting things up manually, or maybe there's a compiler's difference. Either way, this looks like something our CI should've flagged, nice catch!

[addaleax]

@zyscoder Right, on older release lines you may have to manually keep the reference to the ArrayBuffer::Allocator alive, with a separate std::unique_ptr or so. But as far as I am concerned, fixing this in master is good enough, as it’s not a “real” issue in the sense that Node.js users are affected by it.

Anyway, I’ve opened #32850 with the patch from above.

[mmarchini]

@zyscoder which clang version did you use?

[zyscoder]

@zyscoder which clang version did you use?

clang-8.0.0