From ac4e5dc97ca1bfd77343f583b87655226a2b992e Mon Sep 17 00:00:00 2001 From: jedel1043 Date: Wed, 26 Oct 2022 01:52:30 -0500 Subject: [PATCH 1/2] Fix var collisions in strict eval calls --- boa_engine/src/builtins/eval/mod.rs | 87 ++++++++++++------- boa_engine/src/context/mod.rs | 6 +- boa_engine/src/environments/runtime.rs | 59 +++++++------ .../src/syntax/parser/expression/unary.rs | 8 +- 4 files changed, 89 insertions(+), 71 deletions(-) diff --git a/boa_engine/src/builtins/eval/mod.rs b/boa_engine/src/builtins/eval/mod.rs index fc1088c4eed..1fa4cf4c9cf 100644 --- a/boa_engine/src/builtins/eval/mod.rs +++ b/boa_engine/src/builtins/eval/mod.rs @@ -11,11 +11,13 @@ use crate::{ builtins::{BuiltIn, JsArgs}, + environments::DeclarativeEnvironment, error::JsNativeError, object::FunctionBuilder, property::Attribute, Context, JsResult, JsValue, }; +use boa_gc::Gc; use boa_profiler::Profiler; use rustc_hash::FxHashSet; @@ -63,9 +65,14 @@ impl Eval { pub(crate) fn perform_eval( x: &JsValue, direct: bool, - strict: bool, + mut strict: bool, context: &mut Context, ) -> JsResult { + enum EnvStackAction { + Truncate(usize), + Restore(Vec>), + } + // 1. Assert: If direct is false, then strictCaller is also false. debug_assert!(direct || !strict); @@ -85,20 +92,44 @@ impl Eval { Err(e) => return Err(JsNativeError::syntax().with_message(e.to_string()).into()), }; - // 12 - 13 are implicit in the call of `Context::compile_with_new_declarative`. + strict |= body.strict(); - // Because our environment model does not map directly to the spec this section looks very different. + // Because our environment model does not map directly to the spec, this section looks very different. + // 12 - 13 are implicit in the call of `Context::compile_with_new_declarative`. // 14 - 33 are in the following section, together with EvalDeclarationInstantiation. - if direct { + let action = if direct { // If the call to eval is direct, the code is executed in the current environment. // Poison the current environment, because it may contain new declarations after/during eval. - context.realm.environments.poison_current(); + if !strict { + context.realm.environments.poison_current(); + } // Set the compile time environment to the current running environment and save the number of current environments. context.realm.compile_env = context.realm.environments.current_compile_environment(); let environments_len = context.realm.environments.len(); + // Pop any added runtime environments that were not removed during the eval execution. + EnvStackAction::Truncate(environments_len) + } else { + // If the call to eval is indirect, the code is executed in the global environment. + + // Poison all environments, because the global environment may contain new declarations after/during eval. + if !strict { + context.realm.environments.poison_all(); + } + + // Pop all environments before the eval execution. + let environments = context.realm.environments.pop_to_global(); + context.realm.compile_env = context.realm.environments.current_compile_environment(); + + // Restore all environments to the state from before the eval execution. + EnvStackAction::Restore(environments) + }; + + // Only need to check on non-strict mode since strict mode automatically creates a function + // environment for all eval calls. + if !strict { // Error if any var declaration in the eval code already exists as a let/const declaration in the current running environment. let mut vars = FxHashSet::default(); body.var_declared_names(&mut vars); @@ -111,39 +142,31 @@ impl Eval { let msg = format!("variable declaration {name} in eval function already exists as a lexical variable"); return Err(JsNativeError::syntax().with_message(msg).into()); } + } - // Compile and execute the eval statement list. - let code_block = context.compile_with_new_declarative(&body, strict)?; + // TODO: check if private identifiers inside `eval` are valid. + + // Compile and execute the eval statement list. + let code_block = context.compile_with_new_declarative(&body, strict)?; + if direct { context .realm .environments .extend_outer_function_environment(); - let result = context.execute(code_block); - - // Pop any added runtime environments that where not removed during the eval execution. - context.realm.environments.truncate(environments_len); - - result - } else { - // If the call to eval is indirect, the code is executed in the global environment. - - // Poison all environments, because the global environment may contain new declarations after/during eval. - context.realm.environments.poison_all(); - - // Pop all environments before the eval execution. - let environments = context.realm.environments.pop_to_global(); - let environments_len = context.realm.environments.len(); - context.realm.compile_env = context.realm.environments.current_compile_environment(); - - // Compile and execute the eval statement list. - let code_block = context.compile_with_new_declarative(&body, false)?; - let result = context.execute(code_block); - - // Restore all environments to the state from before the eval execution. - context.realm.environments.truncate(environments_len); - context.realm.environments.extend(environments); + } + let result = context.execute(code_block); - result + match action { + EnvStackAction::Truncate(size) => { + context.realm.environments.truncate(size); + } + EnvStackAction::Restore(envs) => { + // Pop all environments created during the eval execution and restore the original stack. + context.realm.environments.truncate(1); + context.realm.environments.extend(envs); + } } + + result } } diff --git a/boa_engine/src/context/mod.rs b/boa_engine/src/context/mod.rs index 0265946ab6d..2a308cd3c7d 100644 --- a/boa_engine/src/context/mod.rs +++ b/boa_engine/src/context/mod.rs @@ -501,11 +501,7 @@ impl Context { ) -> JsResult> { let _timer = Profiler::global().start_event("Compilation", "Main"); let mut compiler = ByteCompiler::new(Sym::MAIN, statement_list.strict(), self); - compiler.compile_statement_list_with_new_declarative( - statement_list, - true, - strict || statement_list.strict(), - )?; + compiler.compile_statement_list_with_new_declarative(statement_list, true, strict)?; Ok(Gc::new(compiler.finish())) } diff --git a/boa_engine/src/environments/runtime.rs b/boa_engine/src/environments/runtime.rs index e7d2be0e77a..d2c74b96e44 100644 --- a/boa_engine/src/environments/runtime.rs +++ b/boa_engine/src/environments/runtime.rs @@ -1,8 +1,10 @@ +use std::cell::Cell; + use crate::{ environments::CompileTimeEnvironment, error::JsNativeError, object::JsObject, syntax::ast::expression::Identifier, Context, JsValue, }; -use boa_gc::{Cell, Finalize, Gc, Trace}; +use boa_gc::{Cell as GcCell, Finalize, Gc, Trace}; use rustc_hash::FxHashSet; @@ -28,8 +30,9 @@ use rustc_hash::FxHashSet; /// All poisoned environments have to be checked for added bindings. #[derive(Debug, Trace, Finalize)] pub(crate) struct DeclarativeEnvironment { - bindings: Cell>>, - compile: Gc>, + bindings: GcCell>>, + compile: Gc>, + #[unsafe_ignore_trace] poisoned: Cell, slots: Option, } @@ -37,13 +40,13 @@ pub(crate) struct DeclarativeEnvironment { /// Describes the different types of internal slot data that an environment can hold. #[derive(Clone, Debug, Trace, Finalize)] pub(crate) enum EnvironmentSlots { - Function(Cell), + Function(GcCell), Global, } impl EnvironmentSlots { /// Return the slots if they are part of a function environment. - pub(crate) fn as_function_slots(&self) -> Option<&Cell> { + pub(crate) fn as_function_slots(&self) -> Option<&GcCell> { if let Self::Function(env) = &self { Some(env) } else { @@ -227,10 +230,10 @@ pub struct DeclarativeEnvironmentStack { impl DeclarativeEnvironmentStack { /// Create a new environment stack with the most outer declarative environment. #[inline] - pub(crate) fn new(global_compile_environment: Gc>) -> Self { + pub(crate) fn new(global_compile_environment: Gc>) -> Self { Self { stack: vec![Gc::new(DeclarativeEnvironment { - bindings: Cell::new(Vec::new()), + bindings: GcCell::new(Vec::new()), compile: global_compile_environment, poisoned: Cell::new(false), slots: Some(EnvironmentSlots::Global), @@ -354,18 +357,17 @@ impl DeclarativeEnvironmentStack { pub(crate) fn push_declarative( &mut self, num_bindings: usize, - compile_environment: Gc>, + compile_environment: Gc>, ) { let poisoned = self .stack .last() .expect("global environment must always exist") .poisoned - .borrow() - .to_owned(); + .get(); self.stack.push(Gc::new(DeclarativeEnvironment { - bindings: Cell::new(vec![None; num_bindings]), + bindings: GcCell::new(vec![None; num_bindings]), compile: compile_environment, poisoned: Cell::new(poisoned), slots: None, @@ -381,7 +383,7 @@ impl DeclarativeEnvironmentStack { pub(crate) fn push_function( &mut self, num_bindings: usize, - compile_environment: Gc>, + compile_environment: Gc>, this: Option, function_object: JsObject, new_target: Option, @@ -392,7 +394,7 @@ impl DeclarativeEnvironmentStack { .last() .expect("global environment must always exist"); - let poisoned = outer.poisoned.borrow().to_owned(); + let poisoned = outer.poisoned.get(); let this_binding_status = if lexical { ThisBindingStatus::Lexical @@ -409,10 +411,10 @@ impl DeclarativeEnvironmentStack { }; self.stack.push(Gc::new(DeclarativeEnvironment { - bindings: Cell::new(vec![None; num_bindings]), + bindings: GcCell::new(vec![None; num_bindings]), compile: compile_environment, poisoned: Cell::new(poisoned), - slots: Some(EnvironmentSlots::Function(Cell::new(FunctionSlots { + slots: Some(EnvironmentSlots::Function(GcCell::new(FunctionSlots { this, this_binding_status, function_object, @@ -429,18 +431,18 @@ impl DeclarativeEnvironmentStack { pub(crate) fn push_function_inherit( &mut self, num_bindings: usize, - compile_environment: Gc>, + compile_environment: Gc>, ) { let outer = self .stack .last() .expect("global environment must always exist"); - let poisoned = outer.poisoned.borrow().to_owned(); + let poisoned = outer.poisoned.get(); let slots = outer.slots.clone(); self.stack.push(Gc::new(DeclarativeEnvironment { - bindings: Cell::new(vec![None; num_bindings]), + bindings: GcCell::new(vec![None; num_bindings]), compile: compile_environment, poisoned: Cell::new(poisoned), slots, @@ -474,7 +476,7 @@ impl DeclarativeEnvironmentStack { /// # Panics /// /// Panics if no environment exists on the stack. - pub(crate) fn current_compile_environment(&self) -> Gc> { + pub(crate) fn current_compile_environment(&self) -> Gc> { self.stack .last() .expect("global environment must always exist") @@ -489,24 +491,21 @@ impl DeclarativeEnvironmentStack { /// Panics if no environment exists on the stack. #[inline] pub(crate) fn poison_current(&mut self) { - let mut poisoned = self - .stack + self.stack .last() .expect("global environment must always exist") .poisoned - .borrow_mut(); - *poisoned = true; + .set(true); } /// Mark that there may be added binding in all environments. #[inline] pub(crate) fn poison_all(&mut self) { for env in &mut self.stack { - let mut poisoned = env.poisoned.borrow_mut(); - if *poisoned { + if env.poisoned.get() { return; } - *poisoned = true; + env.poisoned.set(true); } } @@ -528,7 +527,7 @@ impl DeclarativeEnvironmentStack { .stack .get(env_index) .expect("environment index must be in range"); - if !*env.poisoned.borrow() { + if !env.poisoned.get() { break; } let compile = env.compile.borrow(); @@ -559,7 +558,7 @@ impl DeclarativeEnvironmentStack { #[inline] pub(crate) fn get_value_global_poisoned(&self, name: Identifier) -> Option { for env in self.stack.iter().rev() { - if !*env.poisoned.borrow() { + if !env.poisoned.get() { return None; } let compile = env.compile.borrow(); @@ -624,7 +623,7 @@ impl DeclarativeEnvironmentStack { .stack .get(env_index) .expect("environment index must be in range"); - if !*env.poisoned.borrow() { + if !env.poisoned.get() { break; } let compile = env.compile.borrow(); @@ -692,7 +691,7 @@ impl DeclarativeEnvironmentStack { #[inline] pub(crate) fn put_value_global_poisoned(&mut self, name: Identifier, value: &JsValue) -> bool { for env in self.stack.iter().rev() { - if !*env.poisoned.borrow() { + if !env.poisoned.get() { return false; } let compile = env.compile.borrow(); diff --git a/boa_engine/src/syntax/parser/expression/unary.rs b/boa_engine/src/syntax/parser/expression/unary.rs index 060a2fb6d7a..241c15d7646 100644 --- a/boa_engine/src/syntax/parser/expression/unary.rs +++ b/boa_engine/src/syntax/parser/expression/unary.rs @@ -84,15 +84,15 @@ where match val { Expression::Identifier(_) if cursor.strict_mode() => { return Err(ParseError::lex(LexError::Syntax( - "Delete statements not allowed in strict mode".into(), + "cannot delete variables in strict mode".into(), token_start, ))); } Expression::PropertyAccess(PropertyAccess::Private(_)) => { - return Err(ParseError::general( - "private fields can not be deleted", + return Err(ParseError::lex(LexError::Syntax( + "cannot delete private fields".into(), position, - )); + ))); } _ => {} } From a66a08a394df22b19c17671a40fa5a396315807d Mon Sep 17 00:00:00 2001 From: jedel1043 Date: Wed, 26 Oct 2022 12:06:28 -0500 Subject: [PATCH 2/2] Skip extending env on strict mode --- boa_engine/src/builtins/eval/mod.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/boa_engine/src/builtins/eval/mod.rs b/boa_engine/src/builtins/eval/mod.rs index 1fa4cf4c9cf..a7a2c287bfe 100644 --- a/boa_engine/src/builtins/eval/mod.rs +++ b/boa_engine/src/builtins/eval/mod.rs @@ -148,7 +148,11 @@ impl Eval { // Compile and execute the eval statement list. let code_block = context.compile_with_new_declarative(&body, strict)?; - if direct { + // Indirect calls don't need extensions, because a non-strict indirect call modifies only + // the global object. + // Strict direct calls also don't need extensions, since all strict eval calls push a new + // function environment before evaluating. + if direct && !strict { context .realm .environments