Fix overflow in LengthBuckets for large strings

[prozolic]

#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.

[Copilot]

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 int to long to prevent overflow
• Added explicit cast to long for the multiplication operation before bounds checking
• Safely cast back to int only after validating the result is within acceptable bounds

[eiriktsarpalis]

Could you add a regression test please?

[prozolic]

@eiriktsarpalis I Added test case with extremely large strings that exceed length bucket boundaries to FrozenFromKnownValuesTests.StringStringData. This error has been confirmed to occur in both the ToFrozenDictionary method and the ToFrozenSet method, so adding this test case verifies both.
Could you please confirm if this is acceptable?

[eiriktsarpalis]

@prozolic given the size of the input I would suggest crafting a separate unit test and additionally annotate it with [OuterLoop] attribute. This will make it less of an issue if it ends up impacting one of more resource constrained machines in CI.

[prozolic]

Thank you for your advice. Is the approach of adding unit tests with the [OuterLoop] attribute to the existing classes as shown below correct?

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));
    }
}

[eiriktsarpalis]

@prozolic yep that looks right 👍

[prozolic]

@eiriktsarpalis Thank you! I've added the tests.

[eiriktsarpalis]

Thank you!