diff --git a/src/roma/config/config.h b/src/roma/config/config.h index 5d12cc918..77c93d41d 100644 --- a/src/roma/config/config.h +++ b/src/roma/config/config.h @@ -129,6 +129,11 @@ class Config { */ bool enable_profilers = false; + /** + * @brief Disable returning UDF stack trace in response. + */ + bool disable_udf_stacktraces_in_response = false; + /** * @brief Indicates whether a custom logging function has been registered. * diff --git a/src/roma/roma_service/BUILD.bazel b/src/roma/roma_service/BUILD.bazel index 220251b8a..9ec6f6a6b 100644 --- a/src/roma/roma_service/BUILD.bazel +++ b/src/roma/roma_service/BUILD.bazel @@ -194,6 +194,7 @@ cc_test( "//src/roma/native_function_grpc_server:test_request_handlers", "//src/roma/native_function_grpc_server/proto:test_service_roma_host_api", "//src/util:duration", + "@com_google_absl//absl/log:scoped_mock_log", "@com_google_absl//absl/strings", "@com_google_absl//absl/synchronization", "@com_google_absl//absl/time", diff --git a/src/roma/roma_service/roma_service.h b/src/roma/roma_service/roma_service.h index 218361db2..68dc706d9 100644 --- a/src/roma/roma_service/roma_service.h +++ b/src/roma/roma_service/roma_service.h @@ -306,7 +306,9 @@ class RomaService { config_.enable_sandbox_sharing_request_response_with_buffer_only, /*v8_flags=*/v8_flags, /*enable_profilers=*/config_.enable_profilers, - /*logging_function_set=*/config_.logging_function_set); + /*logging_function_set=*/config_.logging_function_set, + /*disable_udf_stacktraces_in_response*/ + config_.disable_udf_stacktraces_in_response); PS_RETURN_IF_ERROR(workers_.back().Init()); PS_RETURN_IF_ERROR(workers_.back().Run()); } diff --git a/src/roma/roma_service/sandboxed_service_test.cc b/src/roma/roma_service/sandboxed_service_test.cc index da6512072..abc963dc9 100644 --- a/src/roma/roma_service/sandboxed_service_test.cc +++ b/src/roma/roma_service/sandboxed_service_test.cc @@ -29,6 +29,8 @@ #include +#include "absl/log/scoped_mock_log.h" +#include "absl/strings/match.h" #include "absl/strings/str_cat.h" #include "absl/strings/str_join.h" #include "absl/synchronization/mutex.h" @@ -1936,6 +1938,136 @@ TEST(SandboxedServiceTest, ShouldGetExecutionErrorWhenJsCodeThrowError) { ASSERT_TRUE(roma_service.Stop().ok()); } +TEST(SandboxedServiceTest, StackTraceAvailableInResponse) { + Config config; + config.number_of_workers = 2; + RomaService<> roma_service(std::move(config)); + ASSERT_TRUE(roma_service.Init().ok()); + + absl::Notification load_finished; + absl::Notification execute_failed; + + { + auto code_obj = std::make_unique(CodeObject{ + .id = "foo", + .version_string = "v1", + .js = R"JS_CODE( + function Handler() { + throw new Error('foobar'); + } + )JS_CODE", + }); + + absl::Status response_status; + ASSERT_TRUE(roma_service + .LoadCodeObj(std::move(code_obj), + [&](absl::StatusOr resp) { + response_status = resp.status(); + load_finished.Notify(); + }) + .ok()); + ASSERT_TRUE( + load_finished.WaitForNotificationWithTimeout(absl::Seconds(10))); + ASSERT_TRUE(response_status.ok()); + } + + { + auto execution_obj = + std::make_unique>(InvocationStrRequest<>{ + .id = "foo", + .version_string = "v1", + .handler_name = "Handler", + }); + + absl::Status response_status; + ASSERT_TRUE(roma_service + .Execute(std::move(execution_obj), + [&](absl::StatusOr resp) { + response_status = resp.status(); + execute_failed.Notify(); + }) + .ok()); + ASSERT_TRUE( + execute_failed.WaitForNotificationWithTimeout(absl::Seconds(10))); + EXPECT_EQ(response_status.code(), absl::StatusCode::kInternal); + EXPECT_TRUE(absl::StrContains(response_status.message(), "at Handler")); + } + + ASSERT_TRUE(roma_service.Stop().ok()); +} + +void LoggingFunction(absl::LogSeverity severity, DefaultMetadata metadata, + std::string_view msg) { + LOG(LEVEL(severity)) << msg; +} + +TEST(SandboxedServiceTest, ShouldNotGetStackTraceWhenDisableStackTraceFlagSet) { + Config config; + config.number_of_workers = 2; + config.disable_udf_stacktraces_in_response = true; + config.SetLoggingFunction(LoggingFunction); + RomaService<> roma_service(std::move(config)); + ASSERT_TRUE(roma_service.Init().ok()); + + absl::Notification load_finished; + absl::Notification execute_failed; + + absl::ScopedMockLog log; + // disable_udf_stacktraces_in_response should not prevent stacktraces from + // being logged. + EXPECT_CALL(log, Log(absl::LogSeverity::kError, _, HasSubstr("at Handler"))); + log.StartCapturingLogs(); + + { + auto code_obj = std::make_unique(CodeObject{ + .id = "foo", + .version_string = "v1", + .js = R"JS_CODE( + function Handler() { + throw new Error('foobar'); + } + )JS_CODE", + }); + + absl::Status response_status; + ASSERT_TRUE(roma_service + .LoadCodeObj(std::move(code_obj), + [&](absl::StatusOr resp) { + response_status = resp.status(); + load_finished.Notify(); + }) + .ok()); + ASSERT_TRUE( + load_finished.WaitForNotificationWithTimeout(absl::Seconds(10))); + ASSERT_TRUE(response_status.ok()); + } + + { + auto execution_obj = + std::make_unique>(InvocationStrRequest<>{ + .id = "foo", + .version_string = "v1", + .handler_name = "Handler", + }); + + absl::Status response_status; + ASSERT_TRUE(roma_service + .Execute(std::move(execution_obj), + [&](absl::StatusOr resp) { + response_status = resp.status(); + execute_failed.Notify(); + }) + .ok()); + ASSERT_TRUE( + execute_failed.WaitForNotificationWithTimeout(absl::Seconds(10))); + EXPECT_EQ(response_status.code(), absl::StatusCode::kInternal); + EXPECT_FALSE(absl::StrContains(response_status.message(), "at Handler")); + } + + ASSERT_TRUE(roma_service.Stop().ok()); + log.StopCapturingLogs(); +} + TEST(SandboxedServiceTest, ShouldGetExecutionErrorWhenJsCodeReturnUndefined) { Config config; config.number_of_workers = 2; diff --git a/src/roma/sandbox/dispatcher/dispatcher_sapi_test.cc b/src/roma/sandbox/dispatcher/dispatcher_sapi_test.cc index f5011117c..188988c02 100644 --- a/src/roma/sandbox/dispatcher/dispatcher_sapi_test.cc +++ b/src/roma/sandbox/dispatcher/dispatcher_sapi_test.cc @@ -55,7 +55,8 @@ std::vector Workers(int num_workers) { /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/std::vector(), /*enable_profilers=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); CHECK_OK(workers.back().Init()); CHECK_OK(workers.back().Run()); } diff --git a/src/roma/sandbox/dispatcher/dispatcher_test.cc b/src/roma/sandbox/dispatcher/dispatcher_test.cc index b4f73607e..a3f89b210 100644 --- a/src/roma/sandbox/dispatcher/dispatcher_test.cc +++ b/src/roma/sandbox/dispatcher/dispatcher_test.cc @@ -56,7 +56,8 @@ std::vector Workers(int num_workers) { /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/std::vector(), /*enable_profilers=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); CHECK_OK(workers.back().Init()); CHECK_OK(workers.back().Run()); } diff --git a/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.cc b/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.cc index 2e0174b5d..d8e8aec11 100644 --- a/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.cc +++ b/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.cc @@ -210,13 +210,16 @@ V8JsEngine::V8JsEngine( std::unique_ptr isolate_function_binding, const bool skip_v8_cleanup, const bool enable_profilers, const JsEngineResourceConstraints& v8_resource_constraints, - const bool logging_function_set) + const bool logging_function_set, + const bool disable_udf_stacktraces_in_response) : isolate_function_binding_(std::move(isolate_function_binding)), v8_resource_constraints_(v8_resource_constraints), execution_watchdog_(std::make_unique()), skip_v8_cleanup_(skip_v8_cleanup), enable_profilers_(enable_profilers), - logging_function_set_(logging_function_set) { + logging_function_set_(logging_function_set), + disable_udf_stacktraces_in_response_( + disable_udf_stacktraces_in_response) { if (isolate_function_binding_) { isolate_function_binding_->AddExternalReferences(external_references_); } @@ -287,11 +290,15 @@ absl::Status V8JsEngine::FormatAndLogError(v8::Isolate* isolate, v8::Local context, std::string_view top_level_error, LogOptions log_options) { - std::string err_msg = - absl::StrCat(GetError(isolate, try_catch, top_level_error), "\n", - GetStackTrace(isolate, try_catch, context)); - HandleLog("ROMA_ERROR", err_msg, std::move(log_options)).IgnoreError(); - return absl::InternalError(std::move(err_msg)); + std::string err_msg = GetError(isolate, try_catch, top_level_error); + std::string stack_trace = GetStackTrace(isolate, try_catch, context); + std::string err_msg_stack_trace = absl::StrCat(err_msg, "\n", stack_trace); + HandleLog("ROMA_ERROR", err_msg_stack_trace, std::move(log_options)) + .IgnoreError(); + if (disable_udf_stacktraces_in_response_) { + return absl::InternalError(std::move(err_msg)); + } + return absl::InternalError(std::move(err_msg_stack_trace)); } absl::Status V8JsEngine::HandleLog(std::string_view function_name, diff --git a/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.h b/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.h index 4959cc14e..66a3f2f51 100644 --- a/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.h +++ b/src/roma/sandbox/js_engine/v8_engine/v8_js_engine.h @@ -49,7 +49,8 @@ class V8JsEngine : public JsEngine { bool skip_v8_cleanup = false, bool enable_profilers = false, const JsEngineResourceConstraints& v8_resource_constraints = JsEngineResourceConstraints(), - bool logging_function_set = false); + bool logging_function_set = false, + bool disable_udf_stacktraces_in_response = false); ~V8JsEngine() override; @@ -230,6 +231,7 @@ class V8JsEngine : public JsEngine { const bool skip_v8_cleanup_; const bool enable_profilers_; const bool logging_function_set_; + const bool disable_udf_stacktraces_in_response_; }; } // namespace google::scp::roma::sandbox::js_engine::v8_js_engine diff --git a/src/roma/sandbox/worker_api/sapi/utils.cc b/src/roma/sandbox/worker_api/sapi/utils.cc index abe639bf8..4f0c07d85 100644 --- a/src/roma/sandbox/worker_api/sapi/utils.cc +++ b/src/roma/sandbox/worker_api/sapi/utils.cc @@ -64,7 +64,7 @@ std::unique_ptr CreateWorker(const V8WorkerEngineParams& params) { auto v8_engine = std::make_unique( std::move(isolate_function_binding), params.skip_v8_cleanup, params.enable_profilers, params.resource_constraints, - params.logging_function_set); + params.logging_function_set, params.disable_udf_stacktraces_in_response); v8_engine->OneTimeSetup(GetEngineOneTimeSetup(params)); return std::make_unique(std::move(v8_engine), params.require_preload); } diff --git a/src/roma/sandbox/worker_api/sapi/utils.h b/src/roma/sandbox/worker_api/sapi/utils.h index 185064732..7527d8535 100644 --- a/src/roma/sandbox/worker_api/sapi/utils.h +++ b/src/roma/sandbox/worker_api/sapi/utils.h @@ -44,6 +44,7 @@ struct V8WorkerEngineParams { std::vector v8_flags; bool enable_profilers = false; bool logging_function_set = false; + bool disable_udf_stacktraces_in_response = false; }; absl::flat_hash_map GetEngineOneTimeSetup( diff --git a/src/roma/sandbox/worker_api/sapi/worker_init_params.proto b/src/roma/sandbox/worker_api/sapi/worker_init_params.proto index afcc97efa..6906ac048 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_init_params.proto +++ b/src/roma/sandbox/worker_api/sapi/worker_init_params.proto @@ -63,4 +63,7 @@ message WorkerInitParamsProto { // Set if a custom logging function was set on the Config passed into Roma. bool logging_function_set = 16; + + // Whether UDF stacktraces should be included in the response. + bool disable_udf_stacktraces_in_response = 17; } diff --git a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.cc b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.cc index fddf105b3..32bce8ab3 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.cc +++ b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.cc @@ -55,7 +55,7 @@ WorkerSandboxApi::WorkerSandboxApi( size_t sandbox_request_response_shared_buffer_size_mb, bool enable_sandbox_sharing_request_response_with_buffer_only, const std::vector& v8_flags, bool enable_profilers, - bool logging_function_set) + bool logging_function_set, bool disable_udf_stacktraces_in_response) : require_preload_(require_preload), native_js_function_comms_fd_(native_js_function_comms_fd), native_js_function_names_(native_js_function_names), @@ -70,7 +70,9 @@ WorkerSandboxApi::WorkerSandboxApi( enable_sandbox_sharing_request_response_with_buffer_only), v8_flags_(v8_flags), enable_profilers_(enable_profilers), - logging_function_set_(logging_function_set) { + logging_function_set_(logging_function_set), + disable_udf_stacktraces_in_response_( + disable_udf_stacktraces_in_response) { // create a sandbox2 buffer request_and_response_data_buffer_size_bytes_ = sandbox_request_response_shared_buffer_size_mb > 0 @@ -173,6 +175,8 @@ absl::Status WorkerSandboxApi::Init() { v8_flags_.end()); worker_init_params.set_enable_profilers(enable_profilers_); worker_init_params.set_logging_function_set(logging_function_set_); + worker_init_params.set_disable_udf_stacktraces_in_response( + disable_udf_stacktraces_in_response_); const auto worker_status = worker_wrapper_->Init(worker_init_params); if (!worker_status.ok()) { diff --git a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.h b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.h index 98b5ea92b..40c0d6b15 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.h +++ b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api.h @@ -66,6 +66,10 @@ class WorkerSandboxApi { * @param v8_flags List of flags to pass into v8. (Ex. {"--FLAG_1", * "--FLAG_2"}) * @param enable_profilers Enable the V8 CPU and Heap Profilers + * @param logging_function_set Whether a logging function has been registered + * in Roma + * @param disable_udf_stacktraces_in_response Whether UDF stacktrace should be + * returned in response */ WorkerSandboxApi( bool require_preload, int native_js_function_comms_fd, @@ -78,7 +82,7 @@ class WorkerSandboxApi { size_t sandbox_request_response_shared_buffer_size_mb, bool enable_sandbox_sharing_request_response_with_buffer_only, const std::vector& v8_flags, bool enable_profilers, - bool logging_function_set); + bool logging_function_set, bool disable_udf_stacktraces_in_response); absl::Status Init(); @@ -136,6 +140,7 @@ class WorkerSandboxApi { std::vector v8_flags_; const bool enable_profilers_; const bool logging_function_set_; + const bool disable_udf_stacktraces_in_response_; }; } // namespace google::scp::roma::sandbox::worker_api diff --git a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_sapi_test.cc b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_sapi_test.cc index 2465818d0..22d775b15 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_sapi_test.cc +++ b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_sapi_test.cc @@ -56,7 +56,8 @@ TEST(WorkerSandboxApiSapiTest, /*sandbox_request_response_shared_buffer_size_mb=*/0, /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/{}, /*enable_profilers=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); // Initializing the sandbox fail as we're giving a max of 100MB of virtual // space address for v8 and the sandbox. @@ -81,7 +82,8 @@ class WorkerSandboxApiForTests : public WorkerSandboxApi { /*sandbox_request_response_shared_buffer_size_mb=*/0, /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/{}, /*enable_profilers=*/false, - /*logging_function_set=*/false) {} + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false) {} ::sapi::Sandbox* GetUnderlyingSandbox() { return worker_sapi_sandbox_.get(); } }; diff --git a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_test.cc b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_test.cc index 2a9e1dcf0..aabf1206f 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_test.cc +++ b/src/roma/sandbox/worker_api/sapi/worker_sandbox_api_test.cc @@ -54,7 +54,8 @@ TEST(WorkerSandboxApiTest, WorkerWorksThroughSandbox) { /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/{}, /*enable_profilers=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); ASSERT_TRUE(sandbox_api.Init().ok()); @@ -89,7 +90,8 @@ TEST(WorkerSandboxApiTest, WorkerReturnsInformativeThrowMessageThroughSandbox) { /*sandbox_request_response_shared_buffer_size_mb=*/0, /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/{}, /*enable_profilers=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); ASSERT_TRUE(sandbox_api.Init().ok()); @@ -123,7 +125,8 @@ TEST(WorkerSandboxApiTest, WorkerReturnsInformativeMessageForMissingParam) { /*sandbox_request_response_shared_buffer_size_mb=*/0, /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/{}, /*enable_cpu_profiler=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); ASSERT_TRUE(sandbox_api.Init().ok()); @@ -159,7 +162,8 @@ TEST(WorkerSandboxApiTest, WorkerCanCallHooksThroughSandbox) { /*sandbox_request_response_shared_buffer_size_mb=*/0, /*enable_sandbox_sharing_request_response_with_buffer_only=*/false, /*v8_flags=*/{}, /*enable_profilers=*/false, - /*logging_function_set=*/false); + /*logging_function_set=*/false, + /*disable_udf_stacktraces_in_response=*/false); ASSERT_TRUE(sandbox_api.Init().ok()); diff --git a/src/roma/sandbox/worker_api/sapi/worker_wrapper_impl.cc b/src/roma/sandbox/worker_api/sapi/worker_wrapper_impl.cc index 0f2a6e6d5..91c585669 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_wrapper_impl.cc +++ b/src/roma/sandbox/worker_api/sapi/worker_wrapper_impl.cc @@ -110,6 +110,8 @@ SapiStatusCode Init(worker_api::WorkerInitParamsProto* init_params) { .v8_flags = std::move(v8_flags), .enable_profilers = init_params->enable_profilers(), .logging_function_set = init_params->logging_function_set(), + .disable_udf_stacktraces_in_response = + init_params->disable_udf_stacktraces_in_response(), }; worker_ = CreateWorker(v8_params); diff --git a/src/roma/sandbox/worker_api/sapi/worker_wrapper_nonprod.cc b/src/roma/sandbox/worker_api/sapi/worker_wrapper_nonprod.cc index 37685bfb8..bd1d3a0d2 100644 --- a/src/roma/sandbox/worker_api/sapi/worker_wrapper_nonprod.cc +++ b/src/roma/sandbox/worker_api/sapi/worker_wrapper_nonprod.cc @@ -94,6 +94,8 @@ absl::Status WorkerWrapper::Init( .v8_flags = std::move(v8_flags), .enable_profilers = init_params.enable_profilers(), .logging_function_set = init_params.logging_function_set(), + .disable_udf_stacktraces_in_response = + init_params.disable_udf_stacktraces_in_response(), }; worker_ = CreateWorker(v8_params);