src: Initialize fields accessed by AsyncHooks before AsyncHooks by hybrist · Pull Request #48566 · nodejs/node · GitHub

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.

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.

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: nodejs#48566

@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.

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

Landed in 5ff46db

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: #48566 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: nodejs#48566 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: #48566 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com> Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Jun 26, 2023

hybrist force-pushed the asan-atomics branch from 6c2337f to f1b3c6e Compare June 26, 2023 22:13

hybrist mentioned this pull request Jun 26, 2023

src: avoid using v8 on Isolate termination #44669

Merged

hybrist force-pushed the asan-atomics branch from f1b3c6e to f6f35d7 Compare June 26, 2023 22:17

hybrist force-pushed the asan-atomics branch from f6f35d7 to c291e66 Compare June 26, 2023 22:18

hybrist force-pushed the asan-atomics branch from c291e66 to 4416546 Compare June 26, 2023 22:22

bnoordhuis approved these changes Jun 27, 2023

View reviewed changes

Comment thread src/env.h Outdated

hybrist force-pushed the asan-atomics branch from 4416546 to 29d9d7c Compare June 27, 2023 14:10

bnoordhuis approved these changes Jun 27, 2023

View reviewed changes

hybrist force-pushed the asan-atomics branch from 29d9d7c to 676b2f0 Compare June 28, 2023 16:58

hybrist added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2023

This comment was marked as outdated.

Sign in to view

santigimeno approved these changes Jun 28, 2023

View reviewed changes

lpinca approved these changes Jun 28, 2023

View reviewed changes

joyeecheung approved these changes Jun 28, 2023

View reviewed changes

This was referenced Jun 29, 2023

CI Reliability 2023-06-29 nodejs/reliability#602

Open

CI Reliability 2023-06-30 nodejs/reliability#603

Open

hybrist force-pushed the asan-atomics branch from 676b2f0 to 0481b27 Compare June 30, 2023 21:09

src: fix uninitialized field access in AsyncHooks

fcc8595

Co-authored-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: nodejs#48566

hybrist force-pushed the asan-atomics branch from 0481b27 to fcc8595 Compare June 30, 2023 21:10

hybrist added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Jul 1, 2023

CI Reliability 2023-07-01 nodejs/reliability#604

Open

30 tasks

This was referenced Jul 2, 2023

CI Reliability 2023-07-02 nodejs/reliability#605

Open

CI Reliability 2023-07-03 nodejs/reliability#606

Open

JungMinu approved these changes Jul 3, 2023

View reviewed changes

Flarna approved these changes Jul 3, 2023

View reviewed changes

Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 3, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 3, 2023

nodejs-github-bot merged commit 5ff46db into nodejs:main Jul 3, 2023

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

hybrist deleted the asan-atomics branch July 3, 2023 14:59

This was referenced Jul 4, 2023

CI Reliability 2023-07-04 nodejs/reliability#607

Open

CI Reliability 2023-07-05 nodejs/reliability#608

Open

hybrist mentioned this pull request Jul 5, 2023

Signed integer overflow in latest node 18 LTS release #48621

Closed

This was referenced Jul 6, 2023

CI Reliability 2023-07-06 nodejs/reliability#609

Open

CI Reliability 2023-07-07 nodejs/reliability#610

Open

CI Reliability 2023-07-08 nodejs/reliability#611

Open

CI Reliability 2023-07-09 nodejs/reliability#612

Open

ruyadorno mentioned this pull request Sep 11, 2023

v18.18.0 proposal #49220

Merged

src: Initialize fields accessed by AsyncHooks before AsyncHooks by hybrist · Pull Request #48566 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: Initialize fields accessed by AsyncHooks before AsyncHooks - #48566

src: Initialize fields accessed by AsyncHooks before AsyncHooks#48566
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
hybrist:asan-atomics

hybrist commented Jun 26, 2023 •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

joyeecheung left a comment

Uh oh!

hybrist commented Jun 30, 2023 •
edited
Loading

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jul 1, 2023

Uh oh!

nodejs-github-bot commented Jul 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: Initialize fields accessed by AsyncHooks before AsyncHooks by hybrist · Pull Request #48566 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: Initialize fields accessed by AsyncHooks before AsyncHooks - #48566

src: Initialize fields accessed by AsyncHooks before AsyncHooks#48566nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom hybrist:asan-atomicshybrist/node:asan-atomicsCopy head branch name to clipboard

Conversation

hybrist commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This comment was marked as outdated.

joyeecheung left a comment

Choose a reason for hiding this comment

Uh oh!

hybrist commented Jun 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Jul 1, 2023

Uh oh!

nodejs-github-bot commented Jul 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

src: Initialize fields accessed by AsyncHooks before AsyncHooks#48566
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
hybrist:asan-atomics

hybrist commented Jun 26, 2023 •
edited
Loading

hybrist commented Jun 30, 2023 •
edited
Loading