Fix var collisions in strict eval calls (#2382)

This Pull Request allows collisions of var declarations with already existing lexical bindings if the `eval` call is strict or occurs within strict code. In short, it allows:

```Javascript
{
  let x;
  {
    eval('"use strict"; var x;');
  }
}
```

and

```Javascript
"use strict";
{
  let x;
  {
    eval('var x;');
  }
}
```

This is valid since in strict code all `eval` calls get their own function environment, making it impossible to declare a new var in the outer function environment. This change also skips poisoning environments on strict code, because `eval` cannot add new declarations for the current environment in that situation.

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<JsValue> {
+        enum EnvStackAction {
+            Truncate(usize),
+            Restore(Vec<Gc<DeclarativeEnvironment>>),
+        }
+
         // 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,35 @@ 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)?;
+        // 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
                 .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
     }
 }

boa_engine/src/context/mod.rs

@@ -501,11 +501,7 @@ impl Context {
     ) -> JsResult<Gc<CodeBlock>> {
         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()))
     }
 

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<Vec<Option<JsValue>>>,
-    compile: Gc<Cell<CompileTimeEnvironment>>,
+    bindings: GcCell<Vec<Option<JsValue>>>,
+    compile: Gc<GcCell<CompileTimeEnvironment>>,
+    #[unsafe_ignore_trace]
     poisoned: Cell<bool>,
     slots: Option<EnvironmentSlots>,
 }
@@ -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<FunctionSlots>),
+    Function(GcCell<FunctionSlots>),
     Global,
 }
 
 impl EnvironmentSlots {
     /// Return the slots if they are part of a function environment.
-    pub(crate) fn as_function_slots(&self) -> Option<&Cell<FunctionSlots>> {
+    pub(crate) fn as_function_slots(&self) -> Option<&GcCell<FunctionSlots>> {
         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<Cell<CompileTimeEnvironment>>) -> Self {
+    pub(crate) fn new(global_compile_environment: Gc<GcCell<CompileTimeEnvironment>>) -> 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<Cell<CompileTimeEnvironment>>,
+        compile_environment: Gc<GcCell<CompileTimeEnvironment>>,
     ) {
         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<Cell<CompileTimeEnvironment>>,
+        compile_environment: Gc<GcCell<CompileTimeEnvironment>>,
         this: Option<JsValue>,
         function_object: JsObject,
         new_target: Option<JsObject>,
@@ -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<Cell<CompileTimeEnvironment>>,
+        compile_environment: Gc<GcCell<CompileTimeEnvironment>>,
     ) {
         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<Cell<CompileTimeEnvironment>> {
+    pub(crate) fn current_compile_environment(&self) -> Gc<GcCell<CompileTimeEnvironment>> {
         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<JsValue> {
         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();

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 <variable> 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,
-                        ));
+                        )));
                     }
                     _ => {}
                 }