From 6dd17d6ec49e3a24c3344a8854850f58a0b18087 Mon Sep 17 00:00:00 2001 From: Stephen Adams Date: Sun, 19 Jun 2022 17:02:41 +0000 Subject: [PATCH] [runtime] Avoid SIGSEGV on String.* MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Correctness changes: Fix SEGV by checking for overflow in computing the length of the repeated string. Performance changes: Use unnested loops to fill the string instead of nested loops. Implement `operator *` for _TwoByteString in the same way. (The default implementation using StringBuffer causes a lot of allocations. For example, `"α" * 10000` repeatedly adds the small string to the StringBuffer, which repeatedly compresses a sequence of small strings to make a bigger string to stop the list of parts becoming too large. This compression creates a lot of small strings with the same contents.) Bug: 49289 Change-Id: I06c3d91b531d7e4fffc8de9f3bada3eb62ad185f Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/249122 Reviewed-by: Lasse Nielsen Commit-Queue: Stephen Adams --- sdk/lib/_internal/vm/lib/string_patch.dart | 55 ++++++++++++++++--- .../string_operator_multiply_test.dart | 50 +++++++++++++++++ .../string_operator_multiply_test.dart | 50 +++++++++++++++++ 3 files changed, 148 insertions(+), 7 deletions(-) create mode 100644 tests/corelib/string_operator_multiply_test.dart create mode 100644 tests/corelib_2/string_operator_multiply_test.dart diff --git a/sdk/lib/_internal/vm/lib/string_patch.dart b/sdk/lib/_internal/vm/lib/string_patch.dart index 0a51f33c97f..8e353de9143 100644 --- a/sdk/lib/_internal/vm/lib/string_patch.dart +++ b/sdk/lib/_internal/vm/lib/string_patch.dart @@ -964,6 +964,26 @@ abstract class _StringBase implements String { external static String _concatRangeNative(List strings, int start, int end); } +/// Product of two positive integers, clamped to the maximum int value on +/// overflow or non-positive inputs. +int _clampedPositiveProduct(int a, int b) { + const MAX_INT64 = (-1) >>> 1; + + int product = a * b; + + // `(a | b)` is negative if either is negative. + // `product <= 0` if `a` or `b` is zero, and in some cases of overflow. + if ((a | b) < 0 || product <= 0) return MAX_INT64; + + // Both values are small enough that the product has no overflow. + if ((a | b) < (1 << 30)) return product; + + // Check the product. + if (product ~/ a != b) return MAX_INT64; + + return product; +} + @pragma("vm:entry-point") class _OneByteString extends _StringBase { factory _OneByteString._uninstantiable() { @@ -1094,14 +1114,17 @@ class _OneByteString extends _StringBase { String operator *(int times) { if (times <= 0) return ""; if (times == 1) return this; - int length = this.length; if (this.isEmpty) return this; // Don't clone empty string. - _OneByteString result = _OneByteString._allocate(length * times); - int index = 0; - for (int i = 0; i < times; i++) { - for (int j = 0; j < length; j++) { - result._setAt(index++, this.codeUnitAt(j)); - } + int length = this.length; + int resultLength = _clampedPositiveProduct(length, times); + _OneByteString result = _OneByteString._allocate(resultLength); + // Copy `this` into `result`. + for (int i = 0; i < length; i++) { + result._setAt(i, this.codeUnitAt(i)); + } + // Make more copies by copying within `result`. + for (int i = length; i < resultLength; i++) { + result._setAt(i, result.codeUnitAt(i - length)); } return result; } @@ -1341,6 +1364,24 @@ class _TwoByteString extends _StringBase { bool operator ==(Object other) { return super == other; } + + String operator *(int times) { + if (times <= 0) return ""; + if (times == 1) return this; + if (this.isEmpty) return this; // Don't clone empty string. + int length = this.length; + int resultLength = _clampedPositiveProduct(length, times); + _TwoByteString result = _TwoByteString._allocate(resultLength); + // Copy `this` into `result`. + for (int i = 0; i < length; i++) { + result._setAt(i, this.codeUnitAt(i)); + } + // Make more copies by copying within `result`. + for (int i = length; i < resultLength; i++) { + result._setAt(i, result.codeUnitAt(i - length)); + } + return result; + } } @pragma("vm:entry-point") diff --git a/tests/corelib/string_operator_multiply_test.dart b/tests/corelib/string_operator_multiply_test.dart new file mode 100644 index 00000000000..0dc20f89ae8 --- /dev/null +++ b/tests/corelib/string_operator_multiply_test.dart @@ -0,0 +1,50 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import "package:expect/expect.dart"; + +main() { + Expect.equals('', 'a' * -11); + Expect.equals('', 'α' * -11); + Expect.equals('', '∀' * -11); + + Expect.equals('', 'a' * 0); + Expect.equals('', 'α' * 0); + Expect.equals('', '∀' * 0); + + Expect.equals('a', 'a' * 1); + Expect.equals('α', 'α' * 1); + Expect.equals('∀', '∀' * 1); + + Expect.equals('aa', 'a' * 2); + Expect.equals('αα', 'α' * 2); + Expect.equals('∀∀', '∀' * 2); + + Expect.equals('aaa', 'a' * 3); + Expect.equals('ααα', 'α' * 3); + Expect.equals('∀∀∀', '∀' * 3); + + Expect.equals('', '' * 0x4000000000000000); + + Expect.throws(() => 'a' * 0x4000000000000000); + Expect.throws(() => 'α' * 0x4000000000000000); + Expect.throws(() => '∀' * 0x4000000000000000); + + for (final string in ['a', 'α', '∀', 'hello world', 'abc', 'α∀α']) { + for (final count in [0, 1, 10, 100, 255, 256, 257, 1000, 100000]) { + final expected = List.filled(count, string).join(); + final actual = string * count; + Expect.equals(expected, actual); + } + } + + // http://dartbug.com/49289 + Expect.throws(() => 'abcd' * 0x4000000000000000); + Expect.throws(() => 'αxyz' * 0x4000000000000000); + Expect.throws(() => '∀pqr' * 0x4000000000000000); + + Expect.throws(() => 'abcd' * (0x4000000000000000 + 1)); + Expect.throws(() => 'αxyz' * (0x4000000000000000 + 1)); + Expect.throws(() => '∀pqr' * (0x4000000000000000 + 1)); +} diff --git a/tests/corelib_2/string_operator_multiply_test.dart b/tests/corelib_2/string_operator_multiply_test.dart new file mode 100644 index 00000000000..0dc20f89ae8 --- /dev/null +++ b/tests/corelib_2/string_operator_multiply_test.dart @@ -0,0 +1,50 @@ +// Copyright (c) 2022, the Dart project authors. Please see the AUTHORS file +// for details. All rights reserved. Use of this source code is governed by a +// BSD-style license that can be found in the LICENSE file. + +import "package:expect/expect.dart"; + +main() { + Expect.equals('', 'a' * -11); + Expect.equals('', 'α' * -11); + Expect.equals('', '∀' * -11); + + Expect.equals('', 'a' * 0); + Expect.equals('', 'α' * 0); + Expect.equals('', '∀' * 0); + + Expect.equals('a', 'a' * 1); + Expect.equals('α', 'α' * 1); + Expect.equals('∀', '∀' * 1); + + Expect.equals('aa', 'a' * 2); + Expect.equals('αα', 'α' * 2); + Expect.equals('∀∀', '∀' * 2); + + Expect.equals('aaa', 'a' * 3); + Expect.equals('ααα', 'α' * 3); + Expect.equals('∀∀∀', '∀' * 3); + + Expect.equals('', '' * 0x4000000000000000); + + Expect.throws(() => 'a' * 0x4000000000000000); + Expect.throws(() => 'α' * 0x4000000000000000); + Expect.throws(() => '∀' * 0x4000000000000000); + + for (final string in ['a', 'α', '∀', 'hello world', 'abc', 'α∀α']) { + for (final count in [0, 1, 10, 100, 255, 256, 257, 1000, 100000]) { + final expected = List.filled(count, string).join(); + final actual = string * count; + Expect.equals(expected, actual); + } + } + + // http://dartbug.com/49289 + Expect.throws(() => 'abcd' * 0x4000000000000000); + Expect.throws(() => 'αxyz' * 0x4000000000000000); + Expect.throws(() => '∀pqr' * 0x4000000000000000); + + Expect.throws(() => 'abcd' * (0x4000000000000000 + 1)); + Expect.throws(() => 'αxyz' * (0x4000000000000000 + 1)); + Expect.throws(() => '∀pqr' * (0x4000000000000000 + 1)); +}