src: Initialize fields accessed by AsyncHooks before AsyncHooks

[jkrems]

This fixes an issue that was surfaced by an address sanitizer (asan) run:

.../cxx_atomic_impl.h:387:12: runtime error: load of value 171, which is not a valid value for type 'bool'
    #0 0x56532ea65fab in can_call_into_js src/env-inl.h:614
    #1 0x56532ea65fab in node::AsyncHooks::clear_async_id_stack() src/env.cc:171:14
    #2 0x56532eab1aed in node::AsyncHooks::AsyncHooks(v8::Isolate*, node::AsyncHooks::SerializeInfo const*) src/env.cc:1393:5
    #3 0x56532eaa8ea0 in node::Environment::Environment(node::IsolateData*, v8::Isolate*, std::__u::vector<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::allocator<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>> const&, std::__u::vector<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::allocator<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>> const&, node::EnvSerializeInfo const*, node::EnvironmentFlags::Flags, node::ThreadId) src/env.cc:650:7
    #4 0x56532eaaa6bb in node::Environment::Environment(node::IsolateData*, v8::Local<v8::Context>, std::__u::vector<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::allocator<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>> const&, std::__u::vector<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::allocator<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>> const&, node::EnvSerializeInfo const*, node::EnvironmentFlags::Flags, node::ThreadId) src/env.cc:741:7
    #5 0x56532e9646db in node::CreateEnvironment(node::IsolateData*, v8::Local<v8::Context>, std::__u::vector<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::allocator<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>> const&, std::__u::vector<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>, std::__u::allocator<std::__u::basic_string<char, std::__u::char_traits<char>, std::__u::allocator<char>>>> const&, node::EnvironmentFlags::Flags, node::ThreadId, std::__u::unique_ptr<node::InspectorParentHandle, std::__u::default_delete<node::InspectorParentHandle>>) src/api/environment.cc:401:26
    #6 0x56532ec2bed1 in node::NodeMainInstance::CreateMainEnvironment(int*) src/node_main_instance.cc:184:9
    #7 0x56532ec2ba05 in node::NodeMainInstance::Run() src/node_main_instance.cc:118:7
    #8 0x56532eb3637d in node::LoadSnapshotDataAndRun(node::SnapshotData const**, node::InitializationResult const*) src/node.cc:1231:29

Afaict #44669 added reads to is_stopping_ and can_call_into_js_ but they were triggered from inside of the AsyncHooks constructor. Because of the field order in Environment, the AsyncHooks constructor ran before these fields had been initialized, leading to reads of uninitialized memory.

Moving the fields up seemed like the simplest fix.

[joyeecheung]

AFAICT the problematic code path is supposed to be a noop when it's reachable (clear_async_id_stack() is only called from the constructor when the AsyncHooks is not deserialized, then its js_execution_async_resources_ is supposed to be empty, so there's actually no need to check env()->can_call_into_js() at all because we don't need to do anything to the empty handle). Alternative fix:

diff --git a/src/env.cc b/src/env.cc
index a62b8ef48b..56f4344d9e 100644
--- a/src/env.cc
+++ b/src/env.cc
@@ -170,15 +170,13 @@ bool AsyncHooks::pop_async_context(double async_id) {
 }

 void AsyncHooks::clear_async_id_stack() {
-  if (env()->can_call_into_js()) {
+  if (!js_execution_async_resources_.IsEmpty() && env()->can_call_into_js()) {
     Isolate* isolate = env()->isolate();
     HandleScope handle_scope(isolate);
-    if (!js_execution_async_resources_.IsEmpty()) {
-      USE(PersistentToLocal::Strong(js_execution_async_resources_)
-              ->Set(env()->context(),
-                    env()->length_string(),
-                    Integer::NewFromUnsigned(isolate, 0)));
-    }
+    USE(PersistentToLocal::Strong(js_execution_async_resources_)
+            ->Set(env()->context(),
+                  env()->length_string(),
+                  Integer::NewFromUnsigned(isolate, 0)));
   }

   native_execution_async_resources_.clear();

But I am also fine with the current fix anyway.

[jkrems]

@joyeecheung Good point! I think I'd still rather keep the reordering of the fields to protect against future refactoring. Seems generally good to have "trivial" fields before fields that may have more interesting logic in constructors. But I also added your change to prevent the unnecessary code on startup and in other situations where the async resource may be empty.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/52576/

[nodejs-github-bot]

Landed in 5ff46db