Skip to content

Commit cbc39f4

Browse files
abhinavs1920abhinavs1920
committed
fix(string): add maximum length limit to JsString to prevent OOM crashes
Signed-off-by: Abhinav Sharma <abhinavs1920bpl@gmail.com>
1 parent f075094 commit cbc39f4

File tree

7 files changed

+72
-19
lines changed

7 files changed

+72
-19
lines changed

‎core/engine/src/builtins/string/mod.rs‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -659,8 +659,10 @@ impl String {
659659
// 4. For each element next of args, do
660660
for arg in args {
661661
// a. Let nextString be ? ToString(next).
662+
let next_string = arg.to_string(context)?;
662663
// b. Set R to the string-concatenation of R and nextString.
663-
string = js_string!(&string, &arg.to_string(context)?);
664+
string = JsString::concat(string.as_str(), next_string.as_str())
665+
.map_err(|_| JsNativeError::range().with_message("Invalid string length"))?;
664666
}
665667

666668
// 5. Return R.
@@ -741,7 +743,9 @@ impl String {
741743
}
742744

743745
// 6. Return the String value that is made from n copies of S appended together.
744-
Ok(JsString::concat_array(&result).into())
746+
Ok(JsString::concat_array(&result)
747+
.map_err(|_| JsNativeError::range().with_message("Invalid string length"))?
748+
.into())
745749
}
746750

747751
/// `String.prototype.slice( beginIndex [, endIndex] )`

‎core/engine/src/builtins/string/tests.rs‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -975,3 +975,20 @@ fn match_with_overridden_exec() {
975975
js_str!("fake"),
976976
)]);
977977
}
978+
979+
#[test]
980+
fn concat_max_length_overflow() {
981+
// Test for issue #4409: Repeated string concatenation should throw RangeError
982+
// instead of causing OOM crash
983+
run_test_actions([TestAction::assert_native_error(
984+
indoc! {r"
985+
var s = '\u1234--synchronized-----';
986+
for (var i = 0; i < 17; i++) {
987+
s += s;
988+
s += s;
989+
}
990+
"},
991+
JsNativeErrorKind::Range,
992+
"Invalid string length",
993+
)]);
994+
}

‎core/engine/src/string.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,10 +62,10 @@ macro_rules! js_string {
6262
$crate::string::JsString::from($s)
6363
};
6464
( $x:expr, $y:expr ) => {
65-
$crate::string::JsString::concat($crate::string::JsStr::from($x), $crate::string::JsStr::from($y))
65+
$crate::string::JsString::concat($crate::string::JsStr::from($x), $crate::string::JsStr::from($y)).expect("string concatenation overflow")
6666
};
6767
( $( $s:expr ),+ ) => {
68-
$crate::string::JsString::concat_array(&[ $( $crate::string::JsStr::from($s) ),+ ])
68+
$crate::string::JsString::concat_array(&[ $( $crate::string::JsStr::from($s) ),+ ]).expect("string concatenation overflow")
6969
};
7070
}
7171

‎core/engine/src/value/operations.rs‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,10 @@
11
use crate::{
2-
Context, JsBigInt, JsResult, JsValue, JsVariant,
2+
Context, JsBigInt, JsResult, JsString, JsValue, JsVariant,
33
builtins::{
44
Number,
55
number::{f64_to_int32, f64_to_uint32},
66
},
77
error::JsNativeError,
8-
js_string,
98
value::{JsSymbol, Numeric, PreferredType},
109
};
1110

