From cbc39f44d215586927a113b3d4637bef5d4a5127 Mon Sep 17 00:00:00 2001 From: Abhinav Sharma Date: Sat, 28 Mar 2026 16:36:55 +0530 Subject: [PATCH 1/3] fix(string): add maximum length limit to JsString to prevent OOM crashes Signed-off-by: Abhinav Sharma --- core/engine/src/builtins/string/mod.rs | 8 +++++-- core/engine/src/builtins/string/tests.rs | 17 ++++++++++++++ core/engine/src/string.rs | 4 ++-- core/engine/src/value/operations.rs | 23 ++++++++++++++----- core/engine/src/vm/opcode/concat/mod.rs | 5 ++++- core/string/src/lib.rs | 28 +++++++++++++++++++----- core/string/src/tests.rs | 6 ++--- 7 files changed, 72 insertions(+), 19 deletions(-) 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..6d1dceedaaa 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,29 @@ 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..71576f2bf45 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..c8bfdbe930f 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,23 +632,33 @@ 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; } @@ -707,7 +721,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 +867,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 +875,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)); } From 7fcdccc46dd63e9a258bb246f1ed9bc29e5e4cc5 Mon Sep 17 00:00:00 2001 From: Abhinav Sharma Date: Sat, 28 Mar 2026 17:58:18 +0530 Subject: [PATCH 2/3] fix: lint and formatting issues Signed-off-by: Abhinav Sharma --- core/engine/src/value/operations.rs | 12 ++++++++---- core/engine/src/vm/opcode/concat/mod.rs | 4 ++-- core/string/src/lib.rs | 2 +- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/core/engine/src/value/operations.rs b/core/engine/src/value/operations.rs index 6d1dceedaaa..cbebaad1735 100644 --- a/core/engine/src/value/operations.rs +++ b/core/engine/src/value/operations.rs @@ -36,14 +36,18 @@ impl JsValue { match (x.variant(), y.variant()) { (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"))?; + 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"))?; + let result = + JsString::concat(x_str.as_str(), y.as_str()).map_err(|_| { + JsNativeError::range().with_message("Invalid string length") + })?; Self::from(result) } (_, _) => { diff --git a/core/engine/src/vm/opcode/concat/mod.rs b/core/engine/src/vm/opcode/concat/mod.rs index 71576f2bf45..7b8051923e1 100644 --- a/core/engine/src/vm/opcode/concat/mod.rs +++ b/core/engine/src/vm/opcode/concat/mod.rs @@ -23,8 +23,8 @@ impl ConcatToString { } let s = JsString::concat_array(&strings.iter().map(JsString::as_str).collect::>()) .map_err(|_| { - crate::error::JsNativeError::range().with_message("Invalid string length") - })?; + 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 c8bfdbe930f..49766ed61c4 100644 --- a/core/string/src/lib.rs +++ b/core/string/src/lib.rs @@ -48,7 +48,7 @@ use std::{ }; use vtable::JsStringVTable; -/// Maximum string length allowed (u32::MAX). +/// 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; From 58dec123961fd75590d99977a58dc74841faa074 Mon Sep 17 00:00:00 2001 From: Abhinav Sharma Date: Sat, 28 Mar 2026 18:45:45 +0530 Subject: [PATCH 3/3] fix: extra check for 32 bit systems Signed-off-by: Abhinav Sharma --- core/string/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/core/string/src/lib.rs b/core/string/src/lib.rs index 49766ed61c4..8d5173b3deb 100644 --- a/core/string/src/lib.rs +++ b/core/string/src/lib.rs @@ -665,6 +665,12 @@ impl JsString { 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::>())