search

thaliproject / Thali_CordovaPlugin

Thali p2p plugin
MIT License
226 stars 44 forks source link

ThaliCore: maxVirtualSocketsCount bug #1893

Closed enricogior closed 7 years ago

enricogior commented 7 years ago

maxVirtualSocketsCount is used to limit the max number of concurrent virtual sockets in use by the BrowserRelay but the check [here] can't work properly if an app opens multiple connections in a row. Given the async nature of the createVirtualSocket, the actual virtualSockets.value.count will only be increased later on when the completion handler is invoked. I've verified it opening 30 connections without reaching the limit since all 30 calls to createVirtualSocket were completed before the completion handler is invoked.

enricogior commented 7 years ago

Even if the goal is to limit the max numbers of VirtualSockets, the check should be done at a higher level in the TCPListener class, before even trying to create a new VirtualSocket.

enricogior commented 7 years ago

The max number of overall connections in the native layer is limited by the number of concurrent virtual sockets: each virtual socket adds an entry to RunLoop and the max number of entries per process is 64, that is enforced by the OS. Adding more than 64 entries doesn't trigger an error in RunLoop, simply the entries after the 64th will not be scheduled until other entries have been removed. That is one of the two bugs that were causing the timeout failure since the RunLoop entries were never removed.

The current code that limits each BrowserRelay to 16 connections, can't prevent to reach the overall number of virtual sockets created by the BrowseRelays and the AdvertiserRelays. We should decide the max number of overall concurrent virtual sockets (i.e. it should be less the 64 since we have already at least one entry in the RunLoop for other purpose, so it should be no more than 62 to be safe). I still have to decide where to put the check for the max overall virtual sockets and how to handle the error, since it's not a connection error per se, but just an internal limit.

We should also decide if we need to limit the max number of virtual sockets per BrowserRelay or per Advertiser Relay. It's not strictly needed but if there is a good reason to do so we should fix the current implementation since it's broken (see the first comment).

@yaronyg @mlesnic is there any reason to limit the max connections in each BrowserRelay or in each AdvertiserRelay? If so, what should the max be, 16?

yaronyg commented 7 years ago

So the point of this was really more than anything to try and slow down DOS attacks by people connecting to the device (e.g. on the advertiser, not the browser). But we never really did a good job here. We need #849 and most likely some kind of native event to let us know when we have an incoming connection. I'm o.k. with us either removing the limit and putting in a big warning in the docs that on Mac we can't have more than 64 peer connections in both directions. But we should file a bug saying we suck and we should fix this so we limit things ourselves and return proper errors.

enricogior commented 7 years ago

https://github.com/thaliproject/Thali_CordovaPlugin/issues/1933 https://github.com/thaliproject/Thali_CordovaPlugin/issues/1934

I've also removed the current (broken) limit in BrowserRelay.