Fix overflow in LengthBuckets for large strings - #121674
#121674Fix overflow in LengthBuckets for large strings#121674
Conversation
LengthBuckets.CreateLengthBucketsArrayIfAppropriate caused an integer overflow when calculating bucket array size for very large strings, resulting in a negative value being passed to ArrayPool<T>.Shared.Rent. This triggered an ArgumentOutOfRangeException when creating a FrozenDictionary with large strings approaching Array.MaxLength. Add an overflow check to prevent creating buckets when the calculation would overflow.
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an integer overflow bug in LengthBuckets.CreateLengthBucketsArrayIfAppropriate that occurred when creating FrozenDictionaries with very large strings. The overflow happened during calculation of the bucket array size, causing a negative value to be passed to ArrayPool<T>.Shared.Rent, which resulted in an ArgumentOutOfRangeException.
Key Changes:
- Changed intermediate array size calculation from
inttolongto prevent overflow - Added explicit cast to
longfor the multiplication operation before bounds checking - Safely cast back to
intonly after validating the result is within acceptable bounds
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Could you add a regression test please?
|
@eiriktsarpalis I Added test case with extremely large strings that exceed length bucket boundaries to |
|
@prozolic given the size of the input I would suggest crafting a separate unit test and additionally annotate it with |
|
Thank you for your advice. Is the approach of adding unit tests with the public abstract class FrozenDictionary_Generic_Tests_string_string
{
[Fact]
[OuterLoop]
public void ToFrozenDictionary_WithExtremelyLargeStrings()
{
// Test case with extremely large strings that exceed length bucket boundaries.
var keys = new[] { "", new string('a', 0X7FFFFFC7 / 4) };
var frozen = keys.ToFrozenDictionary(s => s, GetKeyIEqualityComparer());
Assert.Equal(keys.Length, frozen.Count);
Assert.Equal(keys.Length, frozen.Keys.Length);
Assert.Equal(keys.Length, frozen.Values.Length);
Assert.All(keys, key => frozen.ContainsKey(key));
}
}
public abstract class FrozenSet_Generic_Tests_string
{
[Fact]
[OuterLoop]
public void ToFrozenSet_WithExtremelyLargeStrings()
{
// Test case with extremely large strings that exceed length bucket boundaries.
var keys = new[] { "", new string('a', 0X7FFFFFC7 / 4) };
var frozen = keys.ToFrozenSet(GetIEqualityComparer());
Assert.Equal(keys.Length, frozen.Count);
Assert.All(keys, key => frozen.Contains(key));
}
} |
|
@prozolic yep that looks right 👍 |
…es in FrozenDictionary and FrozenSet
|
@eiriktsarpalis Thank you! I've added the tests. |
#121673
LengthBuckets.CreateLengthBucketsArrayIfAppropriate caused an integer overflow when calculating bucket array size for very large strings, resulting in a negative value being passed to ArrayPool.Shared.Rent. This triggered an ArgumentOutOfRangeException when creating a FrozenDictionary with large strings approaching Array.MaxLength.
Add an overflow check to prevent creating buckets when the calculation would overflow.