Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: Initialize fields accessed by AsyncHooks before AsyncHooks #48566

Merged
merged 1 commit into from
Jul 3, 2023

Conversation

jkrems
Copy link
Contributor

@jkrems jkrems commented Jun 26, 2023

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.

@nodejs-github-bot 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
jkrems added a commit to jkrems/node that referenced this pull request Jun 26, 2023
jkrems added a commit to jkrems/node that referenced this pull request Jun 26, 2023
src/env.h Show resolved Hide resolved
jkrems added a commit to jkrems/node that referenced this pull request Jun 28, 2023
@jkrems jkrems added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 28, 2023
@nodejs-github-bot

This comment was marked as outdated.

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

jkrems commented Jun 30, 2023

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

@jkrems jkrems added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 30, 2023
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@Flarna Flarna added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 3, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 5ff46db into nodejs:main Jul 3, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5ff46db

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: #48566
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
@jkrems jkrems deleted the asan-atomics branch July 3, 2023 14:59
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#48566
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: nodejs#48566
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 11, 2023
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: #48566
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Sep 11, 2023
ruyadorno pushed a commit that referenced this pull request Sep 13, 2023
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: #48566
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
ruyadorno pushed a commit that referenced this pull request Sep 17, 2023
Co-authored-by: Joyee Cheung <[email protected]>
PR-URL: #48566
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants