Skip to content

Commit

Permalink
fix: Fix segmentation fault in tests on CPUs with PKU support (#471)
Browse files Browse the repository at this point in the history
Due to V8's PKU feature, only the thread that initialized the V8
platform or those spawned from it can access the isolates; other threads
will get a segmentation fault. Since each test runs in its thread, they
crash on newer CPUs that support PKU.

This issue has two possible solutions:
1. Initialize the platform once in the main thread.
2. Turn off the PKU feature during testing.

This issue was already encountered for the `deno test` command, which
was resolved by initializing the platform in the main thread
(denoland/deno#20495).

In the case of rust tests, the same solution cannot be used, as the
default test runner does not allow running code on the main thread
before any test runs.

My proposed solution is to add a feature flag to deno_core, which, when
enabled, uses the unprotected platform. An alternative solution could be
to call `deno_core::JsRuntime::init_platform` at the beginning of each
test.

This PR will also be a step towards fixing denoland/deno#21439
  • Loading branch information
Digifox03 authored Jan 24, 2024
1 parent 150725a commit ec8cbe2
Show file tree
Hide file tree
Showing 14 changed files with 43 additions and 14 deletions.
8 changes: 8 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ include_icu_data = []
v8_use_custom_libcxx = ["v8/use_custom_libcxx"]
include_js_files_for_snapshotting = []
unsafe_runtime_options = []
unsafe_use_unprotected_platform = []

# Use the old, slower (but better tested) JoinSet driver
op_driver_joinset = []
Expand Down
11 changes: 9 additions & 2 deletions core/runtime/jsruntime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,8 +413,15 @@ fn v8_init(
};
v8::V8::set_flags_from_string(&flags);

let v8_platform = v8_platform
.unwrap_or_else(|| v8::new_default_platform(0, false).make_shared());
let v8_platform = v8_platform.unwrap_or_else(|| {
if cfg!(any(test, feature = "unsafe_use_unprotected_platform")) {
// We want to use the unprotected platform for unit tests
v8::new_unprotected_default_platform(0, false)
} else {
v8::new_default_platform(0, false)
}
.make_shared()
});
v8::V8::initialize_platform(v8_platform.clone());
v8::V8::initialize();

Expand Down
1 change: 1 addition & 0 deletions serde_v8/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ thiserror.workspace = true
v8.workspace = true

[dev-dependencies]
serde_v8_utilities = { path = "./utilities" }
bencher.workspace = true
serde_json.workspace = true
serde_bytes.workspace = true
Expand Down
4 changes: 2 additions & 2 deletions serde_v8/benches/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ use bencher::Bencher;

use serde::Deserialize;

use serde_v8::utils::js_exec;
use serde_v8::utils::v8_do;
use serde_v8::ByteString;
use serde_v8_utilities::js_exec;
use serde_v8_utilities::v8_do;

#[derive(Debug, Deserialize, PartialEq)]
struct MathOp {
Expand Down
2 changes: 1 addition & 1 deletion serde_v8/benches/ser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ use bencher::Bencher;

use serde::Serialize;

use serde_v8::utils::v8_do;
use serde_v8::ByteString;
use serde_v8_utilities::v8_do;

#[derive(Serialize)]
struct MathOp {
Expand Down
1 change: 0 additions & 1 deletion serde_v8/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ mod magic;
mod payload;
mod ser;
mod serializable;
pub mod utils;

pub use de::from_v8;
pub use de::from_v8_cached;
Expand Down
3 changes: 2 additions & 1 deletion serde_v8/magic/v8slice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ where
static V8_ONCE: std::sync::Once = std::sync::Once::new();

V8_ONCE.call_once(|| {
let platform = v8::new_default_platform(0, false).make_shared();
let platform =
v8::new_unprotected_default_platform(0, false).make_shared();
v8::V8::initialize_platform(platform);
v8::V8::initialize();
});
Expand Down
4 changes: 2 additions & 2 deletions serde_v8/tests/de.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,13 @@
use serde::Deserialize;
use serde::Deserializer;

use serde_v8::utils::js_exec;
use serde_v8::utils::v8_do;
use serde_v8::BigInt;
use serde_v8::ByteString;
use serde_v8::Error;
use serde_v8::JsBuffer;
use serde_v8::U16String;
use serde_v8_utilities::js_exec;
use serde_v8_utilities::v8_do;

#[derive(Debug, Deserialize, PartialEq)]
struct MathOp {
Expand Down
4 changes: 2 additions & 2 deletions serde_v8/tests/magic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@
use serde::Deserialize;
use serde::Serialize;

use serde_v8::utils::js_exec;
use serde_v8::utils::v8_do;
use serde_v8::Result;
use serde_v8_utilities::js_exec;
use serde_v8_utilities::v8_do;

#[derive(Deserialize)]
struct MagicOp<'s> {
Expand Down
4 changes: 2 additions & 2 deletions serde_v8/tests/ser.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
// Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.
use serde::Serialize;
use serde_json::json;
use serde_v8::utils::js_exec;
use serde_v8::utils::v8_do;
use serde_v8::BigInt;
use serde_v8_utilities::js_exec;
use serde_v8_utilities::v8_do;

#[derive(Debug, Serialize, PartialEq)]
struct MathOp {
Expand Down
11 changes: 11 additions & 0 deletions serde_v8/utilities/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# Copyright 2018-2023 the Deno authors. All rights reserved. MIT license.

[package]
name = "serde_v8_utilities"
version = "0.0.0"

[lib]
path = "lib.rs"

[dependencies]
v8.workspace = true
2 changes: 1 addition & 1 deletion serde_v8/utils.rs → serde_v8/utilities/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub fn js_exec<'s>(
}

pub fn v8_init() {
let platform = v8::new_default_platform(0, false).make_shared();
let platform = v8::new_unprotected_default_platform(0, false).make_shared();
v8::V8::initialize_platform(platform);
v8::V8::initialize();
}
Expand Down
1 change: 1 addition & 0 deletions testing/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ path = "./lib.rs"

[dev-dependencies]
deno_core.workspace = true
deno_core.features = ["unsafe_use_unprotected_platform"]
pretty_assertions.workspace = true
prettyplease.workspace = true
testing_macros.workspace = true
Expand Down

0 comments on commit ec8cbe2

Please sign in to comment.