From f3fb06c91f44523ce38fd9e4dd46cc7b4ee6ebe3 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 7 Feb 2020 13:56:04 -0800 Subject: [PATCH 1/2] Disallow string.Replace with a zero-weight replacement string --- .../Common/tests/Tests/System/StringTests.cs | 1 + .../src/System/String.Manipulation.cs | 7 +++++++ .../System.Runtime/tests/System/StringTests.cs | 15 +++++++++++++++ 3 files changed, 23 insertions(+) diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index 80cee6578e044d..e7cc5b6cd1572a 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -22,6 +22,7 @@ namespace System.Tests public partial class StringTests { private const string SoftHyphen = "\u00AD"; + private const string ZeroWidthJoiner = "\u200D"; // weightless in both ICU and NLS private static readonly char[] s_whiteSpaceCharacters = { '\u0009', '\u000a', '\u000b', '\u000c', '\u000d', '\u0020', '\u0085', '\u00a0', '\u1680' }; [Theory] diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index 8e51249f5e50b7..c31c2b77ec09ec 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1023,6 +1023,13 @@ private unsafe string ReplaceCore(string oldValue, string? newValue, CultureInfo index = ci.IndexOf(this, oldValue, startIndex, this.Length - startIndex, options, &matchLength); if (index >= 0) { + // There's the possibility that 'oldValue' has zero collation weight (empty string equivalent). + // If this is the case, we need to fail immediately so that we don't enter an infinite loop. + if (matchLength == 0) + { + throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue)); + } + // append the unmodified portion of string result.Append(this.AsSpan(startIndex, index - startIndex)); diff --git a/src/libraries/System.Runtime/tests/System/StringTests.cs b/src/libraries/System.Runtime/tests/System/StringTests.cs index 831b7160e5b798..319655aa3e4fef 100644 --- a/src/libraries/System.Runtime/tests/System/StringTests.cs +++ b/src/libraries/System.Runtime/tests/System/StringTests.cs @@ -730,6 +730,21 @@ public void Replace_StringComparison_EmptyOldValue_ThrowsArgumentException() AssertExtensions.Throws("oldValue", () => "abc".Replace("", "def", true, CultureInfo.CurrentCulture)); } + [Fact] + public void Replace_StringComparison_WeightlessOldValue_WithOrdinalComparison_Succeeds() + { + Assert.Equal("abcdef", ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def")); + Assert.Equal("abcdef", ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def", StringComparison.Ordinal)); + Assert.Equal("abcdef", ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def", StringComparison.OrdinalIgnoreCase)); + } + + [Fact] + public void Replace_StringComparison_WeightlessOldValue_WithLinguisticComparison_ThrowsArgumentException() + { + AssertExtensions.Throws("oldValue", () => ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def", StringComparison.CurrentCulture)); + AssertExtensions.Throws("oldValue", () => ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def", true, CultureInfo.CurrentCulture)); + } + [Theory] [InlineData(StringComparison.CurrentCulture - 1)] [InlineData(StringComparison.OrdinalIgnoreCase + 1)] From aa882c423b6f2dc9fafb85dcfdc4196fdd25638d Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Mon, 10 Feb 2020 18:27:37 -0800 Subject: [PATCH 2/2] Don't fail when given zero-weight target string --- .../src/System/String.Manipulation.cs | 13 +++++-------- .../System.Runtime/tests/System/StringTests.cs | 6 +++--- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs index c31c2b77ec09ec..50e01e12eba010 100644 --- a/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs +++ b/src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs @@ -1021,15 +1021,12 @@ private unsafe string ReplaceCore(string oldValue, string? newValue, CultureInfo do { index = ci.IndexOf(this, oldValue, startIndex, this.Length - startIndex, options, &matchLength); - if (index >= 0) - { - // There's the possibility that 'oldValue' has zero collation weight (empty string equivalent). - // If this is the case, we need to fail immediately so that we don't enter an infinite loop. - if (matchLength == 0) - { - throw new ArgumentException(SR.Argument_StringZeroLength, nameof(oldValue)); - } + // There's the possibility that 'oldValue' has zero collation weight (empty string equivalent). + // If this is the case, we behave as if there are no more substitutions to be made. + + if (index >= 0 && matchLength > 0) + { // append the unmodified portion of string result.Append(this.AsSpan(startIndex, index - startIndex)); diff --git a/src/libraries/System.Runtime/tests/System/StringTests.cs b/src/libraries/System.Runtime/tests/System/StringTests.cs index 319655aa3e4fef..a7c4100ae88076 100644 --- a/src/libraries/System.Runtime/tests/System/StringTests.cs +++ b/src/libraries/System.Runtime/tests/System/StringTests.cs @@ -739,10 +739,10 @@ public void Replace_StringComparison_WeightlessOldValue_WithOrdinalComparison_Su } [Fact] - public void Replace_StringComparison_WeightlessOldValue_WithLinguisticComparison_ThrowsArgumentException() + public void Replace_StringComparison_WeightlessOldValue_WithLinguisticComparison_TerminatesReplacement() { - AssertExtensions.Throws("oldValue", () => ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def", StringComparison.CurrentCulture)); - AssertExtensions.Throws("oldValue", () => ("abc" + ZeroWidthJoiner).Replace(ZeroWidthJoiner, "def", true, CultureInfo.CurrentCulture)); + Assert.Equal("abc" + ZeroWidthJoiner + "def", ("abc" + ZeroWidthJoiner + "def").Replace(ZeroWidthJoiner, "xyz", StringComparison.CurrentCulture)); + Assert.Equal("abc" + ZeroWidthJoiner + "def", ("abc" + ZeroWidthJoiner + "def").Replace(ZeroWidthJoiner, "xyz", true, CultureInfo.CurrentCulture)); } [Theory]