Fix critical bugs in session opening for TCP and UDP in case of Singleplex mode.

[notsure2]

• In case of TCP, don't open the session in the listener accept thread. This causes resource exhaustion of the tcp listener backlog queue in case of internet connection disruption or timeout.

• In case of UDP, don't create a new session for every UDP packet.

This also fixes https://github.com/cbeuw/Cloak-android/issues/17 and corrects a lot of wrong behaviors. @cbeuw Please check and merge, and I recommend a new release.

[notsure2]

Travis says there's a data race in the integration test, but I think it's a bug in the test.

[codecov[bot]]

Codecov Report

Merging #145 (6b6b1b3) into master (7b6a82b) will decrease coverage by 0.14%. The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master     #145      +/-   ##
==========================================
- Coverage   67.62%   67.48%   -0.15%     
==========================================
  Files          37       37              
  Lines        2400     2405       +5     
==========================================
  Hits         1623     1623              
- Misses        615      618       +3     
- Partials      162      164       +2     

Impacted Files	Coverage Δ
cmd/ck-client/ck-client.go	0.00% <0.00%> (ø)
internal/client/piper.go	45.56% <44.44%> (+0.23%)	:arrow_up:
internal/multiplex/streamBufferedPipe.go	90.54% <0.00%> (-2.71%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7b6a82b...6b6b1b3. Read the comment docs.

[cbeuw]

Sorry it took me a while to get back to this. I agree that for singleplexing TCP, the new session function should be called inside the newly created goroutine. The behaviour with UDP is slightly trickier because there isn't a strict sense of connection. Currently Cloak assumes each incoming ip:port pair represents a "connection", which may not be correct to begin with

[cbeuw]

The race condition also exists here, where SessionId in authInfo is written to. Previously this seshMaker() is always called by a single goroutine so this is fine, now we need to copy authInfo from the outside scope to a local variable https://github.com/cbeuw/Cloak/blob/7b6a82b364d81af7b4f314a0ba490678f8e7d4e8/cmd/ck-client/ck-client.go#L171-L178

[cbeuw]

Just some small changes - passing newSeshFunc as an argument to the goroutine function isn't necessary because we are not mutating the variable, we are only reading it. Also I don't think the explicit variable of multiplexSession is necessary. For TCP, the variable becomes local on entry to the goroutine function (despite the name shadowing), and for UDP there isn't any concurrency in session opening anyway. I hope that still gives the right behaviours

[notsure2]

Just some small changes - passing newSeshFunc as an argument to the goroutine function isn't necessary because we are not mutating the variable, we are only reading it.

I considered it as an optimization to avoid allocation of an object to hold the variables captured in the closure, passing it as a parameter reduces memory use (object allocation on every time the goroutine is called).

Also I don't think the explicit variable of multiplexSession is necessary. For TCP, the variable becomes local on entry to the goroutine function (despite the name shadowing),

It is not technically necessary but it makes the code clearer and it clarifies the intention of what we are doing.

And for UDP there isn't any concurrency in session opening anyway.

The problem in UDP was that it was opening a new session for every packet, not a concurrency issue.

I hope that still gives the right behaviours.

I will review and test.

[notsure2]

@cbeuw ok i tested tcp singleplex and multiplex, udp singleplex and multiplex and seems to work.