From 958045c9ac963ca3b0fe8022832765102917dd6e Mon Sep 17 00:00:00 2001 From: Hans Larsen Date: Fri, 5 Apr 2024 12:43:48 -0700 Subject: [PATCH] Properly resolve paths in SimpleModuleLoader and add path to Referrer::Script (#3791) * Properly resolve paths in SimpleModuleLoader and add path to Referrer::Script * Add missing example * Carry active runnable on frame push * Duplicate the tests with proper paths for Windows/Unix * Use the right root for absolute referrers * ugh cargo fmt * Limit doctest to unix * Limit doctest to unix, for real * cargo fmt --------- Co-authored-by: jedel1043 --- .vscode/launch.json | 17 +++ .vscode/tasks.json | 10 ++ Cargo.lock | 34 +++++ core/engine/Cargo.toml | 7 +- core/engine/src/context/mod.rs | 23 ++-- core/engine/src/module/loader.rs | 218 +++++++++++++++++++++++++------ core/engine/src/script.rs | 12 +- core/engine/src/vm/mod.rs | 6 + core/engine/tests/imports.rs | 2 +- tests/tester/src/main.rs | 39 +++--- tests/tester/src/read.rs | 16 ++- 11 files changed, 302 insertions(+), 82 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index ee01298161..3b441e7c6f 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -33,6 +33,18 @@ ], "sourceLanguages": ["rust"], "preLaunchTask": "Cargo Build boa_cli" + }, + { + "type": "lldb", + "request": "launch", + "name": "Debug Boa (Tester)", + "windows": { + "program": "${workspaceFolder}/target/debug/boa_tester.exe" + }, + "program": "${workspaceFolder}/target/debug/boa_tester", + "args": ["run", "-s", "${input:testPath}", "-vvv"], + "sourceLanguages": ["rust"], + "preLaunchTask": "Cargo Build boa_tester" } ], "inputs": [ @@ -47,6 +59,11 @@ "description": "Relative path to the module root directory", "default": "debug", "type": "promptString" + }, + { + "id": "testPath", + "description": "Relative path to the test from the test262 directory", + "type": "promptString" } ] } diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 5940f85e3d..dcb961c709 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -23,6 +23,16 @@ "clear": true } }, + { + "type": "process", + "label": "Cargo Build boa_tester", + "command": "cargo", + "args": ["build", "-p", "boa_tester"], + "group": "build", + "presentation": { + "clear": true + } + }, { "type": "process", "label": "Run JS file", diff --git a/Cargo.lock b/Cargo.lock index 1423f11c2e..61f7048fcb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -438,6 +438,7 @@ dependencies = [ "sys-locale", "tap", "temporal_rs", + "test-case", "textwrap", "thin-vec", "thiserror", @@ -3735,6 +3736,39 @@ dependencies = [ "winapi-util", ] +[[package]] +name = "test-case" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "eb2550dd13afcd286853192af8601920d959b14c401fcece38071d53bf0768a8" +dependencies = [ + "test-case-macros", +] + +[[package]] +name = "test-case-core" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "adcb7fd841cd518e279be3d5a3eb0636409487998a4aff22f3de87b81e88384f" +dependencies = [ + "cfg-if", + "proc-macro2", + "quote", + "syn 2.0.57", +] + +[[package]] +name = "test-case-macros" +version = "3.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c89e72a01ed4c579669add59014b9a524d609c0c88c6a585ce37485879f6ffb" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.57", + "test-case-core", +] + [[package]] name = "textwrap" version = "0.16.1" diff --git a/core/engine/Cargo.toml b/core/engine/Cargo.toml index 9436ef0d05..184eafb100 100644 --- a/core/engine/Cargo.toml +++ b/core/engine/Cargo.toml @@ -68,7 +68,7 @@ js = ["dep:web-time"] [dependencies] boa_interner.workspace = true -boa_gc = { workspace = true, features = [ "thin-vec" ] } +boa_gc = { workspace = true, features = ["thin-vec"] } boa_profiler.workspace = true boa_macros.workspace = true boa_ast.workspace = true @@ -106,7 +106,7 @@ time.workspace = true hashbrown.workspace = true # intl deps -boa_icu_provider = {workspace = true, features = ["std"], optional = true } +boa_icu_provider = { workspace = true, features = ["std"], optional = true } sys-locale = { version = "0.3.1", optional = true } icu_provider = { workspace = true, optional = true } icu_locid = { workspace = true, features = ["serde"], optional = true } @@ -116,7 +116,7 @@ icu_calendar = { workspace = true, default-features = false, optional = true } icu_collator = { workspace = true, default-features = false, features = ["serde"], optional = true } icu_plurals = { workspace = true, default-features = false, features = ["serde", "experimental"], optional = true } icu_list = { workspace = true, default-features = false, features = ["serde"], optional = true } -icu_casemap = { workspace = true, default-features = false, features = ["serde"], optional = true} +icu_casemap = { workspace = true, default-features = false, features = ["serde"], optional = true } icu_segmenter = { workspace = true, default-features = false, features = ["auto", "serde"], optional = true } icu_decimal = { workspace = true, default-features = false, features = ["serde"], optional = true } writeable = { workspace = true, optional = true } @@ -137,6 +137,7 @@ float-cmp = "0.9.0" indoc.workspace = true textwrap.workspace = true futures-lite = "2.3.0" +test-case = "3.3.1" [target.x86_64-unknown-linux-gnu.dev-dependencies] jemallocator = "0.5.4" diff --git a/core/engine/src/context/mod.rs b/core/engine/src/context/mod.rs index 453e3891f6..c69ebd5ec8 100644 --- a/core/engine/src/context/mod.rs +++ b/core/engine/src/context/mod.rs @@ -1,20 +1,17 @@ //! The ECMAScript context. -mod hooks; -#[cfg(feature = "intl")] -pub(crate) mod icu; -pub mod intrinsics; +use std::{cell::Cell, path::Path, rc::Rc}; +use boa_ast::StatementList; +use boa_interner::Interner; use boa_parser::source::ReadChar; +use boa_profiler::Profiler; pub use hooks::{DefaultHooks, HostHooks}; - #[cfg(feature = "intl")] pub use icu::IcuError; - use intrinsics::Intrinsics; -use std::{cell::Cell, path::Path, rc::Rc}; - +use crate::vm::RuntimeLimits; use crate::{ builtins, class::{Class, ClassBuilder}, @@ -30,14 +27,14 @@ use crate::{ vm::{ActiveRunnable, CallFrame, Vm}, JsNativeError, JsResult, JsString, JsValue, Source, }; -use boa_ast::StatementList; -use boa_interner::Interner; -use boa_profiler::Profiler; - -use crate::vm::RuntimeLimits; use self::intrinsics::StandardConstructor; +mod hooks; +#[cfg(feature = "intl")] +pub(crate) mod icu; +pub mod intrinsics; + thread_local! { static CANNOT_BLOCK_COUNTER: Cell = const { Cell::new(0) }; } diff --git a/core/engine/src/module/loader.rs b/core/engine/src/module/loader.rs index 9fcf9d73e5..7381e4109b 100644 --- a/core/engine/src/module/loader.rs +++ b/core/engine/src/module/loader.rs @@ -1,4 +1,4 @@ -use std::path::{Path, PathBuf}; +use std::path::{Component, Path, PathBuf}; use rustc_hash::FxHashMap; @@ -13,6 +13,97 @@ use crate::{ use super::Module; +/// Resolves paths from the referrer and the specifier, normalize the paths and ensure the path +/// is within a base. If the base is empty, that last verification will be skipped. +/// +/// The returned specifier is a resolved absolute path that is guaranteed to be +/// a descendant of `base`. All path component that are either empty or `.` and +/// `..` have been resolved. +/// +/// # Errors +/// This predicate will return an error if the specifier is relative but the referrer +/// does not have a path, or if the resolved path is outside `base`. +/// +/// # Examples +/// ``` +/// #[cfg(target_family = "unix")] +/// # { +/// # use std::path::Path; +/// # use boa_engine::{Context, js_string}; +/// # use boa_engine::module::resolve_module_specifier; +/// assert_eq!( +/// resolve_module_specifier( +/// Some(Path::new("/base")), +/// &js_string!("../a.js"), +/// Some(Path::new("/base/hello/ref.js")), +/// &mut Context::default() +/// ), +/// Ok("/base/a.js".into()) +/// ); +/// # } +/// ``` +pub fn resolve_module_specifier( + base: Option<&Path>, + specifier: &JsString, + referrer: Option<&Path>, + _context: &mut Context, +) -> JsResult { + let base = base.map_or_else(|| PathBuf::from(""), PathBuf::from); + let referrer_dir = referrer.and_then(|p| p.parent()); + + let specifier = specifier.to_std_string_escaped(); + let short_path = Path::new(&specifier); + + // In ECMAScript, a path is considered relative if it starts with + // `./` or `../`. In Rust it's any path that start with `/`. + let is_relative = short_path.starts_with(".") || short_path.starts_with(".."); + + let long_path = if is_relative { + if let Some(r_path) = referrer_dir { + base.join(r_path).join(short_path) + } else { + return Err(JsError::from_opaque( + js_string!("relative path without referrer").into(), + )); + } + } else { + base.join(&specifier) + }; + + if long_path.is_relative() { + return Err(JsError::from_opaque( + js_string!("resolved path is relative").into(), + )); + } + + // Normalize the path. We cannot use `canonicalize` here because it will fail + // if the path doesn't exist. + let path = long_path + .components() + .filter(|c| c != &Component::CurDir || c == &Component::Normal("".as_ref())) + .try_fold(PathBuf::new(), |mut acc, c| { + if c == Component::ParentDir { + if acc.as_os_str().is_empty() { + return Err(JsError::from_opaque( + js_string!("path is outside the module root").into(), + )); + } + acc.pop(); + } else { + acc.push(c); + } + Ok(acc) + })?; + + if path.starts_with(&base) { + Ok(path) + } else { + Err(JsError::from_opaque( + js_string!("path is outside the module root").into(), + )) + } +} + /// The referrer from which a load request of a module originates. #[derive(Debug, Clone)] pub enum Referrer { @@ -31,7 +122,7 @@ impl Referrer { match self { Self::Module(module) => module.path(), Self::Realm(_) => None, - Self::Script(_script) => None, + Self::Script(script) => script.path(), } } } @@ -194,53 +285,21 @@ impl ModuleLoader for SimpleModuleLoader { context: &mut Context, ) { let result = (|| { - // If the referrer has a path, we use it as the base for the specifier. - let path = specifier - .to_std_string() - .map_err(|err| JsNativeError::typ().with_message(err.to_string()))?; - - let short_path = Path::new(&path); - - let path = if let Some(p) = referrer.path().and_then(|p| p.parent()) { - let root = if p.is_absolute() { - p.to_path_buf() - } else { - self.root.join(p) - }; - root.join(short_path) - } else { - self.root.join(short_path) - }; - - // Make sure we don't exit the root. - if !path.starts_with(&self.root) { - return Err(JsNativeError::typ() - .with_message(format!( - "path `{}` is outside the module root", - path.display() - )) - .into()); - } - - let path = path.canonicalize().map_err(|err| { - JsNativeError::typ() - .with_message(format!( - "could not canonicalize path `{}`", - short_path.display() - )) - .with_cause(JsError::from_opaque(js_string!(err.to_string()).into())) - })?; + let short_path = specifier.to_std_string_escaped(); + let path = + resolve_module_specifier(Some(&self.root), &specifier, referrer.path(), context)?; if let Some(module) = self.get(&path) { return Ok(module); } + let source = Source::from_filepath(&path).map_err(|err| { JsNativeError::typ() - .with_message(format!("could not open file `{}`", short_path.display())) + .with_message(format!("could not open file `{short_path}`")) .with_cause(JsError::from_opaque(js_string!(err.to_string()).into())) })?; let module = Module::parse(source, None, context).map_err(|err| { JsNativeError::syntax() - .with_message(format!("could not parse module `{}`", short_path.display())) + .with_message(format!("could not parse module `{short_path}`")) .with_cause(err) })?; self.insert(path, module.clone()); @@ -250,3 +309,82 @@ impl ModuleLoader for SimpleModuleLoader { finish_load(result, context); } } + +#[cfg(test)] +mod tests { + use std::path::PathBuf; + + use test_case::test_case; + + use super::*; + + // Tests on Windows and Linux are different because of the path separator and the definition + // of absolute paths. + #[rustfmt::skip] + #[cfg(target_family = "unix")] + #[test_case(Some("/hello/ref.js"), "a.js", Ok("/base/a.js"))] + #[test_case(Some("/base/ref.js"), "./b.js", Ok("/base/b.js"))] + #[test_case(Some("/base/other/ref.js"), "./c.js", Ok("/base/other/c.js"))] + #[test_case(Some("/base/other/ref.js"), "../d.js", Ok("/base/d.js"))] + #[test_case(Some("/base/ref.js"), "e.js", Ok("/base/e.js"))] + #[test_case(Some("/base/ref.js"), "./f.js", Ok("/base/f.js"))] + #[test_case(Some("./ref.js"), "./g.js", Ok("/base/g.js"))] + #[test_case(Some("./other/ref.js"), "./other/h.js", Ok("/base/other/other/h.js"))] + #[test_case(Some("./other/ref.js"), "./other/../h1.js", Ok("/base/other/h1.js"))] + #[test_case(Some("./other/ref.js"), "./../h2.js", Ok("/base/h2.js"))] + #[test_case(None, "./i.js", Err(()))] + #[test_case(None, "j.js", Ok("/base/j.js"))] + #[test_case(None, "other/k.js", Ok("/base/other/k.js"))] + #[test_case(None, "other/../../l.js", Err(()))] + #[test_case(Some("/base/ref.js"), "other/../../m.js", Err(()))] + #[test_case(None, "../n.js", Err(()))] + fn resolve_test(ref_path: Option<&str>, spec: &str, expected: Result<&str, ()>) { + let base = PathBuf::from("/base"); + + let mut context = Context::default(); + let spec = js_string!(spec); + let ref_path = ref_path.map(PathBuf::from); + + let actual = resolve_module_specifier( + Some(&base), + &spec, + ref_path.as_deref(), + &mut context, + ); + assert_eq!(actual.map_err(|_| ()), expected.map(PathBuf::from)); + } + + #[rustfmt::skip] + #[cfg(target_family = "windows")] + #[test_case(Some("a:\\hello\\ref.js"), "a.js", Ok("a:\\base\\a.js"))] + #[test_case(Some("a:\\base\\ref.js"), ".\\b.js", Ok("a:\\base\\b.js"))] + #[test_case(Some("a:\\base\\other\\ref.js"), ".\\c.js", Ok("a:\\base\\other\\c.js"))] + #[test_case(Some("a:\\base\\other\\ref.js"), "..\\d.js", Ok("a:\\base\\d.js"))] + #[test_case(Some("a:\\base\\ref.js"), "e.js", Ok("a:\\base\\e.js"))] + #[test_case(Some("a:\\base\\ref.js"), ".\\f.js", Ok("a:\\base\\f.js"))] + #[test_case(Some(".\\ref.js"), ".\\g.js", Ok("a:\\base\\g.js"))] + #[test_case(Some(".\\other\\ref.js"), ".\\other\\h.js", Ok("a:\\base\\other\\other\\h.js"))] + #[test_case(Some(".\\other\\ref.js"), ".\\other\\..\\h1.js", Ok("a:\\base\\other\\h1.js"))] + #[test_case(Some(".\\other\\ref.js"), ".\\..\\h2.js", Ok("a:\\base\\h2.js"))] + #[test_case(None, ".\\i.js", Err(()))] + #[test_case(None, "j.js", Ok("a:\\base\\j.js"))] + #[test_case(None, "other\\k.js", Ok("a:\\base\\other\\k.js"))] + #[test_case(None, "other\\..\\..\\l.js", Err(()))] + #[test_case(Some("\\base\\ref.js"), "other\\..\\..\\m.js", Err(()))] + #[test_case(None, "..\\n.js", Err(()))] + fn resolve_test(ref_path: Option<&str>, spec: &str, expected: Result<&str, ()>) { + let base = PathBuf::from("a:\\base"); + + let mut context = Context::default(); + let spec = js_string!(spec); + let ref_path = ref_path.map(PathBuf::from); + + let actual = resolve_module_specifier( + Some(&base), + &spec, + ref_path.as_deref(), + &mut context, + ); + assert_eq!(actual.map_err(|_| ()), expected.map(PathBuf::from)); + } +} diff --git a/core/engine/src/script.rs b/core/engine/src/script.rs index 276fbebd3e..bb34e98188 100644 --- a/core/engine/src/script.rs +++ b/core/engine/src/script.rs @@ -8,10 +8,13 @@ //! [spec]: https://tc39.es/ecma262/#sec-scripts //! [script]: https://tc39.es/ecma262/#sec-script-records +use std::path::{Path, PathBuf}; + +use rustc_hash::FxHashMap; + use boa_gc::{Finalize, Gc, GcRefCell, Trace}; use boa_parser::{source::ReadChar, Parser, Source}; use boa_profiler::Profiler; -use rustc_hash::FxHashMap; use crate::{ bytecompiler::ByteCompiler, @@ -47,6 +50,7 @@ struct Inner { codeblock: GcRefCell>>, loaded_modules: GcRefCell>, host_defined: HostDefined, + path: Option, } impl Script { @@ -80,6 +84,7 @@ impl Script { context: &mut Context, ) -> JsResult { let _timer = Profiler::global().start_event("Script parsing", "Main"); + let path = src.path().map(std::path::Path::to_path_buf); let mut parser = Parser::new(src); parser.set_identifier(context.next_parser_identifier()); if context.is_strict() { @@ -97,6 +102,7 @@ impl Script { codeblock: GcRefCell::default(), loaded_modules: GcRefCell::default(), host_defined: HostDefined::default(), + path, }), }) } @@ -212,4 +218,8 @@ impl Script { Ok(()) } + + pub(super) fn path(&self) -> Option<&Path> { + self.inner.path.as_deref() + } } diff --git a/core/engine/src/vm/mod.rs b/core/engine/src/vm/mod.rs index 00815f4b4a..46fcdfc297 100644 --- a/core/engine/src/vm/mod.rs +++ b/core/engine/src/vm/mod.rs @@ -174,6 +174,12 @@ impl Vm { ); } + // Keep carrying the last active runnable in case the current callframe + // yields. + if frame.active_runnable.is_none() { + frame.active_runnable = self.frames.last().and_then(|fr| fr.active_runnable.clone()); + } + self.frames.push(frame); } diff --git a/core/engine/tests/imports.rs b/core/engine/tests/imports.rs index 2257d759e3..d3f38e2c68 100644 --- a/core/engine/tests/imports.rs +++ b/core/engine/tests/imports.rs @@ -19,7 +19,7 @@ fn subdirectories() { .build() .unwrap(); - let source = Source::from_bytes(b"export { file1 } from './file1.js';"); + let source = Source::from_bytes(b"export { file1 } from 'file1.js';"); let module = boa_engine::Module::parse(source, None, &mut context).unwrap(); let result = module.load_link_evaluate(&mut context); diff --git a/tests/tester/src/main.rs b/tests/tester/src/main.rs index 79999829cd..dfeac5b5d3 100644 --- a/tests/tester/src/main.rs +++ b/tests/tester/src/main.rs @@ -9,38 +9,41 @@ clippy::cast_precision_loss )] -mod edition; -mod exec; -mod read; -mod results; - -use self::{ - read::{read_harness, read_suite, read_test, MetaData, Negative, TestFlag}, - results::{compare_results, write_json}, +use std::{ + ops::{Add, AddAssign}, + path::{Path, PathBuf}, + process::Command, + time::Instant, }; + use bitflags::bitflags; -use boa_engine::optimizer::OptimizerOptions; use clap::{ArgAction, Parser, ValueHint}; use color_eyre::{ eyre::{bail, eyre, WrapErr}, Result, }; use colored::Colorize; -use edition::SpecEdition; use once_cell::sync::Lazy; -use read::ErrorType; use rustc_hash::{FxHashMap, FxHashSet}; use serde::{ de::{Unexpected, Visitor}, Deserialize, Deserializer, Serialize, }; -use std::{ - ops::{Add, AddAssign}, - path::{Path, PathBuf}, - process::Command, - time::Instant, + +use boa_engine::optimizer::OptimizerOptions; +use edition::SpecEdition; +use read::ErrorType; + +use self::{ + read::{read_harness, read_suite, read_test, MetaData, Negative, TestFlag}, + results::{compare_results, write_json}, }; +mod edition; +mod exec; +mod read; +mod results; + static START: Lazy = Lazy::new(Instant::now); /// Structure that contains the configuration of the tester. @@ -225,7 +228,9 @@ fn main() -> Result<()> { clone_test262(test262_commit, verbose)?; Path::new(DEFAULT_TEST262_DIRECTORY) - }; + } + .canonicalize(); + let test262_path = &test262_path.wrap_err("could not get the Test262 path")?; run_test_suite( &config, diff --git a/tests/tester/src/read.rs b/tests/tester/src/read.rs index b486b381b2..44d4620328 100644 --- a/tests/tester/src/read.rs +++ b/tests/tester/src/read.rs @@ -1,19 +1,21 @@ //! Module to read the list of test suites from disk. -use crate::{HarnessFile, Ignored}; +use std::{ + ffi::OsStr, + fs, io, + path::{Path, PathBuf}, +}; -use super::{Harness, Locale, Phase, Test, TestSuite}; use color_eyre::{ eyre::{eyre, WrapErr}, Result, }; use rustc_hash::FxHashMap; use serde::Deserialize; -use std::{ - ffi::OsStr, - fs, io, - path::{Path, PathBuf}, -}; + +use crate::{HarnessFile, Ignored}; + +use super::{Harness, Locale, Phase, Test, TestSuite}; /// Representation of the YAML metadata in Test262 tests. #[derive(Debug, Clone, Deserialize)]