feat(heap): Make number and string allocation functions unsafe (#60)

trynova · Nov 1, 2023 · a3e0e14 · a3e0e14
1 parent d2b4f50
commit a3e0e14
Show file tree

Hide file tree

Showing 10 changed files with 118 additions and 96 deletions.
diff --git a/nova_vm/src/ecmascript/types/language/number.rs b/nova_vm/src/ecmascript/types/language/number.rs
@@ -33,6 +33,12 @@ impl std::fmt::Debug for Number {
     }
 }
 
+impl From<NumberIndex> for Number {
+    fn from(value: NumberIndex) -> Self {
+        Number::Number(value)
+    }
+}
+
 impl From<SmallInteger> for Number {
     fn from(value: SmallInteger) -> Self {
         Number::Integer(value)
@@ -60,6 +66,25 @@ impl From<f32> for Number {
     }
 }
 
+const MAX_NUMBER: f64 = ((1u64 << 53) - 1) as f64;
+const MIN_NUMBER: f64 = -MAX_NUMBER;
+
+impl TryFrom<f64> for Number {
+    type Error = ();
+
+    fn try_from(value: f64) -> Result<Self, ()> {
+        if value.is_finite() && value.trunc() == value && (MIN_NUMBER..=MAX_NUMBER).contains(&value)
+        {
+            debug_assert_eq!(value as i64 as f64, value);
+            Ok(Number::try_from(value as i64).unwrap())
+        } else if value as f32 as f64 == value {
+            Ok(Number::Float(value as f32))
+        } else {
+            Err(())
+        }
+    }
+}
+
 impl TryFrom<Value> for Number {
     type Error = ();
     fn try_from(value: Value) -> Result<Self, Self::Error> {
@@ -76,13 +101,15 @@ impl TryFrom<Value> for Number {
 }
 
 impl Number {
-    pub fn new(value: Value) -> Self {
-        debug_assert!(matches!(
-            value,
-            Value::Number(_) | Value::Integer(_) | Value::Float(_)
-        ));
-        // SAFETY: Sub-enum.
-        unsafe { std::mem::transmute::<Value, Number>(value) }
+    pub fn from_f64(agent: &mut Agent, value: f64) -> Self {
+        if let Ok(value) = Number::try_from(value) {
+            value
+        } else {
+            // SAFETY: Number was not representable as a
+            // stack-allocated Number.
+            let id = unsafe { agent.heap.alloc_number(value) };
+            Number::Number(id)
+        }
     }
 
     pub fn nan() -> Self {

diff --git a/nova_vm/src/ecmascript/types/language/object/property_key.rs b/nova_vm/src/ecmascript/types/language/object/property_key.rs
@@ -11,7 +11,7 @@ use crate::{
     },
     heap::{
         indexes::{StringIndex, SymbolIndex},
-        GetHeapData,
+        CreateHeapData, GetHeapData,
     },
     Heap, SmallInteger, SmallString,
 };
@@ -28,11 +28,7 @@ pub enum PropertyKey {
 impl PropertyKey {
     // FIXME: This API is not necessarily in the right place.
     pub fn from_str(heap: &mut Heap, str: &str) -> Self {
-        if let Ok(ascii_string) = SmallString::try_from(str) {
-            PropertyKey::SmallString(ascii_string)
-        } else {
-            PropertyKey::String(heap.alloc_string(str))
-        }
+        heap.create(str).into()
     }
 
     pub fn into_value(self) -> Value {

diff --git a/nova_vm/src/ecmascript/types/language/string.rs b/nova_vm/src/ecmascript/types/language/string.rs
@@ -3,7 +3,7 @@ mod data;
 use super::Value;
 use crate::{
     ecmascript::execution::Agent,
-    heap::{indexes::StringIndex, GetHeapData},
+    heap::{indexes::StringIndex, CreateHeapData, GetHeapData},
     SmallString,
 };
 
@@ -59,11 +59,7 @@ impl From<String> for Value {
 
 impl String {
     pub fn from_str(agent: &mut Agent, message: &str) -> String {
-        if let Ok(ascii_string) = SmallString::try_from(message) {
-            String::SmallString(ascii_string)
-        } else {
-            String::String(agent.heap.alloc_string(message))
-        }
+        agent.heap.create(message)
     }
 
     pub fn from_small_string(message: &'static str) -> String {

diff --git a/nova_vm/src/ecmascript/types/language/value.rs b/nova_vm/src/ecmascript/types/language/value.rs
@@ -12,7 +12,7 @@ use crate::{
             ArrayBufferIndex, ArrayIndex, BigIntIndex, DateIndex, ErrorIndex, FunctionIndex,
             NumberIndex, ObjectIndex, RegExpIndex, StringIndex, SymbolIndex,
         },
-        GetHeapData,
+        CreateHeapData, GetHeapData,
     },
     Heap, SmallInteger, SmallString,
 };
@@ -130,26 +130,11 @@ pub(crate) const REGEXP_DISCRIMINANT: u8 =
 
 impl Value {
     pub fn from_str(heap: &mut Heap, message: &str) -> Value {
-        if let Ok(ascii_string) = SmallString::try_from(message) {
-            Value::SmallString(ascii_string)
-        } else {
-            Value::String(heap.alloc_string(message))
-        }
+        heap.create(message).into()
     }
 
-    pub fn from_f64(heap: &mut Heap, value: f64) -> Value {
-        let is_int = value.fract() == 0.0;
-        if is_int {
-            if let Ok(data) = Value::try_from(value as i64) {
-                return data;
-            }
-        }
-        if value as f32 as f64 == value {
-            // TODO: Verify logic
-            Value::Float(value as f32)
-        } else {
-            Value::Number(heap.alloc_number(value))
-        }
+    pub fn from_f64(agent: &mut Agent, value: f64) -> Value {
+        Number::from_f64(agent, value).into()
     }
 
     pub fn nan() -> Self {
@@ -314,12 +299,7 @@ impl TryFrom<&str> for Value {
 impl TryFrom<f64> for Value {
     type Error = ();
     fn try_from(value: f64) -> Result<Self, ()> {
-        // TODO: verify logic
-        if value as f32 as f64 == value {
-            Ok(Value::Float(value as f32))
-        } else {
-            Err(())
-        }
+        Number::try_from(value).map(|v| v.into())
     }
 }
 

diff --git a/nova_vm/src/engine/small_integer.rs b/nova_vm/src/engine/small_integer.rs
@@ -11,10 +11,10 @@ impl std::fmt::Debug for SmallInteger {
 }
 
 impl SmallInteger {
-    pub const MIN_BIGINT: i64 = -2i64.pow(55);
+    pub const MIN_BIGINT: i64 = -(2i64.pow(55));
     pub const MAX_BIGINT: i64 = 2i64.pow(55) - 1;
 
-    pub const MIN_NUMBER: i64 = -2i64.pow(53) + 1;
+    pub const MIN_NUMBER: i64 = -(2i64.pow(53)) + 1;
     pub const MAX_NUMBER: i64 = 2i64.pow(53) - 1;
 
     #[inline]

diff --git a/nova_vm/src/heap.rs b/nova_vm/src/heap.rs
@@ -87,13 +87,16 @@ pub trait GetHeapData<'a, T, F: 'a> {
 
 impl CreateHeapData<f64, Number> for Heap<'_, '_> {
     fn create(&mut self, data: f64) -> Number {
-        if let Ok(value) = Value::try_from(data) {
-            Number::new(value)
-        } else if data as f32 as f64 == data {
-            Number::new(Value::Float(data as f32))
+        // NOTE: This function cannot currently be implemented
+        // directly using `Number::from_f64` as it takes an Agent
+        // parameter that we do not have access to here.
+        if let Ok(value) = Number::try_from(data) {
+            value
         } else {
-            let id = self.alloc_number(data);
-            Value::Number(id).try_into().unwrap()
+            // SAFETY: Number was not representable as a
+            // stack-allocated Number.
+            let id = unsafe { self.alloc_number(data) };
+            Number::Number(id)
         }
     }
 }
@@ -153,7 +156,8 @@ impl CreateHeapData<&str, String> for Heap<'_, '_> {
         if let Ok(value) = String::try_from(data) {
             value
         } else {
-            let id = self.alloc_string(data);
+            // SAFETY: String couldn't be represented as a SmallString.
+            let id = unsafe { self.alloc_string(data) };
             Value::String(id).try_into().unwrap()
         }
     }
@@ -250,7 +254,18 @@ impl<'ctx, 'host> Heap<'ctx, 'host> {
             .expect("RealmIdentifier matched a freed Realm")
     }
 
-    pub fn alloc_string(&mut self, message: &str) -> StringIndex {
+    /// Allocate a string onto the Agent heap
+    ///
+    /// This method will currently iterate through all heap strings to look for
+    /// a possible matching string and if found will return its StringIndex
+    /// instead of allocating a copy.
+    ///
+    /// SAFETY: The string being allocated must not be representable as a
+    /// SmallString. All SmallStrings must be kept on the stack to ensure that
+    /// comparison between heap allocated strings and SmallStrings can be
+    /// guaranteed to never equal true.
+    pub unsafe fn alloc_string(&mut self, message: &str) -> StringIndex {
+        debug_assert!(message.len() > 7 || message.ends_with('\0'));
         let wtf8 = Wtf8::from_str(message);
         let found = self
             .strings
@@ -270,7 +285,13 @@ impl<'ctx, 'host> Heap<'ctx, 'host> {
         }
     }
 
-    pub fn alloc_number(&mut self, number: f64) -> NumberIndex {
+    /// Allocate a 64-bit floating point number onto the Agent heap
+    ///
+    /// SAFETY: The number being allocated must not be representable
+    /// as a SmallInteger or f32. All stack-allocated numbers must be
+    /// inequal to any heap-allocated number.
+    pub unsafe fn alloc_number(&mut self, number: f64) -> NumberIndex {
+        debug_assert!(number.fract() != 0.0 || number as f32 as f64 != number);
         self.numbers.push(Some(number.into()));
         NumberIndex::last(&self.numbers)
     }

diff --git a/nova_vm/src/heap/heap_gc.rs b/nova_vm/src/heap/heap_gc.rs
@@ -185,7 +185,7 @@ pub fn heap_gc(heap: &mut Heap) {
                 marked.store(true, Ordering::Relaxed);
                 let data = heap.symbols.get(index).unwrap().as_ref().unwrap();
                 if let Some(string_index) = data.descriptor {
-                    queues.push_value(Value::String(string_index));
+                    queues.push_value(string_index.into());
                 }
             }
         });

diff --git a/nova_vm/src/heap/math.rs b/nova_vm/src/heap/math.rs
@@ -1,22 +1,22 @@
 use super::{
     heap_constants::WellKnownSymbolIndexes,
     object::{ObjectEntry, PropertyDescriptor},
-    Heap,
+    CreateHeapData, Heap,
 };
 use crate::ecmascript::{
     execution::JsResult,
     types::{PropertyKey, Value},
 };
 
 pub(super) fn initialize_math_object(heap: &mut Heap) {
-    let e = Value::from_f64(heap, std::f64::consts::E);
-    let ln10 = Value::from_f64(heap, std::f64::consts::LN_10);
-    let ln2 = Value::from_f64(heap, std::f64::consts::LN_2);
-    let log10e = Value::from_f64(heap, std::f64::consts::LOG10_E);
-    let log2e = Value::from_f64(heap, std::f64::consts::LOG2_E);
-    let pi = Value::from_f64(heap, std::f64::consts::PI);
-    let sqrt1_2 = Value::from_f64(heap, std::f64::consts::FRAC_1_SQRT_2);
-    let sqrt2 = Value::from_f64(heap, std::f64::consts::SQRT_2);
+    let e = heap.create(std::f64::consts::E);
+    let ln10 = heap.create(std::f64::consts::LN_10);
+    let ln2 = heap.create(std::f64::consts::LN_2);
+    let log10e = heap.create(std::f64::consts::LOG10_E);
+    let log2e = heap.create(std::f64::consts::LOG2_E);
+    let pi = heap.create(std::f64::consts::PI);
+    let sqrt1_2 = heap.create(std::f64::consts::FRAC_1_SQRT_2);
+    let sqrt2 = heap.create(std::f64::consts::SQRT_2);
     let abs = ObjectEntry::new_prototype_function_entry(heap, "abs", 1, false);
     let acos = ObjectEntry::new_prototype_function_entry(heap, "acos", 1, false);
     let acosh = ObjectEntry::new_prototype_function_entry(heap, "acosh", 1, false);
@@ -53,14 +53,14 @@ pub(super) fn initialize_math_object(heap: &mut Heap) {
     let tanh = ObjectEntry::new_prototype_function_entry(heap, "tanh", 1, false);
     let trunc = ObjectEntry::new_prototype_function_entry(heap, "trunc", 1, false);
     let entries = vec![
-        ObjectEntry::new_frozen_entry(heap, "E", e),
-        ObjectEntry::new_frozen_entry(heap, "LN10", ln10),
-        ObjectEntry::new_frozen_entry(heap, "LN2", ln2),
-        ObjectEntry::new_frozen_entry(heap, "LOG10E", log10e),
-        ObjectEntry::new_frozen_entry(heap, "LOG2E", log2e),
-        ObjectEntry::new_frozen_entry(heap, "PI", pi),
-        ObjectEntry::new_frozen_entry(heap, "SQRT1_2", sqrt1_2),
-        ObjectEntry::new_frozen_entry(heap, "SQRT2", sqrt2),
+        ObjectEntry::new_frozen_entry(heap, "E", e.into()),
+        ObjectEntry::new_frozen_entry(heap, "LN10", ln10.into()),
+        ObjectEntry::new_frozen_entry(heap, "LN2", ln2.into()),
+        ObjectEntry::new_frozen_entry(heap, "LOG10E", log10e.into()),
+        ObjectEntry::new_frozen_entry(heap, "LOG2E", log2e.into()),
+        ObjectEntry::new_frozen_entry(heap, "PI", pi.into()),
+        ObjectEntry::new_frozen_entry(heap, "SQRT1_2", sqrt1_2.into()),
+        ObjectEntry::new_frozen_entry(heap, "SQRT2", sqrt2.into()),
         ObjectEntry::new(
             PropertyKey::Symbol(WellKnownSymbolIndexes::ToStringTag.into()),
             PropertyDescriptor::roxh(Value::from_str(heap, "Math")),

diff --git a/nova_vm/src/heap/number.rs b/nova_vm/src/heap/number.rs
@@ -1,40 +1,41 @@
-use super::{object::ObjectEntry, Heap};
+use super::{object::ObjectEntry, CreateHeapData, Heap};
 use crate::{
     ecmascript::{
         execution::JsResult,
-        types::{Object, PropertyKey, Value},
+        types::{Number, Object, PropertyKey, Value},
     },
     heap::{
         heap_constants::{get_constructor_index, BuiltinObjectIndexes},
         FunctionHeapData, PropertyDescriptor,
     },
+    SmallInteger,
 };
 
 pub fn initialize_number_heap(heap: &mut Heap) {
     let entries = vec![
         ObjectEntry::new(
             PropertyKey::from_str(heap, "EPSILON"),
-            PropertyDescriptor::roh(Value::from_f64(heap, f64::EPSILON)),
+            PropertyDescriptor::roh(heap.create(f64::EPSILON).into()),
         ),
         ObjectEntry::new_prototype_function_entry(heap, "isFinite", 1, false),
         ObjectEntry::new_prototype_function_entry(heap, "isInteger", 1, false),
         ObjectEntry::new_prototype_function_entry(heap, "isNan", 1, false),
         ObjectEntry::new_prototype_function_entry(heap, "isSafeInteger", 1, false),
         ObjectEntry::new(
             PropertyKey::from_str(heap, "MAX_SAFE_INTEGER"),
-            PropertyDescriptor::roh(Value::from_f64(heap, 9007199254740991.0)),
+            PropertyDescriptor::roh(Number::try_from(SmallInteger::MAX_NUMBER).unwrap().into()),
         ),
         ObjectEntry::new(
             PropertyKey::from_str(heap, "MAX_VALUE"),
-            PropertyDescriptor::roh(Value::from_f64(heap, f64::MAX)),
+            PropertyDescriptor::roh(heap.create(f64::MAX).into()),
         ),
         ObjectEntry::new(
             PropertyKey::from_str(heap, "MIN_SAFE_INTEGER"),
-            PropertyDescriptor::roh(Value::from_f64(heap, -9007199254740991.0)),
+            PropertyDescriptor::roh(Number::try_from(SmallInteger::MIN_NUMBER).unwrap().into()),
         ),
         ObjectEntry::new(
             PropertyKey::from_str(heap, "MIN_VALUE"),
-            PropertyDescriptor::roh(Value::from_f64(heap, f64::MIN)),
+            PropertyDescriptor::roh(heap.create(f64::MIN).into()),
         ),
         ObjectEntry::new(
             PropertyKey::from_str(heap, "NaN"),