Skip to content
This repository has been archived by the owner on Sep 29, 2024. It is now read-only.

Fixed "Index out of range" bug in control channel #143

Closed

Conversation

Grivus
Copy link

@Grivus Grivus commented Dec 17, 2019

Hi there!

The bug story

I've tried to establish a VPN connection from iOS device to my test VPN server with SoftEther on board. SoftEhter declares, that it supports openvpn protocol (and it really does).
However, when I built and setup all things, soon after connection in the Demo app, I got a disconnected state.
I've analyzed the logs, and it seems that at some moment of time the runtime error "Index out of range" occures inside network extension, which causes network session daemon to unload this extension and drop connection.
After adding some debug logs I found a place with the problem: we are iterating the queue with packets, deleting the packet from it at the same time. So after deleting we have a problem with array indexes.

The fix
This is a well-known problem, and as far as I could say, in Swift the common solution is to iterate over reversed array, so index changing would not affect any next elements, which I implemented here.

…k extension to be unloaded

Now we iterate over reversed array and removing elements from it is safe
@keeshux
Copy link
Member

keeshux commented Dec 19, 2019

Good catch, will merge ASAP. Thank you!

@keeshux
Copy link
Member

keeshux commented Dec 19, 2019

Now that I see the code though (which is part of an old port that was never really reviewed), I'd rather do the following:

queue.outbound.removeAll { packetIds.contains($0.packetId) }

@Grivus
Copy link
Author

Grivus commented Dec 19, 2019

@keeshux yep, I'm not very familiar with Swift, the code you proposed looks more in-a-swift-way :)

@keeshux keeshux self-assigned this Dec 21, 2019
@keeshux keeshux added the bug Something isn't working label Dec 21, 2019
@keeshux keeshux added this to the 2.2.2 milestone Dec 21, 2019
@keeshux
Copy link
Member

keeshux commented Dec 21, 2019

@Grivus could you amend your code with my version and push force? So that I can still merge the PR as yours and attribute in the CHANGELOG.

I still wonder if you've tested this code with SoftEther and standard OpenVPN servers, or if your fix is only a guess.

@Grivus
Copy link
Author

Grivus commented Dec 21, 2019

@keeshux I could do this amend on Monday I guess, if it is ok for you. If it's too late, I think you could commit it in yours way.

Regarding the tests: I've tested this code with SoftEther VPN, which has like an openvpn switch, and after this fix I've been able to actually establish connection with it, and it stays alive at least for some time (haven't seen what's going on if I leave it for a day e.g.)

I've not tried the demo with standard OpenVPN servers, as I have right now only SoftEther running. I don't think this fix could break the standard OpenVPN servers connection, but of cource, it is worth to verify it.

@keeshux keeshux closed this in e3241f4 Dec 22, 2019
@keeshux
Copy link
Member

keeshux commented Dec 22, 2019

@Grivus please ditch this branch, pull from master and let me know if your SoftEther negotiation is fixed by my code. I tested OpenVPN fine. Credit is given in the CHANGELOG.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants