diff --git a/core/engine/src/builtins/string/mod.rs b/core/engine/src/builtins/string/mod.rs index 9421b4654d7..6144c4aa10b 100644 --- a/core/engine/src/builtins/string/mod.rs +++ b/core/engine/src/builtins/string/mod.rs @@ -659,8 +659,10 @@ impl String { // 4. For each element next of args, do for arg in args { // a. Let nextString be ? ToString(next). + let next_string = arg.to_string(context)?; // b. Set R to the string-concatenation of R and nextString. - string = js_string!(&string, &arg.to_string(context)?); + string = JsString::concat(string.as_str(), next_string.as_str()) + .map_err(|_| JsNativeError::range().with_message("Invalid string length"))?; } // 5. Return R. @@ -741,7 +743,9 @@ impl String { } // 6. Return the String value that is made from n copies of S appended together. - Ok(JsString::concat_array(&result).into()) + Ok(JsString::concat_array(&result) + .map_err(|_| JsNativeError::range().with_message("Invalid string length"))? + .into()) } /// `String.prototype.slice( beginIndex [, endIndex] )` diff --git a/core/engine/src/builtins/string/tests.rs b/core/engine/src/builtins/string/tests.rs index 69215f75e07..827b4a084b8 100644 --- a/core/engine/src/builtins/string/tests.rs +++ b/core/engine/src/builtins/string/tests.rs @@ -975,3 +975,20 @@ fn match_with_overridden_exec() { js_str!("fake"), )]); } + +#[test] +fn concat_max_length_overflow() { + // Test for issue #4409: Repeated string concatenation should throw RangeError + // instead of causing OOM crash + run_test_actions([TestAction::assert_native_error( + indoc! {r" + var s = '\u1234--synchronized-----'; + for (var i = 0; i < 17; i++) { + s += s; + s += s; + } + "}, + JsNativeErrorKind::Range, + "Invalid string length", + )]); +} diff --git a/core/engine/src/string.rs b/core/engine/src/string.rs index 35c40c8c245..9e960a15375 100644 --- a/core/engine/src/string.rs +++ b/core/engine/src/string.rs @@ -62,10 +62,10 @@ macro_rules! js_string { $crate::string::JsString::from($s) }; ( $x:expr, $y:expr ) => { - $crate::string::JsString::concat($crate::string::JsStr::from($x), $crate::string::JsStr::from($y)) + $crate::string::JsString::concat($crate::string::JsStr::from($x), $crate::string::JsStr::from($y)).expect("string concatenation overflow") }; ( $( $s:expr ),+ ) => { - $crate::string::JsString::concat_array(&[ $( $crate::string::JsStr::from($s) ),+ ]) + $crate::string::JsString::concat_array(&[ $( $crate::string::JsStr::from($s) ),+ ]).expect("string concatenation overflow") }; } diff --git a/core/engine/src/value/operations.rs b/core/engine/src/value/operations.rs index 7300684bc4c..cbebaad1735 100644 --- a/core/engine/src/value/operations.rs +++ b/core/engine/src/value/operations.rs @@ -1,11 +1,10 @@ use crate::{ - Context, JsBigInt, JsResult, JsValue, JsVariant, + Context, JsBigInt, JsResult, JsString, JsValue, JsVariant, builtins::{ Number, number::{f64_to_int32, f64_to_uint32}, }, error::JsNativeError, - js_string, value::{JsSymbol, Numeric, PreferredType}, }; @@ -24,15 +23,33 @@ impl JsValue { (JsVariant::BigInt(x), JsVariant::BigInt(y)) => Self::new(JsBigInt::add(&x, &y)), // String concat - (JsVariant::String(x), JsVariant::String(y)) => Self::from(js_string!(&x, &y)), + (JsVariant::String(x), JsVariant::String(y)) => { + let result = JsString::concat(x.as_str(), y.as_str()) + .map_err(|_| JsNativeError::range().with_message("Invalid string length"))?; + Self::from(result) + } // Slow path: (_, _) => { let x = self.to_primitive(context, PreferredType::Default)?; let y = other.to_primitive(context, PreferredType::Default)?; match (x.variant(), y.variant()) { - (JsVariant::String(x), _) => Self::from(js_string!(&x, &y.to_string(context)?)), - (_, JsVariant::String(y)) => Self::from(js_string!(&x.to_string(context)?, &y)), + (JsVariant::String(x), _) => { + let y_str = y.to_string(context)?; + let result = + JsString::concat(x.as_str(), y_str.as_str()).map_err(|_| { + JsNativeError::range().with_message("Invalid string length") + })?; + Self::from(result) + } + (_, JsVariant::String(y)) => { + let x_str = x.to_string(context)?; + let result = + JsString::concat(x_str.as_str(), y.as_str()).map_err(|_| { + JsNativeError::range().with_message("Invalid string length") + })?; + Self::from(result) + } (_, _) => { match (x.to_numeric(context)?, y.to_numeric(context)?) { (Numeric::Number(x), Numeric::Number(y)) => Self::new(x + y), diff --git a/core/engine/src/vm/opcode/concat/mod.rs b/core/engine/src/vm/opcode/concat/mod.rs index eee34e20fa4..7b8051923e1 100644 --- a/core/engine/src/vm/opcode/concat/mod.rs +++ b/core/engine/src/vm/opcode/concat/mod.rs @@ -21,7 +21,10 @@ impl ConcatToString { let val = context.vm.get_register(value.into()).clone(); strings.push(val.to_string(context)?); } - let s = JsString::concat_array(&strings.iter().map(JsString::as_str).collect::>()); + let s = JsString::concat_array(&strings.iter().map(JsString::as_str).collect::>()) + .map_err(|_| { + crate::error::JsNativeError::range().with_message("Invalid string length") + })?; context.vm.set_register(string.into(), s.into()); Ok(()) } diff --git a/core/string/src/lib.rs b/core/string/src/lib.rs index 507a1e4b921..8d5173b3deb 100644 --- a/core/string/src/lib.rs +++ b/core/string/src/lib.rs @@ -48,6 +48,10 @@ use std::{ }; use vtable::JsStringVTable; +/// Maximum string length allowed (`u32::MAX`). +/// This prevents OOM crashes from exponential string growth during concatenation. +pub const MAX_STRING_LENGTH: usize = u32::MAX as usize; + fn alloc_overflow() -> ! { panic!("detected overflow during string allocation") } @@ -628,29 +632,45 @@ impl JsString { } /// Creates a new [`JsString`] from the concatenation of `x` and `y`. + /// + /// # Errors + /// + /// Returns an error if the resulting string would exceed [`MAX_STRING_LENGTH`]. #[inline] - #[must_use] - pub fn concat(x: JsStr<'_>, y: JsStr<'_>) -> Self { + pub fn concat(x: JsStr<'_>, y: JsStr<'_>) -> Result { Self::concat_array(&[x, y]) } /// Creates a new [`JsString`] from the concatenation of every element of /// `strings`. + /// + /// # Errors + /// + /// Returns an error if the resulting string would exceed [`MAX_STRING_LENGTH`]. #[inline] - #[must_use] - pub fn concat_array(strings: &[JsStr<'_>]) -> Self { + pub fn concat_array(strings: &[JsStr<'_>]) -> Result { let mut latin1_encoding = true; let mut full_count = 0usize; for string in strings { let Some(sum) = full_count.checked_add(string.len()) else { - alloc_overflow() + return Err("Invalid string length"); }; + // Check if the resulting string would exceed the maximum length + if sum > MAX_STRING_LENGTH { + return Err("Invalid string length"); + } if !string.is_latin1() { latin1_encoding = false; } full_count = sum; } + // For UTF-16 strings, also check that the byte count doesn't overflow usize + // (each character is 2 bytes). This is important for 32-bit systems. + if !latin1_encoding && full_count.checked_mul(2).is_none() { + return Err("Invalid string length"); + } + let (ptr, data_offset) = if latin1_encoding { let p = SequenceString::::allocate(full_count); (p.cast::(), size_of::>()) @@ -707,7 +727,7 @@ impl JsString { Self { ptr: ptr.cast() } }; - StaticJsStrings::get_string(&string.as_str()).unwrap_or(string) + Ok(StaticJsStrings::get_string(&string.as_str()).unwrap_or(string)) } /// Creates a new [`JsString`] from `data`, without checking if the string is in the interner. @@ -853,6 +873,7 @@ impl From<&[JsString]> for JsString { #[inline] fn from(value: &[JsString]) -> Self { Self::concat_array(&value.iter().map(Self::as_str).collect::>()[..]) + .expect("string concatenation overflow") } } @@ -860,6 +881,7 @@ impl From<&[JsString; N]> for JsString { #[inline] fn from(value: &[JsString; N]) -> Self { Self::concat_array(&value.iter().map(Self::as_str).collect::>()[..]) + .expect("string concatenation overflow") } } diff --git a/core/string/src/tests.rs b/core/string/src/tests.rs index 43d0d3268d9..d3b4fa539f3 100644 --- a/core/string/src/tests.rs +++ b/core/string/src/tests.rs @@ -127,15 +127,15 @@ fn concat() { let x = JsString::from("hello"); let z = JsString::from("world"); - let xy = JsString::concat(x.as_str(), JsString::from(Y).as_str()); + let xy = JsString::concat(x.as_str(), JsString::from(Y).as_str()).unwrap(); assert_eq!(&xy, &ascii_to_utf16(b"hello, ")); assert_eq!(xy.refcount(), Some(1)); - let xyz = JsString::concat(xy.as_str(), z.as_str()); + let xyz = JsString::concat(xy.as_str(), z.as_str()).unwrap(); assert_eq!(&xyz, &ascii_to_utf16(b"hello, world")); assert_eq!(xyz.refcount(), Some(1)); - let xyzw = JsString::concat(xyz.as_str(), JsString::from(W).as_str()); + let xyzw = JsString::concat(xyz.as_str(), JsString::from(W).as_str()).unwrap(); assert_eq!(&xyzw, &ascii_to_utf16(b"hello, world!")); assert_eq!(xyzw.refcount(), Some(1)); }