@@ -24,15 +23,29 @@ impl JsValue {
2423
(JsVariant::BigInt(x), JsVariant::BigInt(y)) => Self::new(JsBigInt::add(&x, &y)),
2524

2625
// String concat
27-
(JsVariant::String(x), JsVariant::String(y)) => Self::from(js_string!(&x, &y)),
26+
(JsVariant::String(x), JsVariant::String(y)) => {
27+
let result = JsString::concat(x.as_str(), y.as_str())
28+
.map_err(|_| JsNativeError::range().with_message("Invalid string length"))?;
29+
Self::from(result)
30+
}
2831

2932
// Slow path:
3033
(_, _) => {
3134
let x = self.to_primitive(context, PreferredType::Default)?;
3235
let y = other.to_primitive(context, PreferredType::Default)?;
3336
match (x.variant(), y.variant()) {
34-
(JsVariant::String(x), _) => Self::from(js_string!(&x, &y.to_string(context)?)),
35-
(_, JsVariant::String(y)) => Self::from(js_string!(&x.to_string(context)?, &y)),
37+
(JsVariant::String(x), _) => {
38+
let y_str = y.to_string(context)?;
39+
let result = JsString::concat(x.as_str(), y_str.as_str())
40+
.map_err(|_| JsNativeError::range().with_message("Invalid string length"))?;
41+
Self::from(result)
42+
}
43+
(_, JsVariant::String(y)) => {
44+
let x_str = x.to_string(context)?;
45+
let result = JsString::concat(x_str.as_str(), y.as_str())
46+
.map_err(|_| JsNativeError::range().with_message("Invalid string length"))?;
47+
Self::from(result)
48+
}
3649
(_, _) => {
3750
match (x.to_numeric(context)?, y.to_numeric(context)?) {
3851
(Numeric::Number(x), Numeric::Number(y)) => Self::new(x + y),

‎core/engine/src/vm/opcode/concat/mod.rs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,10 @@ impl ConcatToString {
2121
let val = context.vm.get_register(value.into()).clone();
2222
strings.push(val.to_string(context)?);
2323
}
24-
let s = JsString::concat_array(&strings.iter().map(JsString::as_str).collect::<Vec<_>>());
24+
let s = JsString::concat_array(&strings.iter().map(JsString::as_str).collect::<Vec<_>>())
25+
.map_err(|_| {
26+
crate::error::JsNativeError::range().with_message("Invalid string length")
27+
})?;
2528
context.vm.set_register(string.into(), s.into());
2629
Ok(())
2730
}

‎core/string/src/lib.rs‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,10 @@ use std::{
4848
};
4949
use vtable::JsStringVTable;
5050

51+
/// Maximum string length allowed (u32::MAX).
52+
/// This prevents OOM crashes from exponential string growth during concatenation.
53+
pub const MAX_STRING_LENGTH: usize = u32::MAX as usize;
54+
5155
fn alloc_overflow() -> ! {
5256
panic!("detected overflow during string allocation")
5357
}
@@ -628,23 +632,33 @@ impl JsString {
628632
}
629633

630634
/// Creates a new [`JsString`] from the concatenation of `x` and `y`.
635+
///
636+
/// # Errors
637+
///
638+
/// Returns an error if the resulting string would exceed [`MAX_STRING_LENGTH`].
631639
#[inline]
632-
#[must_use]
633-
pub fn concat(x: JsStr<'_>, y: JsStr<'_>) -> Self {
640+
pub fn concat(x: JsStr<'_>, y: JsStr<'_>) -> Result<Self, &'static str> {
634641
Self::concat_array(&[x, y])
635642
}
636643

637644
/// Creates a new [`JsString`] from the concatenation of every element of
638645
/// `strings`.
646+
///
647+
/// # Errors
648+
///
649+
/// Returns an error if the resulting string would exceed [`MAX_STRING_LENGTH`].
639650
#[inline]
640-
#[must_use]
641-
pub fn concat_array(strings: &[JsStr<'_>]) -> Self {
651+
pub fn concat_array(strings: &[JsStr<'_>]) -> Result<Self, &'static str> {
642652
let mut latin1_encoding = true;
643653
let mut full_count = 0usize;
644654
for string in strings {
645655
let Some(sum) = full_count.checked_add(string.len()) else {
646-
alloc_overflow()
656+
return Err("Invalid string length");
647657
};
658+
// Check if the resulting string would exceed the maximum length
659+
if sum > MAX_STRING_LENGTH {
660+
return Err("Invalid string length");
661+
}
648662
if !string.is_latin1() {
649663
latin1_encoding = false;
650664
}
@@ -707,7 +721,7 @@ impl JsString {
707721
Self { ptr: ptr.cast() }
708722
};
709723

710-
StaticJsStrings::get_string(&string.as_str()).unwrap_or(string)
724+
Ok(StaticJsStrings::get_string(&string.as_str()).unwrap_or(string))
711725
}
712726

713727
/// Creates a new [`JsString`] from `data`, without checking if the string is in the interner.
@@ -853,13 +867,15 @@ impl From<&[JsString]> for JsString {
853867
#[inline]
854868
fn from(value: &[JsString]) -> Self {
855869
Self::concat_array(&value.iter().map(Self::as_str).collect::<Vec<_>>()[..])
870+
.expect("string concatenation overflow")
856871
}
857872
}
858873

859874
impl<const N: usize> From<&[JsString; N]> for JsString {
860875
#[inline]
861876
fn from(value: &[JsString; N]) -> Self {
862877
Self::concat_array(&value.iter().map(Self::as_str).collect::<Vec<_>>()[..])
878+
.expect("string concatenation overflow")
863879
}
864880
}
865881

‎core/string/src/tests.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -127,15 +127,15 @@ fn concat() {
127127
let x = JsString::from("hello");
128128
let z = JsString::from("world");
129129

130-
let xy = JsString::concat(x.as_str(), JsString::from(Y).as_str());
130+
let xy = JsString::concat(x.as_str(), JsString::from(Y).as_str()).unwrap();
131131
assert_eq!(&xy, &ascii_to_utf16(b"hello, "));
132132
assert_eq!(xy.refcount(), Some(1));
133133

134-
let xyz = JsString::concat(xy.as_str(), z.as_str());
134+
let xyz = JsString::concat(xy.as_str(), z.as_str()).unwrap();
135135
assert_eq!(&xyz, &ascii_to_utf16(b"hello, world"));
136136
assert_eq!(xyz.refcount(), Some(1));
137137

138-
let xyzw = JsString::concat(xyz.as_str(), JsString::from(W).as_str());
138+
let xyzw = JsString::concat(xyz.as_str(), JsString::from(W).as_str()).unwrap();
139139
assert_eq!(&xyzw, &ascii_to_utf16(b"hello, world!"));
140140
assert_eq!(xyzw.refcount(), Some(1));
141141
}

0 commit comments

Comments
 (0)