Skip to content

Commit 6766eb7

Browse files
GrabYourPitchforksGrabYourPitchforks
authored
Fix incorrect early exit in SortKey.Compare and seal type (#31779)
Also addresses some erroneous parameter checking in GetHashCode and fixes endianness issues in InvariantCreateSortKey
1 parent 0d607a7 commit 6766eb7

File tree

8 files changed

+104
-79
lines changed

8 files changed

+104
-79
lines changed

‎src/libraries/System.Globalization/tests/CompareInfo/CompareInfoTests.cs‎

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,7 @@ public void GetHashCode_Invalid()
7171
{
7272
AssertExtensions.Throws<ArgumentNullException>("source", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode(null, CompareOptions.None));
7373

74-
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test", CompareOptions.StringSort));
74+
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test", CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreCase));
7575
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test", CompareOptions.Ordinal | CompareOptions.IgnoreSymbols));
7676
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test", (CompareOptions)(-1)));
7777
}
@@ -182,9 +182,15 @@ public static IEnumerable<object[]> SortKey_TestData()
182182
yield return new object[] { s_invariantCompare, "\u30FC", "\u2010", ignoreKanaIgnoreWidthIgnoreCase, 1 };
183183

184184
yield return new object[] { s_invariantCompare, "/", "\uFF0F", ignoreKanaIgnoreWidthIgnoreCase, 0 };
185-
yield return new object[] { s_invariantCompare, "'", "\uFF07", ignoreKanaIgnoreWidthIgnoreCase, PlatformDetection.IsWindows7 ? -1 : 0};
186185
yield return new object[] { s_invariantCompare, "\"", "\uFF02", ignoreKanaIgnoreWidthIgnoreCase, 0 };
187186

187+
if (!PlatformDetection.IsWindows7)
188+
{
189+
// For the below string, LCMapStringEx and CompareStringEx on Windows 7 return inconsistent results.
190+
// We'll only run this test case on Win8+ or on non-Windows machines.
191+
yield return new object[] { s_invariantCompare, "'", "\uFF07", ignoreKanaIgnoreWidthIgnoreCase, 0 };
192+
}
193+
188194
yield return new object[] { s_invariantCompare, "\u3042", "\u30A1", CompareOptions.None, s_expectedHiraganaToKatakanaCompare };
189195
yield return new object[] { s_invariantCompare, "\u3042", "\u30A2", CompareOptions.None, s_expectedHiraganaToKatakanaCompare };
190196
yield return new object[] { s_invariantCompare, "\u3042", "\uFF71", CompareOptions.None, s_expectedHiraganaToKatakanaCompare };
@@ -349,12 +355,18 @@ public void SortKeyKanaTest(CompareInfo compareInfo, string string1, string stri
349355

350356
[Theory]
351357
[MemberData(nameof(SortKey_TestData))]
352-
public void SortKeyTest(CompareInfo compareInfo, string string1, string string2, CompareOptions options, int expected)
358+
public void SortKeyTest(CompareInfo compareInfo, string string1, string string2, CompareOptions options, int expectedSign)
353359
{
354360
SortKey sk1 = compareInfo.GetSortKey(string1, options);
355361
SortKey sk2 = compareInfo.GetSortKey(string2, options);
356362

357-
Assert.Equal(expected, SortKey.Compare(sk1, sk2));
363+
Assert.Equal(expectedSign, Math.Sign(SortKey.Compare(sk1, sk2)));
364+
Assert.Equal(expectedSign == 0, sk1.Equals(sk2));
365+
Assert.Equal(Math.Sign(compareInfo.Compare(string1, string2, options)), Math.Sign(SortKey.Compare(sk1, sk2)));
366+
367+
Assert.Equal(compareInfo.GetHashCode(string1, options), sk1.GetHashCode());
368+
Assert.Equal(compareInfo.GetHashCode(string2, options), sk2.GetHashCode());
369+
358370
Assert.Equal(string1, sk1.OriginalString);
359371
Assert.Equal(string2, sk2.OriginalString);
360372
}
@@ -389,6 +401,9 @@ public void SortKeyMiscTest()
389401
Assert.Equal(sk4.GetHashCode(), sk5.GetHashCode());
390402
Assert.Equal(sk4.KeyData, sk5.KeyData);
391403

404+
Assert.False(sk1.Equals(null));
405+
Assert.True(sk1.Equals(sk1));
406+
392407
AssertExtensions.Throws<ArgumentNullException>("source", () => ci.GetSortKey(null));
393408
AssertExtensions.Throws<ArgumentException>("options", () => ci.GetSortKey(s1, CompareOptions.Ordinal));
394409
}
@@ -462,7 +477,7 @@ public void GetHashCode_NullAndEmptySpan()
462477
[Fact]
463478
public void GetHashCode_Span_Invalid()
464479
{
465-
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test".AsSpan(), CompareOptions.StringSort));
480+
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test".AsSpan(), CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreCase));
466481
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test".AsSpan(), CompareOptions.Ordinal | CompareOptions.IgnoreSymbols));
467482
AssertExtensions.Throws<ArgumentException>("options", () => CultureInfo.InvariantCulture.CompareInfo.GetHashCode("Test".AsSpan(), (CompareOptions)(-1)));
468483
}

‎src/libraries/System.Globalization/tests/Invariant/InvariantMode.cs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -639,6 +639,25 @@ public void TestSortKey_ZeroWeightCodePoints()
639639
Assert.NotEqual(0, SortKey.Compare(sortKeyForEmptyString, sortKeyForZeroWidthJoiner));
640640
}
641641

