avm1: Fix string methods handling of negative args (fix #4437)

* Improve string.substr handling of negative args
* Add tests for more substr() combinations

core/src/avm1/globals/string.rs

@@ -345,13 +345,19 @@ fn substr<'gc>(
     let this_val = Value::from(this);
     let this = this_val.coerce_to_string(activation)?;
     let this_len = this.encode_utf16().count();
-    let start_index_raw = args.get(0).unwrap().coerce_to_i32(activation)?;
-    let start_index = string_wrapping_index(start_index_raw, this_len);
+    let start_index = string_wrapping_index(
+        args.get(0)
+            .unwrap_or(&Value::Undefined)
+            .coerce_to_i32(activation)?,
+        this_len,
+    );
 
     let len = match args.get(1) {
-        None | Some(Value::Undefined) => this_len,
-        Some(n) => string_index_substr(start_index_raw, n.coerce_to_i32(activation)?, this_len),
+        None | Some(Value::Undefined) => this_len as i32,
+        Some(n) => n.coerce_to_i32(activation)?,
     };
+    let end_index = string_wrapping_index((start_index as i32) + len, this_len);
+    let len = end_index.saturating_sub(start_index);
 
     let ret = string_utils::utf16_iter_to_string(this.encode_utf16().skip(start_index).take(len));
     Ok(AvmString::new(activation.context.gc_context, ret).into())
@@ -441,57 +447,21 @@ fn to_upper_case<'gc>(
 /// Normalizes an  index parameter used in `String` functions such as `substring`.
 /// The returned index will be within the range of `[0, len]`.
 fn string_index(i: i32, len: usize) -> usize {
-    if i > 0 {
-        let i = i as usize;
-        if i < len {
-            i
-        } else {
-            len
-        }
-    } else {
+    if i < 0 {
         0
+    } else {
+        (i as usize).min(len)
     }
 }
 
 /// Normalizes an wrapping index parameter used in `String` functions such as `slice`.
 /// Negative values will count backwards from `len`.
 /// The returned index will be within the range of `[0, len]`.
 fn string_wrapping_index(i: i32, len: usize) -> usize {
-    if i >= 0 {
-        let i = i as usize;
-        if i < len {
-            i
-        } else {
-            len
-        }
+    if i < 0 {
+        let offset = i as isize;
+        len.saturating_sub((-offset) as usize)
     } else {
-        let i = (-i) as usize;
-        if i <= len {
-            len - i
-        } else {
-            len
-        }
-    }
-}
-
-/// Normalizes an index parameter used in substr.
-/// If start + length is not less than zero, the parameter is zero,
-/// otherwise negative values will count backwards from `len`.
-/// The returned index will be within the range of `[0, len]`.
-fn string_index_substr(s: i32, e: i32, len: usize) -> usize {
-    if e >= 0 {
-        string_index(e, len)
-    } else {
-        let t = s + e;
-        if t >= 0 {
-            0
-        } else {
-            let e = (-e) as usize;
-            if e <= len {
-                len - e
-            } else {
-                len
-            }
-        }
+        (i as usize).min(len)
     }
 }

tests/tests/regression_tests.rs

@@ -182,8 +182,8 @@ swf_tests! {
     (slash_syntax, "avm1/slash_syntax", 2),
     (strictequals_swf6, "avm1/strictequals_swf6", 1),
     (string_methods, "avm1/string_methods", 1),
+    (string_methods_negative_args, "avm1/string_methods_negative_args", 1),
     (string_ops_swf6, "avm1/string_ops_swf6", 1),
-    (substr_negative, "avm1/substr_negative", 1),
     (path_string, "avm1/path_string", 1),
     (global_is_bare, "avm1/global_is_bare", 1),
     (primitive_type_globals, "avm1/primitive_type_globals", 1),

tests/tests/swfs/avm1/string_methods_negative_args/Test.as

@@ -0,0 +1,39 @@
+// Compile with:
+//  mtasc -main -header 200:150:30 Test.as -swf test.swf 
+class Test {
+    static function main(current) {
+
+		trace("// var str = hello world");
+		var str = "hello world";
+		trace("// str.substr(0, -1)");
+		trace(str.substr(0,-1));
+		trace("// str.substr(0, -4)");
+		trace(str.substr(0,-4));
+		trace("// str.substr(8, -4)");
+		trace(str.substr(8,-4));
+		trace("// str.substr(3, -3)");
+		trace(str.substr(3,-3));
+		trace("// str.substr(3, -4)");
+		trace(str.substr(3,-4));
+		trace("// str.substr(3, -5)");
+		trace(str.substr(3,-5));
+		trace("// str.substr(4, -4)");
+		trace(str.substr(4,-4));
+		trace("// str.substr(4, -5)");
+		trace(str.substr(4,-5));
+		trace("// str.substr(null, -1)");
+		trace(str.substr(null,-1));
+		trace("// str.substr(undefined, -1)");
+		trace(str.substr(undefined,-1));
+
+		var text = "abcd";
+		for (var i = -5; i < 5; i += 1) {
+			trace("// text.substr(" + i + ")");
+			trace(text.substr(i));
+			for (var j = -5; j < 5; j += 1) {
+				trace("// text.substr(" + i + "," + j + ")");
+				trace(text.substr(i, j));
+			}
+		}
+    }
+}

tests/tests/swfs/avm1/string_methods_negative_args/output.txt

@@ -0,0 +1,241 @@
+// var str = hello world
+// str.substr(0, -1)
+hello worl
+// str.substr(0, -4)
+hello w
+// str.substr(8, -4)
+
+// str.substr(3, -3)
+
+// str.substr(3, -4)
+lo worl
+// str.substr(3, -5)
+lo wor
+// str.substr(4, -4)
+
+// str.substr(4, -5)
+o worl
+// str.substr(null, -1)
+hello worl
+// str.substr(undefined, -1)
+hello worl
+// text.substr(-5)
+abcd
+// text.substr(-5,-5)
+
+// text.substr(-5,-4)
+
+// text.substr(-5,-3)
+a
+// text.substr(-5,-2)
+ab
+// text.substr(-5,-1)
+abc
+// text.substr(-5,0)
+
+// text.substr(-5,1)
+a
+// text.substr(-5,2)
+ab
+// text.substr(-5,3)
+abc
+// text.substr(-5,4)
+abcd
+// text.substr(-4)
+abcd
+// text.substr(-4,-5)
+
+// text.substr(-4,-4)
+
+// text.substr(-4,-3)
+a
+// text.substr(-4,-2)
+ab
+// text.substr(-4,-1)
+abc
+// text.substr(-4,0)
+
+// text.substr(-4,1)
+a
+// text.substr(-4,2)
+ab
+// text.substr(-4,3)
+abc
+// text.substr(-4,4)
+abcd
+// text.substr(-3)
+bcd
+// text.substr(-3,-5)
+
+// text.substr(-3,-4)
+
+// text.substr(-3,-3)
+b
+// text.substr(-3,-2)
+bc
+// text.substr(-3,-1)
+
+// text.substr(-3,0)
+
+// text.substr(-3,1)
+b
+// text.substr(-3,2)
+bc
+// text.substr(-3,3)
+bcd
+// text.substr(-3,4)
+bcd
+// text.substr(-2)
+cd
+// text.substr(-2,-5)
+
+// text.substr(-2,-4)
+
+// text.substr(-2,-3)
+c
+// text.substr(-2,-2)
+
+// text.substr(-2,-1)
+
+// text.substr(-2,0)
+
+// text.substr(-2,1)
+c
+// text.substr(-2,2)
+cd
+// text.substr(-2,3)
+cd
+// text.substr(-2,4)
+cd
+// text.substr(-1)
+d
+// text.substr(-1,-5)
+
+// text.substr(-1,-4)
+
+// text.substr(-1,-3)
+
+// text.substr(-1,-2)
+
+// text.substr(-1,-1)
+
+// text.substr(-1,0)
+
+// text.substr(-1,1)
+d
+// text.substr(-1,2)
+d
+// text.substr(-1,3)
+d
+// text.substr(-1,4)
+d
+// text.substr(0)
+abcd
+// text.substr(0,-5)
+
+// text.substr(0,-4)
+
+// text.substr(0,-3)
+a
+// text.substr(0,-2)
+ab
+// text.substr(0,-1)
+abc
+// text.substr(0,0)
+
+// text.substr(0,1)
+a
+// text.substr(0,2)
+ab
+// text.substr(0,3)
+abc
+// text.substr(0,4)
+abcd
+// text.substr(1)
+bcd
+// text.substr(1,-5)
+
+// text.substr(1,-4)
+
+// text.substr(1,-3)
+b
+// text.substr(1,-2)
+bc
+// text.substr(1,-1)
+
+// text.substr(1,0)
+
+// text.substr(1,1)
+b
+// text.substr(1,2)
+bc
+// text.substr(1,3)
+bcd
+// text.substr(1,4)
+bcd
+// text.substr(2)
+cd
+// text.substr(2,-5)
+
+// text.substr(2,-4)
+
+// text.substr(2,-3)
+c
+// text.substr(2,-2)
+
+// text.substr(2,-1)
+
+// text.substr(2,0)
+
+// text.substr(2,1)
+c
+// text.substr(2,2)
+cd
+// text.substr(2,3)
+cd
+// text.substr(2,4)
+cd
+// text.substr(3)
+d
+// text.substr(3,-5)
+
+// text.substr(3,-4)
+
+// text.substr(3,-3)
+
+// text.substr(3,-2)
+
+// text.substr(3,-1)
+
+// text.substr(3,0)
+
+// text.substr(3,1)
+d
+// text.substr(3,2)
+d
+// text.substr(3,3)
+d
+// text.substr(3,4)
+d
+// text.substr(4)
+
+// text.substr(4,-5)
+
+// text.substr(4,-4)
+
+// text.substr(4,-3)
+
+// text.substr(4,-2)
+
+// text.substr(4,-1)
+
+// text.substr(4,0)
+
+// text.substr(4,1)
+
+// text.substr(4,2)
+
+// text.substr(4,3)
+
+// text.substr(4,4)
+