From e80613c70d6c1efc6c6bbef85c95569d13ffdef4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 22 Jun 2022 15:24:34 +0800 Subject: [PATCH] bootstrap: handle snapshot errors gracefully This patch refactors the SnapshotBuilder::Generate() routines so that when running into errors during the snapshot building process, they can exit gracefully by printing the error and return a non-zero exit code. If the error is likely to be caused by internal scripts, the return code would be 12, if the error is caused by user scripts the return code would be 1. In addition this refactors the generation of embedded snapshots and directly writes to the output file stream instead of producing an intermediate string with string streams. --- src/env.h | 21 +- src/node_snapshot_builder.h | 11 +- src/node_snapshotable.cc | 332 ++++++++++++++++-------------- test/fixtures/snapshot/error.js | 1 + tools/snapshot/node_mksnapshot.cc | 17 +- 5 files changed, 209 insertions(+), 173 deletions(-) create mode 100644 test/fixtures/snapshot/error.js diff --git a/src/env.h b/src/env.h index 921b6758ac5b6c..9a1a0f722e6e14 100644 --- a/src/env.h +++ b/src/env.h @@ -989,13 +989,17 @@ struct EnvSerializeInfo { }; struct SnapshotData { - // The result of v8::SnapshotCreator::CreateBlob() during the snapshot - // building process. - v8::StartupData v8_snapshot_blob_data; + enum class DataOwnership { kOwned, kNotOwned }; static const size_t kNodeBaseContextIndex = 0; static const size_t kNodeMainContextIndex = kNodeBaseContextIndex + 1; + DataOwnership data_ownership; + + // The result of v8::SnapshotCreator::CreateBlob() during the snapshot + // building process. + v8::StartupData v8_snapshot_blob_data; + std::vector isolate_data_indices; // TODO(joyeecheung): there should be a vector of env_info once we snapshot // the worker environments. @@ -1005,6 +1009,17 @@ struct SnapshotData { // read only space. We use native_module::CodeCacheInfo because // v8::ScriptCompiler::CachedData is not copyable. std::vector code_cache; + + static std::unique_ptr New(); + ~SnapshotData(); + + SnapshotData(const SnapshotData&) = delete; + SnapshotData& operator=(const SnapshotData&) = delete; + SnapshotData(SnapshotData&&) = delete; + SnapshotData& operator=(SnapshotData&&) = delete; + + // Only invoked by New(). + SnapshotData() = default; }; class Environment : public MemoryRetainer { diff --git a/src/node_snapshot_builder.h b/src/node_snapshot_builder.h index c5d2ee2a4bcd83..7a82c00255e780 100644 --- a/src/node_snapshot_builder.h +++ b/src/node_snapshot_builder.h @@ -15,13 +15,14 @@ struct SnapshotData; class NODE_EXTERN_PRIVATE SnapshotBuilder { public: - static std::string Generate(const std::vector args, - const std::vector exec_args); + static int Generate(std::ostream& out, + const std::vector args, + const std::vector exec_args); // Generate the snapshot into out. - static void Generate(SnapshotData* out, - const std::vector args, - const std::vector exec_args); + static int Generate(SnapshotData* out, + const std::vector args, + const std::vector exec_args); // If nullptr is returned, the binary is not built with embedded // snapshot. diff --git a/src/node_snapshotable.cc b/src/node_snapshotable.cc index b71c46acabed72..d3e5aa2e02e34f 100644 --- a/src/node_snapshotable.cc +++ b/src/node_snapshotable.cc @@ -29,7 +29,6 @@ using v8::FunctionCallbackInfo; using v8::HandleScope; using v8::Isolate; using v8::Local; -using v8::MaybeLocal; using v8::Object; using v8::ScriptCompiler; using v8::ScriptOrigin; @@ -39,8 +38,22 @@ using v8::String; using v8::TryCatch; using v8::Value; +std::unique_ptr SnapshotData::New() { + std::unique_ptr result = std::make_unique(); + result->data_ownership = DataOwnership::kOwned; + result->v8_snapshot_blob_data.data = nullptr; + return result; +} + +SnapshotData::~SnapshotData() { + if (data_ownership == DataOwnership::kOwned && + v8_snapshot_blob_data.data != nullptr) { + delete[] v8_snapshot_blob_data.data; + } +} + template -void WriteVector(std::ostringstream* ss, const T* vec, size_t size) { +void WriteVector(std::ostream* ss, const T* vec, size_t size) { for (size_t i = 0; i < size; i++) { *ss << std::to_string(vec[i]) << (i == size - 1 ? '\n' : ','); } @@ -70,15 +83,14 @@ static std::string FormatSize(size_t size) { return buf; } -static void WriteStaticCodeCacheData(std::ostringstream* ss, +static void WriteStaticCodeCacheData(std::ostream* ss, const native_module::CodeCacheInfo& info) { *ss << "static const uint8_t " << GetCodeCacheDefName(info.id) << "[] = {\n"; WriteVector(ss, info.data.data(), info.data.size()); *ss << "};"; } -static void WriteCodeCacheInitializer(std::ostringstream* ss, - const std::string& id) { +static void WriteCodeCacheInitializer(std::ostream* ss, const std::string& id) { std::string def_name = GetCodeCacheDefName(id); *ss << " { \"" << id << "\",\n"; *ss << " {" << def_name << ",\n"; @@ -87,9 +99,7 @@ static void WriteCodeCacheInitializer(std::ostringstream* ss, *ss << " },\n"; } -std::string FormatBlob(SnapshotData* data) { - std::ostringstream ss; - +void FormatBlob(std::ostream& ss, SnapshotData* data) { ss << R"(#include #include "env.h" #include "node_snapshot_builder.h" @@ -116,6 +126,9 @@ static const int v8_snapshot_blob_size = )" } ss << R"(SnapshotData snapshot_data { + // -- data_ownership begins -- + SnapshotData::DataOwnership::kNotOwned, + // -- data_ownership ends -- // -- v8_snapshot_blob_data begins -- { v8_snapshot_blob_data, v8_snapshot_blob_size }, // -- v8_snapshot_blob_data ends -- @@ -148,8 +161,6 @@ const SnapshotData* SnapshotBuilder::GetEmbeddedSnapshotData() { } } // namespace node )"; - - return ss.str(); } Mutex SnapshotBuilder::snapshot_data_mutex_; @@ -166,175 +177,182 @@ void SnapshotBuilder::InitializeIsolateParams(const SnapshotData* data, const_cast(&(data->v8_snapshot_blob_data)); } -void SnapshotBuilder::Generate(SnapshotData* out, - const std::vector args, - const std::vector exec_args) { +constexpr int INTERNAL_ERROR = 12; + +int SnapshotBuilder::Generate(SnapshotData* out, + const std::vector args, + const std::vector exec_args) { + const std::vector& external_references = + CollectExternalReferences(); Isolate* isolate = Isolate::Allocate(); - isolate->SetCaptureStackTraceForUncaughtExceptions( - true, 10, v8::StackTrace::StackTraceOptions::kDetailed); + // Must be done before the SnapshotCreator creation so that the + // memory reducer can be initialized. per_process::v8_platform.Platform()->RegisterIsolate(isolate, uv_default_loop()); - std::unique_ptr main_instance; - std::string result; + + SnapshotCreator creator(isolate, external_references.data()); + + isolate->SetCaptureStackTraceForUncaughtExceptions( + true, 10, v8::StackTrace::StackTraceOptions::kDetailed); + + Environment* env = nullptr; + std::unique_ptr main_instance = + NodeMainInstance::Create(isolate, + uv_default_loop(), + per_process::v8_platform.Platform(), + args, + exec_args); + + // The cleanups should be done in case of an early exit due to errors. + auto cleanup = OnScopeLeave([&]() { + // Must be done while the snapshot creator isolate is entered i.e. the + // creator is still alive. The snapshot creator destructor will destroy + // the isolate. + if (env != nullptr) { + FreeEnvironment(env); + } + main_instance->Dispose(); + per_process::v8_platform.Platform()->UnregisterIsolate(isolate); + }); { - const std::vector& external_references = - CollectExternalReferences(); - SnapshotCreator creator(isolate, external_references.data()); - Environment* env; - { - main_instance = - NodeMainInstance::Create(isolate, - uv_default_loop(), - per_process::v8_platform.Platform(), - args, - exec_args); - out->isolate_data_indices = - main_instance->isolate_data()->Serialize(&creator); - - HandleScope scope(isolate); - - // The default context with only things created by V8. - creator.SetDefaultContext(Context::New(isolate)); - - auto CreateBaseContext = [&]() { - TryCatch bootstrapCatch(isolate); - // Run the per-context scripts. - Local base_context = NewContext(isolate); - if (bootstrapCatch.HasCaught()) { - PrintCaughtException(isolate, base_context, bootstrapCatch); - abort(); - } - return base_context; - }; - - // The Node.js-specific context with primodials, can be used by workers - // TODO(joyeecheung): investigate if this can be used by vm contexts - // without breaking compatibility. - { - size_t index = creator.AddContext(CreateBaseContext()); - CHECK_EQ(index, SnapshotData::kNodeBaseContextIndex); + HandleScope scope(isolate); + TryCatch bootstrapCatch(isolate); + + auto print_Exception = OnScopeLeave([&]() { + if (bootstrapCatch.HasCaught()) { + PrintCaughtException( + isolate, isolate->GetCurrentContext(), bootstrapCatch); } + }); - // The main instance context. - { - Local main_context = CreateBaseContext(); - Context::Scope context_scope(main_context); - TryCatch bootstrapCatch(isolate); - - // Create the environment. - env = new Environment(main_instance->isolate_data(), - main_context, - args, - exec_args, - nullptr, - node::EnvironmentFlags::kDefaultFlags, - {}); - - // Run scripts in lib/internal/bootstrap/ - MaybeLocal result = env->RunBootstrapping(); - if (bootstrapCatch.HasCaught()) { - // TODO(joyeecheung): fail by exiting with a non-zero exit code. - PrintCaughtException(isolate, main_context, bootstrapCatch); - abort(); - } - result.ToLocalChecked(); - // If --build-snapshot is true, lib/internal/main/mksnapshot.js would be - // loaded via LoadEnvironment() to execute process.argv[1] as the entry - // point (we currently only support this kind of entry point, but we - // could also explore snapshotting other kinds of execution modes - // in the future). - if (per_process::cli_options->build_snapshot) { + out->isolate_data_indices = + main_instance->isolate_data()->Serialize(&creator); + + // The default context with only things created by V8. + Local default_context = Context::New(isolate); + + // The Node.js-specific context with primodials, can be used by workers + // TODO(joyeecheung): investigate if this can be used by vm contexts + // without breaking compatibility. + Local base_context = NewContext(isolate); + if (base_context.IsEmpty()) { + return INTERNAL_ERROR; + } + + Local main_context = NewContext(isolate); + if (main_context.IsEmpty()) { + return INTERNAL_ERROR; + } + // Initialize the main instance context. + { + Context::Scope context_scope(main_context); + + // Create the environment. + env = new Environment(main_instance->isolate_data(), + main_context, + args, + exec_args, + nullptr, + node::EnvironmentFlags::kDefaultFlags, + {}); + + // Run scripts in lib/internal/bootstrap/ + if (env->RunBootstrapping().IsEmpty()) { + return INTERNAL_ERROR; + } + // If --build-snapshot is true, lib/internal/main/mksnapshot.js would be + // loaded via LoadEnvironment() to execute process.argv[1] as the entry + // point (we currently only support this kind of entry point, but we + // could also explore snapshotting other kinds of execution modes + // in the future). + if (per_process::cli_options->build_snapshot) { #if HAVE_INSPECTOR - env->InitializeInspector({}); + // TODO(joyeecheung): move this before RunBootstrapping(). + env->InitializeInspector({}); #endif - // TODO(joyeecheung): we could use the result for something special, - // like setting up initializers that should be invoked at snapshot - // dehydration. - MaybeLocal result = - LoadEnvironment(env, StartExecutionCallback{}); - if (bootstrapCatch.HasCaught()) { - // TODO(joyeecheung): fail by exiting with a non-zero exit code. - PrintCaughtException(isolate, main_context, bootstrapCatch); - abort(); - } - result.ToLocalChecked(); - // FIXME(joyeecheung): right now running the loop in the snapshot - // builder seems to introduces inconsistencies in JS land that need to - // be synchronized again after snapshot restoration. - int exit_code = SpinEventLoop(env).FromMaybe(1); - CHECK_EQ(exit_code, 0); - if (bootstrapCatch.HasCaught()) { - // TODO(joyeecheung): fail by exiting with a non-zero exit code. - PrintCaughtException(isolate, main_context, bootstrapCatch); - abort(); - } + if (LoadEnvironment(env, StartExecutionCallback{}).IsEmpty()) { + return 1; } - - if (per_process::enabled_debug_list.enabled( - DebugCategory::MKSNAPSHOT)) { - env->PrintAllBaseObjects(); - printf("Environment = %p\n", env); + // FIXME(joyeecheung): right now running the loop in the snapshot + // builder seems to introduces inconsistencies in JS land that need to + // be synchronized again after snapshot restoration. + int exit_code = SpinEventLoop(env).FromMaybe(1); + if (exit_code != 0) { + return exit_code; } + } - // Serialize the native states - out->env_info = env->Serialize(&creator); - // Serialize the context - size_t index = creator.AddContext( - main_context, {SerializeNodeContextInternalFields, env}); - CHECK_EQ(index, SnapshotData::kNodeMainContextIndex); + if (per_process::enabled_debug_list.enabled(DebugCategory::MKSNAPSHOT)) { + env->PrintAllBaseObjects(); + printf("Environment = %p\n", env); + } + + // Serialize the native states + out->env_info = env->Serialize(&creator); #ifdef NODE_USE_NODE_CODE_CACHE - // Regenerate all the code cache. - CHECK(native_module::NativeModuleEnv::CompileAllModules(main_context)); - native_module::NativeModuleEnv::CopyCodeCache(&(out->code_cache)); - for (const auto& item : out->code_cache) { - std::string size_str = FormatSize(item.data.size()); - per_process::Debug(DebugCategory::MKSNAPSHOT, - "Generated code cache for %d: %s\n", - item.id.c_str(), - size_str.c_str()); - } -#endif + // Regenerate all the code cache. + if (!native_module::NativeModuleEnv::CompileAllModules(main_context)) { + return INTERNAL_ERROR; + } + native_module::NativeModuleEnv::CopyCodeCache(&(out->code_cache)); + for (const auto& item : out->code_cache) { + std::string size_str = FormatSize(item.data.size()); + per_process::Debug(DebugCategory::MKSNAPSHOT, + "Generated code cache for %d: %s\n", + item.id.c_str(), + size_str.c_str()); } +#endif } - // Must be out of HandleScope - out->v8_snapshot_blob_data = - creator.CreateBlob(SnapshotCreator::FunctionCodeHandling::kClear); - - // We must be able to rehash the blob when we restore it or otherwise - // the hash seed would be fixed by V8, introducing a vulnerability. - CHECK(out->v8_snapshot_blob_data.CanBeRehashed()); - - // We cannot resurrect the handles from the snapshot, so make sure that - // no handles are left open in the environment after the blob is created - // (which should trigger a GC and close all handles that can be closed). - if (!env->req_wrap_queue()->IsEmpty() - || !env->handle_wrap_queue()->IsEmpty() - || per_process::enabled_debug_list.enabled(DebugCategory::MKSNAPSHOT)) { - PrintLibuvHandleInformation(env->event_loop(), stderr); - } - CHECK(env->req_wrap_queue()->IsEmpty()); - CHECK(env->handle_wrap_queue()->IsEmpty()); + // Global handles to the contexts can't be disposed before the + // blob is created. So initialize all the contexts before adding them. + // TODO(joyeecheung): figure out how to remove this restriction. + creator.SetDefaultContext(default_context); + size_t index = creator.AddContext(base_context); + CHECK_EQ(index, SnapshotData::kNodeBaseContextIndex); + index = creator.AddContext(main_context, + {SerializeNodeContextInternalFields, env}); + CHECK_EQ(index, SnapshotData::kNodeMainContextIndex); + } - // Must be done while the snapshot creator isolate is entered i.e. the - // creator is still alive. - FreeEnvironment(env); - main_instance->Dispose(); + // Must be out of HandleScope + out->v8_snapshot_blob_data = + creator.CreateBlob(SnapshotCreator::FunctionCodeHandling::kClear); + + // We must be able to rehash the blob when we restore it or otherwise + // the hash seed would be fixed by V8, introducing a vulnerability. + if (!out->v8_snapshot_blob_data.CanBeRehashed()) { + return INTERNAL_ERROR; } - per_process::v8_platform.Platform()->UnregisterIsolate(isolate); + // We cannot resurrect the handles from the snapshot, so make sure that + // no handles are left open in the environment after the blob is created + // (which should trigger a GC and close all handles that can be closed). + bool queues_are_empty = + env->req_wrap_queue()->IsEmpty() && env->handle_wrap_queue()->IsEmpty(); + if (!queues_are_empty || + per_process::enabled_debug_list.enabled(DebugCategory::MKSNAPSHOT)) { + PrintLibuvHandleInformation(env->event_loop(), stderr); + } + if (!queues_are_empty) { + return INTERNAL_ERROR; + } + return 0; } -std::string SnapshotBuilder::Generate( - const std::vector args, - const std::vector exec_args) { - SnapshotData data; - Generate(&data, args, exec_args); - std::string result = FormatBlob(&data); - delete[] data.v8_snapshot_blob_data.data; - return result; +int SnapshotBuilder::Generate(std::ostream& out, + const std::vector args, + const std::vector exec_args) { + std::unique_ptr data = SnapshotData::New(); + int exit_code = Generate(data.get(), args, exec_args); + if (exit_code != 0) { + return exit_code; + } + FormatBlob(out, data.get()); + return exit_code; } SnapshotableObject::SnapshotableObject(Environment* env, diff --git a/test/fixtures/snapshot/error.js b/test/fixtures/snapshot/error.js new file mode 100644 index 00000000000000..478d9ec609c4a8 --- /dev/null +++ b/test/fixtures/snapshot/error.js @@ -0,0 +1 @@ +throw new Error('test'); \ No newline at end of file diff --git a/tools/snapshot/node_mksnapshot.cc b/tools/snapshot/node_mksnapshot.cc index d166559a715b14..15f96070a7e3a9 100644 --- a/tools/snapshot/node_mksnapshot.cc +++ b/tools/snapshot/node_mksnapshot.cc @@ -83,17 +83,18 @@ int BuildSnapshot(int argc, char* argv[]) { return 1; } + int exit_code = 0; { - std::string snapshot = - node::SnapshotBuilder::Generate(result.args, result.exec_args); - out << snapshot; - - if (!out) { - std::cerr << "Failed to write " << out_path << "\n"; - return 1; + exit_code = + node::SnapshotBuilder::Generate(out, result.args, result.exec_args); + if (exit_code == 0) { + if (!out) { + std::cerr << "Failed to write " << out_path << "\n"; + exit_code = 1; + } } } node::TearDownOncePerProcess(); - return 0; + return exit_code; }