642+
[Theory]
643+
[InlineData("", "", 0)]
644+
[InlineData("", "not-empty", -1)]
645+
[InlineData("not-empty", "", 1)]
646+
[InlineData("hello", "hello", 0)]
647+
[InlineData("prefix", "prefix-with-more-data", -1)]
648+
[InlineData("prefix-with-more-data", "prefix", 1)]
649+
[InlineData("e", "\u0115", -1)] // U+0115 = LATIN SMALL LETTER E WITH BREVE, tests endianness handling
650+
public void TestSortKey_Compare_And_Equals(string value1, string value2, int expectedSign)
651+
{
652+
// These tests are in the "invariant" unit test project because we rely on Invariant mode
653+
// copying the input data directly into the sort key.
654+
655+
SortKey sortKey1 = CultureInfo.InvariantCulture.CompareInfo.GetSortKey(value1);
656+
SortKey sortKey2 = CultureInfo.InvariantCulture.CompareInfo.GetSortKey(value2);
657+
658+
Assert.Equal(expectedSign, Math.Sign(SortKey.Compare(sortKey1, sortKey2)));
659+
Assert.Equal(expectedSign == 0, sortKey1.Equals(sortKey2));
660+
}
642661

643662
private static StringComparison GetStringComparison(CompareOptions options)
644663
{

‎src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Invariant.cs‎

Lines changed: 48 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5+
using System.Buffers.Binary;
56
using System.Diagnostics;
67
using System.Runtime.InteropServices;
78

@@ -85,19 +86,19 @@ private static unsafe int InvariantFindString(char* source, int sourceCount, cha
8586
lastSourceStart = sourceCount - valueCount;
8687
if (ignoreCase)
8788
{
88-
char firstValueChar = InvariantToUpper(value[0]);
89+
char firstValueChar = InvariantCaseFold(value[0]);
8990
for (ctrSource = 0; ctrSource <= lastSourceStart; ctrSource++)
9091
{
91-
sourceChar = InvariantToUpper(source[ctrSource]);
92+
sourceChar = InvariantCaseFold(source[ctrSource]);
9293
if (sourceChar != firstValueChar)
9394
{
9495
continue;
9596
}
9697

9798
for (ctrValue = 1; ctrValue < valueCount; ctrValue++)
9899
{
99-
sourceChar = InvariantToUpper(source[ctrSource + ctrValue]);
100-
valueChar = InvariantToUpper(value[ctrValue]);
100+
sourceChar = InvariantCaseFold(source[ctrSource + ctrValue]);
101+
valueChar = InvariantCaseFold(value[ctrValue]);
101102

102103
if (sourceChar != valueChar)
103104
{
@@ -145,18 +146,18 @@ private static unsafe int InvariantFindString(char* source, int sourceCount, cha
145146
lastSourceStart = sourceCount - valueCount;
146147
if (ignoreCase)
147148
{
148-
char firstValueChar = InvariantToUpper(value[0]);
149+
char firstValueChar = InvariantCaseFold(value[0]);
149150
for (ctrSource = lastSourceStart; ctrSource >= 0; ctrSource--)
150151
{
151-
sourceChar = InvariantToUpper(source[ctrSource]);
152+
sourceChar = InvariantCaseFold(source[ctrSource]);
152153
if (sourceChar != firstValueChar)
153154
{
154155
continue;
155156
}
156157
for (ctrValue = 1; ctrValue < valueCount; ctrValue++)
157158
{
158-
sourceChar = InvariantToUpper(source[ctrSource + ctrValue]);
159-
valueChar = InvariantToUpper(value[ctrValue]);
159+
sourceChar = InvariantCaseFold(source[ctrSource + ctrValue]);
160+
valueChar = InvariantCaseFold(value[ctrValue]);
160161

161162
if (sourceChar != valueChar)
162163
{
@@ -203,16 +204,21 @@ private static unsafe int InvariantFindString(char* source, int sourceCount, cha
203204
return -1;
204205
}
205206

206-
private static char InvariantToUpper(char c)
207+
private static char InvariantCaseFold(char c)
207208
{
209+
// If we ever make Invariant mode support more than just simple ASCII-range case folding,
210+
// then we should update this method to perform proper case folding instead of an
211+
// uppercase conversion. For now it only understands the ASCII range and reflects all
212+
// non-ASCII values unchanged.
213+
208214
return (uint)(c - 'a') <= (uint)('z' - 'a') ? (char)(c - 0x20) : c;
209215
}
210216

211-
private unsafe SortKey InvariantCreateSortKey(string source, CompareOptions options)
217+
private SortKey InvariantCreateSortKey(string source, CompareOptions options)
212218
{
213219
if (source == null) { throw new ArgumentNullException(nameof(source)); }
214220

215-
if ((options & ValidSortkeyCtorMaskOffFlags) != 0)
221+
if ((options & ValidCompareMaskOffFlags) != 0)
216222
{
217223
throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options));
218224
}
@@ -227,23 +233,41 @@ private unsafe SortKey InvariantCreateSortKey(string source, CompareOptions opti
227233
// In the invariant mode, all string comparisons are done as ordinal so when generating the sort keys we generate it according to this fact
228234
keyData = new byte[source.Length * sizeof(char)];
229235

230-
fixed (char* pChar = source) fixed (byte* pByte = keyData)
236+
if ((options & (CompareOptions.IgnoreCase | CompareOptions.OrdinalIgnoreCase)) != 0)
231237
{
232-
if ((options & (CompareOptions.IgnoreCase | CompareOptions.OrdinalIgnoreCase)) != 0)
233-
{
234-
short* pShort = (short*)pByte;
235-
for (int i = 0; i < source.Length; i++)
236-
{
237-
pShort[i] = (short)InvariantToUpper(source[i]);
238-
}
239-
}
240-
else
241-
{
242-
Buffer.MemoryCopy(pChar, pByte, keyData.Length, keyData.Length);
243-
}
238+
InvariantCreateSortKeyOrdinalIgnoreCase(source, keyData);
239+
}
240+
else
241+
{
242+
InvariantCreateSortKeyOrdinal(source, keyData);
244243
}
245244
}
245+
246246
return new SortKey(Name, source, options, keyData);
247247
}
248+
249+
private static void InvariantCreateSortKeyOrdinal(ReadOnlySpan<char> source, Span<byte> sortKey)
250+
{
251+
Debug.Assert(sortKey.Length >= source.Length * sizeof(char));
252+
253+
for (int i = 0; i < source.Length; i++)
254+
{
255+
// convert machine-endian to big-endian
256+
BinaryPrimitives.WriteUInt16BigEndian(sortKey, (ushort)source[i]);
257+
sortKey = sortKey.Slice(sizeof(ushort));
258+
}
259+
}
260+
261+
private static void InvariantCreateSortKeyOrdinalIgnoreCase(ReadOnlySpan<char> source, Span<byte> sortKey)
262+
{
263+
Debug.Assert(sortKey.Length >= source.Length * sizeof(char));
264+
265+
for (int i = 0; i < source.Length; i++)
266+
{
267+
// convert machine-endian to big-endian
268+
BinaryPrimitives.WriteUInt16BigEndian(sortKey, (ushort)InvariantCaseFold(source[i]));
269+
sortKey = sortKey.Slice(sizeof(ushort));
270+
}
271+
}
248272
}
249273
}

‎src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Unix.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -816,7 +816,7 @@ private unsafe SortKey CreateSortKey(string source, CompareOptions options)
816816

817817
if (source==null) { throw new ArgumentNullException(nameof(source)); }
818818

819-
if ((options & ValidSortkeyCtorMaskOffFlags) != 0)
819+
if ((options & ValidCompareMaskOffFlags) != 0)
820820
{
821821
throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options));
822822
}

‎src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Windows.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -505,7 +505,7 @@ private unsafe SortKey CreateSortKey(string source, CompareOptions options)
505505

506506
if (source == null) { throw new ArgumentNullException(nameof(source)); }
507507

508-
if ((options & ValidSortkeyCtorMaskOffFlags) != 0)
508+
if ((options & ValidCompareMaskOffFlags) != 0)
509509
{
510510
throw new ArgumentException(SR.Argument_InvalidFlag, nameof(options));
511511
}

‎src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,11 @@ public sealed partial class CompareInfo : IDeserializationCallback
2424
~(CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols | CompareOptions.IgnoreNonSpace |
2525
CompareOptions.IgnoreWidth | CompareOptions.IgnoreKanaType);
2626

27-
// Mask used to check if Compare() has the right flags.
27+
// Mask used to check if Compare() / GetHashCode(string) / GetSortKey has the right flags.
2828
private const CompareOptions ValidCompareMaskOffFlags =
2929
~(CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols | CompareOptions.IgnoreNonSpace |
3030
CompareOptions.IgnoreWidth | CompareOptions.IgnoreKanaType | CompareOptions.StringSort);
3131

32-
// Mask used to check if GetHashCode(string) has the right flags.
33-
private const CompareOptions ValidHashCodeOfStringMaskOffFlags =
34-
~(CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols | CompareOptions.IgnoreNonSpace |
35-
CompareOptions.IgnoreWidth | CompareOptions.IgnoreKanaType);
36-
37-
// Mask used to check if we have the right flags.
38-
private const CompareOptions ValidSortkeyCtorMaskOffFlags =
39-
~(CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols | CompareOptions.IgnoreNonSpace |
40-
CompareOptions.IgnoreWidth | CompareOptions.IgnoreKanaType | CompareOptions.StringSort);
41-
4232
// Cache the invariant CompareInfo
4333
internal static readonly CompareInfo Invariant = CultureInfo.InvariantCulture.CompareInfo;
4434

@@ -1399,7 +1389,7 @@ public int GetHashCode(string source, CompareOptions options)
13991389
{
14001390
throw new ArgumentNullException(nameof(source));
14011391
}
1402-
if ((options & ValidHashCodeOfStringMaskOffFlags) == 0)
1392+
if ((options & ValidCompareMaskOffFlags) == 0)
14031393
{
14041394
// No unsupported flags are set - continue on with the regular logic
14051395
if (GlobalizationMode.Invariant)
@@ -1428,7 +1418,7 @@ public int GetHashCode(string source, CompareOptions options)
14281418

14291419
public int GetHashCode(ReadOnlySpan<char> source, CompareOptions options)
14301420
{
1431-
if ((options & ValidHashCodeOfStringMaskOffFlags) == 0)
1421+
if ((options & ValidCompareMaskOffFlags) == 0)
14321422
{
14331423
// No unsupported flags are set - continue on with the regular logic
14341424
if (GlobalizationMode.Invariant)

‎src/libraries/System.Private.CoreLib/src/System/Globalization/SortKey.cs‎

Lines changed: 9 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ namespace System.Globalization
99
/// <summary>
1010
/// This class implements a set of methods for retrieving
1111
/// </summary>
12-
public partial class SortKey
12+
public sealed partial class SortKey
1313
{
1414
private readonly string _localeName;
1515
private readonly CompareOptions _options;
@@ -32,13 +32,13 @@ internal SortKey(string localeName, string str, CompareOptions options, byte[] k
3232
/// Returns the original string used to create the current instance
3333
/// of SortKey.
3434
/// </summary>
35-
public virtual string OriginalString => _string;
35+
public string OriginalString => _string;
3636

3737
/// <summary>
3838
/// Returns a byte array representing the current instance of the
3939
/// sort key.
4040
/// </summary>
41-
public virtual byte[] KeyData => (byte[])_keyData.Clone();
41+
public byte[] KeyData => (byte[])_keyData.Clone();
4242

4343
/// <summary>
4444
/// Compares the two sort keys. Returns 0 if the two sort keys are
@@ -62,44 +62,21 @@ public static int Compare(SortKey sortkey1, SortKey sortkey2)
6262
Debug.Assert(key1Data != null, "key1Data != null");
6363
Debug.Assert(key2Data != null, "key2Data != null");
6464

65-
if (key1Data.Length == 0)
66-
{
67-
if (key2Data.Length == 0)
68-
{
69-
return 0;
70-
}
71-
72-
return -1;
73-
}
74-
if (key2Data.Length == 0)
75-
{
76-
return 1;
77-
}
78-
79-
int compLen = (key1Data.Length < key2Data.Length) ? key1Data.Length : key2Data.Length;
80-
for (int i = 0; i < compLen; i++)
81-
{
82-
if (key1Data[i] > key2Data[i])
83-
{
84-
return 1;
85-
}
86-
if (key1Data[i] < key2Data[i])
87-
{
88-
return -1;
89-
}
90-
}
65+
// SortKey comparisons are done as an ordinal comparison by the raw sort key bytes.
9166

92-
return 0;
67+
return new ReadOnlySpan<byte>(key1Data).SequenceCompareTo(key2Data);
9368
}
9469

9570
public override bool Equals(object? value)
9671
{
97-
return value is SortKey otherSortKey && Compare(this, otherSortKey) == 0;
72+
return value is SortKey other
73+
&& new ReadOnlySpan<byte>(_keyData).SequenceEqual(other._keyData);
9874
}
9975

10076
public override int GetHashCode()
10177
{
102-
return CompareInfo.GetCompareInfo(_localeName).GetHashCode(_string, _options);
78+
// keep this in sync with CompareInfo.GetHashCodeOfString
79+
return Marvin.ComputeHash32(_keyData, Marvin.DefaultSeed);
10380
}
10481

10582
public override string ToString()

‎src/libraries/System.Runtime/ref/System.Runtime.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5086,11 +5086,11 @@ public RegionInfo(string name) { }
50865086
public override int GetHashCode() { throw null; }
50875087
public override string ToString() { throw null; }
50885088
}
5089-
public partial class SortKey
5089+
public sealed partial class SortKey
50905090
{
50915091
internal SortKey() { }
5092-
public virtual byte[] KeyData { get { throw null; } }
5093-
public virtual string OriginalString { get { throw null; } }
5092+
public byte[] KeyData { get { throw null; } }
5093+
public string OriginalString { get { throw null; } }
50945094
public static int Compare(System.Globalization.SortKey sortkey1, System.Globalization.SortKey sortkey2) { throw null; }
50955095
public override bool Equals(object? value) { throw null; }
50965096
public override int GetHashCode() { throw null; }

0 commit comments

Comments
 (0)