Skip to content
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.

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

Infinite loop in ToolStripItemCollection.AddRange #12495

Open
ottoman52 opened this issue Nov 15, 2024 · 11 comments · May be fixed by #12513
Open

Infinite loop in ToolStripItemCollection.AddRange #12495

ottoman52 opened this issue Nov 15, 2024 · 11 comments · May be fixed by #12513
Assignees
Labels
🪲 bug Product bug (most likely) 💥 regression-release Regression from a public release 🚧 work in progress Work that is current in progress
Milestone

Comments

@ottoman52
Copy link

.NET version

.NET 8.0

Did it work in .NET Framework?

Yes

Did it work in any of the earlier releases of .NET Core or .NET 5+?

I dont know

Issue description

There is an infinite loop in ToolStripItemCollection.cs:

public void AddRange(ToolStripItemCollection toolStripItems)
{
	ArgumentNullException.ThrowIfNull(toolStripItems, "toolStripItems");
	if (IsReadOnly)
	{
		throw new NotSupportedException(System.SR.ToolStripItemCollectionIsReadOnly);
	}
	using (new LayoutTransaction(_owner, _owner, "Items"))
	{
		for (int i = 0; i < toolStripItems.Count; i++)
		{
			Add(toolStripItems[i--]);
		}
	}
}

for (int i = 0; i < toolStripItems.Count; i++) counts up from 0 but decrements 'i' down in Add(toolStripItems[i--]);

Any call to add items using this override will infinite loop with i going from 0 -> -1 -> 0 -> -1 etc...

Steps to reproduce

Any call to ToolStripItemCollection.AddRange(ToolStripItemCollection toolStripItems) with items.

@ottoman52 ottoman52 added the untriaged The team needs to look at this issue in the next triage label Nov 15, 2024
@elachlan
Copy link
Contributor

@Olina-Zhang can your team test this please?

@elachlan elachlan added the 🪲 bug Product bug (most likely) label Nov 16, 2024
@Olina-Zhang
Copy link
Member

@Olina-Zhang can your team test this please?

We can repro this issue from .NET 6.0 to .NET 10 latest build with following testing code and sample application, not .NET 5.0 and .NET framework 4.8.1

public Form1()
{
    InitializeComponent();
    // Step 1: Add a ToolStrip to form designer, named mainToolStrip
    // Step 2: Create a second ToolStripItemCollection (you can also create individual ToolStripItems)
    ToolStripItemCollection itemCollection = new ToolStripItemCollection(mainToolStrip, new ToolStripItem[] {
    new ToolStripButton("Button 1"),
    new ToolStripButton("Button 2")});

    // Step 3: Call AddRange with the second collection
    mainToolStrip.Items.AddRange(itemCollection);  // This will cause an infinite loop
}

WinFormsApp26.zip

@elachlan elachlan added the 💥 regression-release Regression from a public release label Nov 18, 2024
@elachlan
Copy link
Contributor

@Olina-Zhang since the fix is pretty straight forward. Did your team want to take it on?

@Olina-Zhang
Copy link
Member

@elachlan Tried to modify AddRange method with following, although this issue is resolved, but the issue #4454 is not fixed well. Need to investigate further...

 public void AddRange(ToolStripItemCollection toolStripItems)
 {
     ArgumentNullException.ThrowIfNull(toolStripItems);

     if (IsReadOnly)
     {
         throw new NotSupportedException(SR.ToolStripItemCollectionIsReadOnly);
     }

     using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items))
     {
         for (int i = 0; i < toolStripItems.Count; i++)
         {
             Add(toolStripItems[i]);
         }
     }
 }

@elachlan
Copy link
Contributor

elachlan commented Nov 18, 2024

@Olina-Zhang use a reverse for loop.

public void AddRange(ToolStripItemCollection toolStripItems)
{
    ArgumentNullException.ThrowIfNull(toolStripItems);

    if (IsReadOnly)
    {
        throw new NotSupportedException(SR.ToolStripItemCollectionIsReadOnly);
    }

    // ToolStripDropDown will look for PropertyNames.Items to determine if it needs
    // to resize itself.
    using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items))
    {
        // Iterate in reverse to avoid index shifting issues.
        for (int i = toolStripItems.Count - 1; i >= 0; i--)
        {
            Add(toolStripItems[i]);
        }
    }
}

@Olina-Zhang
Copy link
Member

Looks good, it can resolve both of issues.

@elachlan
Copy link
Contributor

We should make sure we have test coverage of both issues.

@Olina-Zhang
Copy link
Member

for (int i = toolStripItems.Count - 1; i >= 0; i--)
{
Add(toolStripItems[i]);
}

this will cause the order of items is reversed, see following:
Image

can we do toolStripItems.InnerList.Reverse() firstly?

using (new LayoutTransaction(_owner, _owner!, PropertyNames.Items))
    {
        // Iterate in reverse to avoid index shifting issues.
        toolStripItems.InnerList.Reverse();
        for (int i = toolStripItems.Count - 1; i >= 0; i--)
        {
            Add(toolStripItems[i]);
        }
    }

@merriemcgaw merriemcgaw removed the untriaged The team needs to look at this issue in the next triage label Nov 19, 2024
@merriemcgaw merriemcgaw added this to the .NET 10.0 milestone Nov 19, 2024
@merriemcgaw
Copy link
Member

merriemcgaw commented Nov 19, 2024

Great work @Olina-Zhang and @elachlan ! Feel free to submit the PR 😄

@Tanya-Solyanik
Copy link
Member

Consider converting collection to an array (.ToArray()) instead of reversing and reading backwards.

@dotnet-policy-service dotnet-policy-service bot added the 🚧 work in progress Work that is current in progress label Nov 20, 2024
@Tanya-Solyanik
Copy link
Member

@merriemcgaw - are we servicing this hung?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪲 bug Product bug (most likely) 💥 regression-release Regression from a public release 🚧 work in progress Work that is current in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants