meebey / SmartIrc4net

IRC C# Library
http://www.meebey.net/projects/smartirc4net/
Other
126 stars 52 forks source link

Round-robin connection crash when shortening address list #36

Open djkaty opened 9 years ago

djkaty commented 9 years ago

I haven't checked this, but if I'm reading the code in IrcConnection.cs properly, is there not potential for an IndexOutOfBoundsException under the following circumstances:

  1. Turn AutoRetry on
  2. Provide a series of addresses in Connect
  3. Retry advances one or more times to position X in the list before a successful connection
  4. Later, you call Connect again with a shorter list of addresses
  5. First connection attempt is at position X; if X > number of addresses in the new list, the index is out of bounds

_NextAddress() is never called / _CurrentAddress is never reset to 0 at the beginning of a new connection attempt, I think? Could someone confirm this?

meebey commented 9 years ago

You are right if the initial list has more than one entry and the first address failed it will increment _CurrentAddress but Connect() assumes the list length hasnt changed. It should reset the incrementor if the list has changed from the initial list. The recursion also buggers me, if a server isnt available for a long time it will crash with a stack overflow. This happened in Smuxi already.

djkaty commented 9 years ago

I'll fix it in this branch. I've already merged a few code paths where threads are unnecessarily closing things that were already half-centralized in Disconnect(). It's a nightmare to abstract.

djkaty commented 9 years ago

I'll fix the recursion as well.

djkaty commented 9 years ago

Both these issues are now fixed in my codebase. PR soon.

djkaty commented 9 years ago

There was also a bug in the AutoRetry code (condition of how many attempts had been made was using >= instead of <, causing it to never make more than one attempt), which is also fixed.