fix: process issues from external security review (#897)

* fix: avoid call stack overflow

* fix(engine): throw errors on more allocation failures

Author: aapoalas

nova_vm/src/ecmascript/abstract_operations/operations_on_iterator_objects.rs

@@ -25,7 +25,7 @@ use crate::{
     },
     engine::{
         ScopableCollection, ScopedCollection, VmIteratorRecord,
-        context::{Bindable, GcScope, bindable_handle},
+        context::{Bindable, GcScope, NoGcScope, bindable_handle},
         rootable::Scopable,
     },
     heap::{
@@ -609,7 +609,8 @@ pub(crate) fn create_iter_result_object<'a>(
     agent: &mut Agent,
     value: Value<'a>,
     done: bool,
-) -> OrdinaryObject<'a> {
+    gc: NoGcScope<'a, '_>,
+) -> JsResult<'a, OrdinaryObject<'a>> {
     // 1. Let obj be OrdinaryObjectCreate(%Object.prototype%).
     // 2. Perform ! CreateDataPropertyOrThrow(obj, "value", value).
     // 3. Perform ! CreateDataPropertyOrThrow(obj, "done", done).
@@ -644,6 +645,7 @@ pub(crate) fn create_iter_result_object<'a>(
             },
         ],
     )
+    .map_err(|err| agent.throw_allocation_exception(err, gc))
 }
 
 /// ### [7.4.16 IteratorToList ( iteratorRecord )](https://tc39.es/ecma262/#sec-iteratortolist)

nova_vm/src/ecmascript/abstract_operations/operations_on_objects.rs

@@ -2248,7 +2248,7 @@ pub(crate) fn try_copy_data_properties_into_object<'gc, 'b>(
         }
     }
 
-    TryResult::Continue(OrdinaryObject::create_object(
+    let obj = OrdinaryObject::create_object(
         agent,
         Some(
             agent
@@ -2258,7 +2258,13 @@ pub(crate) fn try_copy_data_properties_into_object<'gc, 'b>(
                 .into_object(),
         ),
         &entries,
-    ))
+    );
+    if let Ok(obj) = obj {
+        TryResult::Continue(obj)
+    } else {
+        // We failed to allocate: try run GC.
+        TryError::GcError.into()
+    }
 }
 
 /// ### [7.3.25 CopyDataProperties ( target, source, excludedItems )](https://tc39.es/ecma262/#sec-copydataproperties)
@@ -2327,7 +2333,7 @@ pub(crate) fn copy_data_properties_into_object<'a, 'b>(
         i += 1;
     }
 
-    let object = OrdinaryObject::create_object(
+    let object = match OrdinaryObject::create_object(
         agent,
         Some(
             agent
@@ -2337,8 +2343,10 @@ pub(crate) fn copy_data_properties_into_object<'a, 'b>(
                 .into_object(),
         ),
         &entries,
-    )
-    .bind(gc.nogc());
+    ) {
+        Ok(obj) => obj,
+        Err(err) => return Err(agent.throw_allocation_exception(err, gc.into_nogc())),
+    };
 
     if broke {
         let _ = keys.drain(..i);

nova_vm/src/ecmascript/builders/ordinary_object_builder.rs

@@ -361,7 +361,8 @@ pub(super) fn create_intrinsic_backing_object(
     let (cap, index) = agent
         .heap
         .elements
-        .allocate_keys_with_capacity(properties_count);
+        .allocate_keys_with_capacity(properties_count)
+        .expect("Intrinsic object's property allocation failed");
     let cap = cap.make_intrinsic();
     let keys_memory = agent.heap.elements.get_keys_uninit_raw(cap, index);
     for (slot, key) in keys_memory.iter_mut().zip(properties.iter().map(|e| e.0)) {

nova_vm/src/ecmascript/builtins/arguments.rs

@@ -26,6 +26,8 @@
 //!
 //! ECMAScript implementations of arguments exotic objects have historically contained an accessor property named "caller". Prior to ECMAScript 2017, this specification included the definition of a throwing "caller" property on ordinary arguments objects. Since implementations do not contain this extension any longer, ECMAScript 2017 dropped the requirement for a throwing "caller" accessor.
 
+use std::collections::TryReserveError;
+
 use ahash::AHashMap;
 
 use crate::{
@@ -124,7 +126,7 @@ pub(crate) fn create_unmapped_arguments_object<'a, 'b>(
     agent: &mut Agent,
     arguments_list: &ScopedArgumentsList<'b>,
     gc: NoGcScope<'a, 'b>,
-) -> Object<'a> {
+) -> Result<Object<'a>, TryReserveError> {
     // 1. Let len be the number of elements in argumentsList.
     let len = arguments_list.len(agent);
     // SAFETY: GC is not allowed in this scope, and no other scoped values are
@@ -136,11 +138,11 @@ pub(crate) fn create_unmapped_arguments_object<'a, 'b>(
     // 2. Let obj be OrdinaryObjectCreate(%Object.prototype%, « [[ParameterMap]] »).
     let prototype = agent.current_realm_record().intrinsics().object_prototype();
     let mut shape = ObjectShape::get_shape_for_prototype(agent, Some(prototype.into_object()));
-    shape = shape.get_child_shape(agent, BUILTIN_STRING_MEMORY.length.to_property_key());
-    shape = shape.get_child_shape(agent, BUILTIN_STRING_MEMORY.callee.into());
-    shape = shape.get_child_shape(agent, WellKnownSymbolIndexes::Iterator.into());
+    shape = shape.get_child_shape(agent, BUILTIN_STRING_MEMORY.length.to_property_key())?;
+    shape = shape.get_child_shape(agent, BUILTIN_STRING_MEMORY.callee.into())?;
+    shape = shape.get_child_shape(agent, WellKnownSymbolIndexes::Iterator.into())?;
     for index in 0..len {
-        shape = shape.get_child_shape(agent, index.into());
+        shape = shape.get_child_shape(agent, index.into())?;
     }
     let obj = OrdinaryObject::create_object_with_shape(agent, shape)
         .expect("Failed to create Arguments object storage");
@@ -213,7 +215,7 @@ pub(crate) fn create_unmapped_arguments_object<'a, 'b>(
     // [[Configurable]]: false
     // }).
     // 9. Return obj.
-    Object::Arguments(obj)
+    Ok(Object::Arguments(obj))
 }
 
 // 10.4.4.7 CreateMappedArgumentsObject ( func, formals, argumentsList, env )

nova_vm/src/ecmascript/builtins/array/abstract_operations.rs

@@ -63,7 +63,11 @@ pub(crate) fn array_create<'a>(
         {
             None
         } else {
-            Some(OrdinaryObject::create_object(agent, Some(proto), &[]).bind(gc))
+            Some(
+                OrdinaryObject::create_object(agent, Some(proto), &[])
+                    .expect("Should perform GC here")
+                    .bind(gc),
+            )
         }
     } else {
         None

nova_vm/src/ecmascript/builtins/bound_function.rs

@@ -169,6 +169,7 @@ impl<'a> FunctionInternalProperties<'a> for BoundFunction<'a> {
         arguments_list: ArgumentsList,
         gc: GcScope<'gc, '_>,
     ) -> JsResult<'gc, Value<'gc>> {
+        agent.check_call_depth(gc.nogc()).unbind()?;
         let f = self.bind(gc.nogc());
         let arguments_list = arguments_list.bind(gc.nogc());
         // 1. Let target be F.[[BoundTargetFunction]].
@@ -219,6 +220,7 @@ impl<'a> FunctionInternalProperties<'a> for BoundFunction<'a> {
         new_target: Function,
         gc: GcScope<'gc, '_>,
     ) -> JsResult<'gc, Object<'gc>> {
+        agent.check_call_depth(gc.nogc()).unbind()?;
         let arguments_list = arguments_list.bind(gc.nogc());
         let new_target = new_target.bind(gc.nogc());
         // 1. Let target be F.[[BoundTargetFunction]].

nova_vm/src/ecmascript/builtins/builtin_constructor.rs

@@ -351,7 +351,8 @@ pub(crate) fn create_builtin_constructor<'a>(
         },
     };
     let entries = [length_entry, name_entry, prototype_entry];
-    let backing_object = OrdinaryObject::create_intrinsic_object(agent, args.prototype, &entries);
+    let backing_object = OrdinaryObject::create_intrinsic_object(agent, args.prototype, &entries)
+        .expect("Should perform GC here");
 
     // 13. Return func.
     agent

nova_vm/src/ecmascript/builtins/builtin_function.rs

@@ -665,6 +665,8 @@ fn builtin_call_or_construct<'gc>(
     new_target: Option<Function>,
     gc: GcScope<'gc, '_>,
 ) -> JsResult<'gc, Value<'gc>> {
+    agent.check_call_depth(gc.nogc()).unbind()?;
+
     let f = f.bind(gc.nogc());
     let this_argument = this_argument.bind(gc.nogc());
     let arguments_list = arguments_list.bind(gc.nogc());
@@ -812,11 +814,10 @@ pub fn create_builtin_function<'a>(
                     configurable: true,
                 },
             };
-            Some(OrdinaryObject::create_object(
-                agent,
-                Some(prototype),
-                &[length_entry, name_entry],
-            ))
+            Some(
+                OrdinaryObject::create_object(agent, Some(prototype), &[length_entry, name_entry])
+                    .expect("Should perform GC here"),
+            )
         }
     } else {
         None

nova_vm/src/ecmascript/builtins/control_abstraction_objects/async_generator_objects/async_generator_abstract_operations.rs

@@ -127,7 +127,8 @@ fn async_generator_complete_step(
             agent.set_current_realm(realm);
         }
         // iii. Let iteratorResult be CreateIteratorResultObject(value, done).
-        let iterator_result = create_iter_result_object(agent, value, done);
+        let iterator_result =
+            create_iter_result_object(agent, value, done, gc).expect("Should perform GC here");
         // iv. Set the running execution context's Realm to oldRealm.
         if set_realm {
             agent.set_current_realm(old_realm);
@@ -136,7 +137,7 @@ fn async_generator_complete_step(
     } else {
         // c. Else,
         // i. Let iteratorResult be CreateIteratorResultObject(value, done).
-        create_iter_result_object(agent, value, done)
+        create_iter_result_object(agent, value, done, gc).expect("Should perform GC here")
     };
     // d. Perform ! Call(promiseCapability.[[Resolve]], undefined, « iteratorResult »).
     unwrap_try(promise_capability.try_resolve(agent, iterator_result.into_value(), gc));

nova_vm/src/ecmascript/builtins/control_abstraction_objects/async_generator_objects/async_generator_prototype.rs

@@ -86,7 +86,10 @@ impl AsyncGeneratorPrototype {
         // 6. If state is completed, then
         if state.is_completed() {
             // a. Let iteratorResult be CreateIteratorResultObject(undefined, true).
-            let iterator_result = create_iter_result_object(agent, Value::Undefined, true);
+            let iterator_result =
+                create_iter_result_object(agent, Value::Undefined, true, gc.nogc())
+                    .unbind()?
+                    .bind(gc.nogc());
             // b. Perform ! Call(promiseCapability.[[Resolve]], undefined, « iteratorResult »).
             promise_capability
                 .unbind()