Skip to content

Fix ISXB-1792: HaveDuplicateReferences inner loop bound (n < count no… #2376

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
K-Tone wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fix ISXB-1792: HaveDuplicateReferences inner loop bound (n < count no… #2376

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions Assets/Tests/InputSystem/Utilities/ArrayHelperTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,23 @@ public void Utilities_IndexOfReference__IsUsingReferenceEqualsAndConstrainedBySt
Assert.AreEqual(2, arr.IndexOfReference(arr[2], 1, 3));
}

[Test]
[Category("Utilities")]
public void Utilities_HaveDuplicateReferences_DetectsDuplicatesInFullRange()
{
var withDup = new object[] { new object(), new object(), new object() };
withDup[2] = withDup[0]; // duplicate at 0 and 2
Assert.That(withDup.HaveDuplicateReferences(0, 3), Is.True);
Comment on lines +126 to +127
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium
Have you considered adding a test case with a non-zero starting index (e.g., index: 1)? This would ensure the bounds are correctly respected. Because the implementation of HaveDuplicateReferences currently ignores the index parameter, a test relying on an offset would fail, helping to expose the missing offset bug.

🤖 Helpful? 👍/👎


var noDup = new object[] { new object(), new object(), new object() };
Assert.That(noDup.HaveDuplicateReferences(0, 3), Is.False);

// Regression test for ISXB-1792: inner loop was "n < count - i" so later pairs were never checked
var dupAtEnd = new object[] { new object(), new object(), new object(), new object() };
dupAtEnd[3] = dupAtEnd[2]; // duplicate at 2 and 3
Assert.That(dupAtEnd.HaveDuplicateReferences(0, 4), Is.True);
}

[Test]
[Category("Utilities")]
public void Utilities_IndexOfPredicate__IsUsingPredicateForEqualityAndConstraintedByStartIndexAndCount()
Expand Down
2 changes: 1 addition & 1 deletion Packages/com.unity.inputsystem/InputSystem/Utilities/ArrayHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public static bool HaveDuplicateReferences<TFirst>(this TFirst[] first, int inde
for (var i = 0; i < count; ++i)
{
var element = first[i];
for (var n = i + 1; n < count - i; ++n)
for (var n = i + 1; n < count; ++n)
Comment on lines 120 to +123
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high
While this PR successfully fixes the inner loop's end condition, the method still entirely ignores the index parameter. Both the outer and inner loops iterate up to count but start at 0 instead of index, meaning they will scan the wrong range of elements if index > 0.

Should this be updated to respect the index parameter?

for (var i = index; i < index + count; ++i)
{
    var element = first[i];
    for (var n = i + 1; n < index + count; ++n)
    {
        if (ReferenceEquals(element, first[n]))
            return true;
    }
}

🤖 Helpful? 👍/👎

{
if (ReferenceEquals(element, first[n]))
Copy link
Contributor

@u-pr u-pr bot Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

low
Because ReferenceEquals is used for comparison, this method will always return false for value types (like int or struct) because they are boxed into unique objects when passed as object arguments. Have you considered adding a where TFirst : class constraint to the generic type to prevent accidental misuse with value types?

🤖 Helpful? 👍/👎

return true;
Expand Down
Loading