From 05063f074aa5722d74c4d27011ad8079cd624a88 Mon Sep 17 00:00:00 2001 From: noisyfox Date: Wed, 14 Sep 2016 00:46:45 +1000 Subject: [PATCH 1/4] Fix an issue that the socket may not be released. - New wrapper to handle tcp socket connection. - Fix some memory leak. Signed-off-by: noisyfox --- .../Controller/Service/PortForwarder.cs | 12 +- shadowsocks-csharp/Controller/Service/TCPRelay.cs | 16 +- shadowsocks-csharp/Proxy/DirectConnect.cs | 11 +- shadowsocks-csharp/Proxy/Socks5Proxy.cs | 14 +- shadowsocks-csharp/Util/SocketUtil.cs | 132 ---------- shadowsocks-csharp/Util/Sockets/SocketUtil.cs | 72 ++++++ shadowsocks-csharp/Util/Sockets/WrappedSocket.cs | 268 +++++++++++++++++++++ shadowsocks-csharp/shadowsocks-csharp.csproj | 3 +- 8 files changed, 374 insertions(+), 154 deletions(-) delete mode 100644 shadowsocks-csharp/Util/SocketUtil.cs create mode 100644 shadowsocks-csharp/Util/Sockets/SocketUtil.cs create mode 100644 shadowsocks-csharp/Util/Sockets/WrappedSocket.cs diff --git a/shadowsocks-csharp/Controller/Service/PortForwarder.cs b/shadowsocks-csharp/Controller/Service/PortForwarder.cs index 6be4dbef..54ecd49d 100644 --- a/shadowsocks-csharp/Controller/Service/PortForwarder.cs +++ b/shadowsocks-csharp/Controller/Service/PortForwarder.cs @@ -1,7 +1,7 @@ using System; using System.Net; using System.Net.Sockets; -using Shadowsocks.Util; +using Shadowsocks.Util.Sockets; namespace Shadowsocks.Controller { @@ -29,7 +29,7 @@ namespace Shadowsocks.Controller private byte[] _firstPacket; private int _firstPacketLength; private Socket _local; - private Socket _remote; + private WrappedSocket _remote; private bool _closed = false; private bool _localShutdown = false; private bool _remoteShutdown = false; @@ -49,7 +49,8 @@ namespace Shadowsocks.Controller EndPoint remoteEP = SocketUtil.GetEndPoint("localhost", targetPort); // Connect to the remote endpoint. - SocketUtil.BeginConnectTcp(remoteEP, ConnectCallback, null); + _remote = new WrappedSocket(); + _remote.BeginConnect(remoteEP, ConnectCallback, null); } catch (Exception e) { @@ -66,7 +67,8 @@ namespace Shadowsocks.Controller } try { - _remote = SocketUtil.EndConnectTcp(ar); + _remote.EndConnect(ar); + _remote.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.NoDelay, true); HandshakeReceive(); } catch (Exception e) @@ -243,7 +245,7 @@ namespace Shadowsocks.Controller try { _remote.Shutdown(SocketShutdown.Both); - _remote.Close(); + _remote.Dispose(); } catch (SocketException e) { diff --git a/shadowsocks-csharp/Controller/Service/TCPRelay.cs b/shadowsocks-csharp/Controller/Service/TCPRelay.cs index d266e0ee..975c6df8 100644 --- a/shadowsocks-csharp/Controller/Service/TCPRelay.cs +++ b/shadowsocks-csharp/Controller/Service/TCPRelay.cs @@ -9,7 +9,7 @@ using Shadowsocks.Controller.Strategy; using Shadowsocks.Encryption; using Shadowsocks.Model; using Shadowsocks.Proxy; -using Shadowsocks.Util; +using Shadowsocks.Util.Sockets; namespace Shadowsocks.Controller { @@ -40,7 +40,6 @@ namespace Shadowsocks.Controller handler.controller = _controller; handler.tcprelay = this; - handler.Start(firstPacket, length); IList handlersToClose = new List(); lock (Handlers) { @@ -59,6 +58,15 @@ namespace Shadowsocks.Controller Logging.Debug("Closing timed out TCP connection."); handler1.Close(); } + + /* + * Start after we put it into Handlers set. Otherwise if it failed in handler.Start() + * then it will call handler.Close() before we add it into the set. + * Then the handler will never release until the next Handle call. Sometimes it will + * cause odd problems (especially during memory profiling). + */ + handler.Start(firstPacket, length); + return true; } @@ -455,7 +463,7 @@ namespace Shadowsocks.Controller timer.Dispose(); - if (_proxyConnected || _destConnected) + if (_proxyConnected || _destConnected || _closed) { return; } @@ -527,7 +535,7 @@ namespace Shadowsocks.Controller timer.Enabled = false; timer.Dispose(); - if (_destConnected) + if (_destConnected || _closed) { return; } diff --git a/shadowsocks-csharp/Proxy/DirectConnect.cs b/shadowsocks-csharp/Proxy/DirectConnect.cs index 76bdbc77..22f5d610 100644 --- a/shadowsocks-csharp/Proxy/DirectConnect.cs +++ b/shadowsocks-csharp/Proxy/DirectConnect.cs @@ -2,7 +2,7 @@ using System.Net; using System.Net.Sockets; using System.Threading; -using Shadowsocks.Util; +using Shadowsocks.Util.Sockets; namespace Shadowsocks.Proxy { @@ -31,7 +31,7 @@ namespace Shadowsocks.Proxy } } - private Socket _remote; + private WrappedSocket _remote = new WrappedSocket(); public EndPoint LocalEndPoint => _remote.LocalEndPoint; @@ -55,12 +55,13 @@ namespace Shadowsocks.Proxy { DestEndPoint = destEndPoint; - SocketUtil.BeginConnectTcp(destEndPoint, callback, state); + _remote.BeginConnect(destEndPoint, callback, state); } public void EndConnectDest(IAsyncResult asyncResult) { - _remote = SocketUtil.EndConnectTcp(asyncResult); + _remote.EndConnect(asyncResult); + _remote.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.NoDelay, true); } public void BeginSend(byte[] buffer, int offset, int size, SocketFlags socketFlags, AsyncCallback callback, @@ -92,7 +93,7 @@ namespace Shadowsocks.Proxy public void Close() { - _remote?.Close(); + _remote?.Dispose(); } } } diff --git a/shadowsocks-csharp/Proxy/Socks5Proxy.cs b/shadowsocks-csharp/Proxy/Socks5Proxy.cs index 4ee6917c..5227ae74 100644 --- a/shadowsocks-csharp/Proxy/Socks5Proxy.cs +++ b/shadowsocks-csharp/Proxy/Socks5Proxy.cs @@ -1,12 +1,10 @@ using System; -using System.Collections.Generic; -using System.Linq; using System.Net; using System.Net.Sockets; using System.Text; using System.Threading; using Shadowsocks.Controller; -using Shadowsocks.Util; +using Shadowsocks.Util.Sockets; namespace Shadowsocks.Proxy { @@ -41,7 +39,7 @@ namespace Shadowsocks.Proxy public Exception ex { get; set; } } - private Socket _remote; + private WrappedSocket _remote = new WrappedSocket(); private const int Socks5PktMaxSize = 4 + 16 + 2; private readonly byte[] _receiveBuffer = new byte[Socks5PktMaxSize]; @@ -58,7 +56,7 @@ namespace Shadowsocks.Proxy ProxyEndPoint = remoteEP; - SocketUtil.BeginConnectTcp(remoteEP, ConnectCallback, st); + _remote.BeginConnect(remoteEP, ConnectCallback, st); } public void EndConnectProxy(IAsyncResult asyncResult) @@ -168,7 +166,7 @@ namespace Shadowsocks.Proxy public void Close() { - _remote?.Close(); + _remote?.Dispose(); } @@ -177,7 +175,9 @@ namespace Shadowsocks.Proxy var state = (Socks5State) ar.AsyncState; try { - _remote = SocketUtil.EndConnectTcp(ar); + _remote.EndConnect(ar); + + _remote.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.NoDelay, true); byte[] handshake = {5, 1, 0}; _remote.BeginSend(handshake, 0, handshake.Length, 0, Socks5HandshakeSendCallback, state); diff --git a/shadowsocks-csharp/Util/SocketUtil.cs b/shadowsocks-csharp/Util/SocketUtil.cs deleted file mode 100644 index ea55262e..00000000 --- a/shadowsocks-csharp/Util/SocketUtil.cs +++ /dev/null @@ -1,132 +0,0 @@ -using System; -using System.Net; -using System.Net.Sockets; -using System.Threading; - -namespace Shadowsocks.Util -{ - public static class SocketUtil - { - private class DnsEndPoint2 : DnsEndPoint - { - public DnsEndPoint2(string host, int port) : base(host, port) - { - } - - public DnsEndPoint2(string host, int port, AddressFamily addressFamily) : base(host, port, addressFamily) - { - } - - public override string ToString() - { - return this.Host + ":" + this.Port; - } - } - - public static EndPoint GetEndPoint(string host, int port) - { - IPAddress ipAddress; - bool parsed = IPAddress.TryParse(host, out ipAddress); - if (parsed) - { - return new IPEndPoint(ipAddress, port); - } - - // maybe is a domain name - return new DnsEndPoint2(host, port); - } - - private class AutoReleaseAsyncResult : IAsyncResult - { - - public bool IsCompleted { get; } = true; - public WaitHandle AsyncWaitHandle { get; } = null; - public object AsyncState { get; set; } - public bool CompletedSynchronously { get; } = true; - - public TcpUserToken UserToken { get; set; } - - ~AutoReleaseAsyncResult() - { - UserToken.Dispose(); - } - } - - private class TcpUserToken - { - public AsyncCallback Callback { get; private set; } - public SocketAsyncEventArgs Args { get; private set; } - public object AsyncState { get; private set; } - - public TcpUserToken(AsyncCallback callback, object state, SocketAsyncEventArgs args) - { - Callback = callback; - AsyncState = state; - Args = args; - } - - public void Dispose() - { - Args?.Dispose(); - Callback = null; - Args = null; - AsyncState = null; - } - } - - private static void OnTcpConnectCompleted(object sender, SocketAsyncEventArgs args) - { - args.Completed -= OnTcpConnectCompleted; - TcpUserToken token = (TcpUserToken) args.UserToken; - - AutoReleaseAsyncResult r = new AutoReleaseAsyncResult - { - AsyncState = token.AsyncState, - UserToken = token - }; - - token.Callback(r); - } - - public static void BeginConnectTcp(EndPoint endPoint, AsyncCallback callback, object state) - { - var arg = new SocketAsyncEventArgs(); - arg.RemoteEndPoint = endPoint; - arg.Completed += OnTcpConnectCompleted; - arg.UserToken = new TcpUserToken(callback, state, arg); - - - Socket.ConnectAsync(SocketType.Stream, ProtocolType.Tcp, arg); - } - - public static Socket EndConnectTcp(IAsyncResult asyncResult) - { - var r = asyncResult as AutoReleaseAsyncResult; - if (r == null) - { - throw new ArgumentException("Invalid asyncResult.", nameof(asyncResult)); - } - - var tut = r.UserToken; - - var arg = tut.Args; - - if (arg.SocketError != SocketError.Success) - { - if (arg.ConnectByNameError != null) - { - throw arg.ConnectByNameError; - } - - var ex = new SocketException((int)arg.SocketError); - throw ex; - } - - var so = tut.Args.ConnectSocket; - - so.SetSocketOption(SocketOptionLevel.Tcp, SocketOptionName.NoDelay, true); - - return so; - } - } -} diff --git a/shadowsocks-csharp/Util/Sockets/SocketUtil.cs b/shadowsocks-csharp/Util/Sockets/SocketUtil.cs new file mode 100644 index 00000000..2cf583dd --- /dev/null +++ b/shadowsocks-csharp/Util/Sockets/SocketUtil.cs @@ -0,0 +1,72 @@ +using System; +using System.Net; +using System.Net.Sockets; + +namespace Shadowsocks.Util.Sockets +{ + public static class SocketUtil + { + private class DnsEndPoint2 : DnsEndPoint + { + public DnsEndPoint2(string host, int port) : base(host, port) + { + } + + public DnsEndPoint2(string host, int port, AddressFamily addressFamily) : base(host, port, addressFamily) + { + } + + public override string ToString() + { + return this.Host + ":" + this.Port; + } + } + + public static EndPoint GetEndPoint(string host, int port) + { + IPAddress ipAddress; + bool parsed = IPAddress.TryParse(host, out ipAddress); + if (parsed) + { + return new IPEndPoint(ipAddress, port); + } + + // maybe is a domain name + return new DnsEndPoint2(host, port); + } + + + public static void FullClose(this System.Net.Sockets.Socket s) + { + try + { + s.Shutdown(SocketShutdown.Both); + } + catch (Exception) + { + } + try + { + s.Disconnect(false); + } + catch (Exception) + { + } + try + { + s.Close(); + } + catch (Exception) + { + } + try + { + s.Dispose(); + } + catch (Exception) + { + } + } + + } +} diff --git a/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs b/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs new file mode 100644 index 00000000..93a5f7a6 --- /dev/null +++ b/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs @@ -0,0 +1,268 @@ +using System; +using System.Net; +using System.Net.Sockets; +using System.Threading; + +namespace Shadowsocks.Util.Sockets +{ + /* + * A wrapped socket class which support both ipv4 and ipv6 based on the + * connected remote endpoint. + * + * If the server address is host name, then it may have both ipv4 and ipv6 address + * after resolving. The main idea is we don't want to resolve and choose the address + * by ourself. Instead, Socket.ConnectAsync() do handle this thing internally by trying + * each address and returning an established socket connection. Such approach solves + * two problem: + * 1. Async DNS resolving. + * 2. + */ + public class WrappedSocket + { + public EndPoint LocalEndPoint => _activeSocket?.LocalEndPoint; + + private SpinLock _socketSyncLock = new SpinLock(); + // Only used during connection and close, so it won't cost too much. + + private volatile bool _disposed; + private bool Connected => _activeSocket != null; + private Socket _activeSocket; + + + public void BeginConnect(EndPoint remoteEP, AsyncCallback callback, object state) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (Connected) + { + throw new SocketException((int) SocketError.IsConnected); + } + + var arg = new SocketAsyncEventArgs(); + arg.RemoteEndPoint = remoteEP; + arg.Completed += OnTcpConnectCompleted; + arg.UserToken = new TcpUserToken(callback, state); + + Socket.ConnectAsync(SocketType.Stream, ProtocolType.Tcp, arg); + } + + private class FakeAsyncResult : IAsyncResult + { + public bool IsCompleted { get; } = true; + public WaitHandle AsyncWaitHandle { get; } = null; + public object AsyncState { get; set; } + public bool CompletedSynchronously { get; } = true; + public Exception InternalException { get; set; } = null; + } + + private class TcpUserToken + { + public AsyncCallback Callback { get; } + public object AsyncState { get; } + + public TcpUserToken(AsyncCallback callback, object state) + { + Callback = callback; + AsyncState = state; + } + } + + private void OnTcpConnectCompleted(object sender, SocketAsyncEventArgs args) + { + using (args) + { + args.Completed -= OnTcpConnectCompleted; + var token = (TcpUserToken) args.UserToken; + + if (args.SocketError != SocketError.Success) + { + var ex = args.ConnectByNameError ?? new SocketException((int) args.SocketError); + + var r = new FakeAsyncResult() + { + AsyncState = token.AsyncState, + InternalException = ex + }; + + token.Callback(r); + } + else + { + var lockTaken = false; + if (!_socketSyncLock.IsHeldByCurrentThread) + { + _socketSyncLock.TryEnter(ref lockTaken); + } + try + { + if (Connected) + { + args.ConnectSocket.FullClose(); + } + else + { + _activeSocket = args.ConnectSocket; + if (_disposed) + { + _activeSocket.FullClose(); + } + + var r = new FakeAsyncResult() + { + AsyncState = token.AsyncState + }; + token.Callback(r); + } + } + finally + { + if (lockTaken) + { + _socketSyncLock.Exit(); + } + } + } + } + } + + public void EndConnect(IAsyncResult asyncResult) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + + var r = asyncResult as FakeAsyncResult; + if (r == null) + { + throw new ArgumentException("Invalid asyncResult.", nameof(asyncResult)); + } + + if (r.InternalException != null) + { + throw r.InternalException; + } + } + + public void Dispose() + { + if (_disposed) + { + return; + } + var lockTaken = false; + if (!_socketSyncLock.IsHeldByCurrentThread) + { + _socketSyncLock.TryEnter(ref lockTaken); + } + try + { + _disposed = true; + _activeSocket?.FullClose(); + } + finally + { + if (lockTaken) + { + _socketSyncLock.Exit(); + } + } + + } + + public IAsyncResult BeginSend(byte[] buffer, int offset, int size, SocketFlags socketFlags, + AsyncCallback callback, + object state) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (!Connected) + { + throw new SocketException((int) SocketError.NotConnected); + } + + return _activeSocket.BeginSend(buffer, offset, size, socketFlags, callback, state); + } + + public int EndSend(IAsyncResult asyncResult) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (!Connected) + { + throw new SocketException((int) SocketError.NotConnected); + } + + return _activeSocket.EndSend(asyncResult); + } + + public IAsyncResult BeginReceive(byte[] buffer, int offset, int size, SocketFlags socketFlags, + AsyncCallback callback, + object state) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (!Connected) + { + throw new SocketException((int) SocketError.NotConnected); + } + + return _activeSocket.BeginReceive(buffer, offset, size, socketFlags, callback, state); + } + + public int EndReceive(IAsyncResult asyncResult) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (!Connected) + { + throw new SocketException((int) SocketError.NotConnected); + } + + return _activeSocket.EndReceive(asyncResult); + } + + public void Shutdown(SocketShutdown how) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (!Connected) + { + return; + } + + _activeSocket.Shutdown(how); + } + + public void SetSocketOption(SocketOptionLevel optionLevel, SocketOptionName optionName, bool optionValue) + { + SetSocketOption(optionLevel, optionName, optionValue ? 1 : 0); + } + + public void SetSocketOption(SocketOptionLevel optionLevel, SocketOptionName optionName, int optionValue) + { + if (_disposed) + { + throw new ObjectDisposedException(GetType().FullName); + } + if (!Connected) + { + throw new SocketException((int)SocketError.NotConnected); + } + + _activeSocket.SetSocketOption(optionLevel, optionName, optionValue); + } + } +} diff --git a/shadowsocks-csharp/shadowsocks-csharp.csproj b/shadowsocks-csharp/shadowsocks-csharp.csproj index ba6cbec5..46a5c89b 100644 --- a/shadowsocks-csharp/shadowsocks-csharp.csproj +++ b/shadowsocks-csharp/shadowsocks-csharp.csproj @@ -179,7 +179,8 @@ - + + Form From 5e082e2961324a9dd62977af511483d914a6c6fe Mon Sep 17 00:00:00 2001 From: noisyfox Date: Sat, 17 Sep 2016 15:12:48 +1000 Subject: [PATCH 2/4] Fix comment Signed-off-by: noisyfox --- shadowsocks-csharp/Util/Sockets/WrappedSocket.cs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs b/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs index 93a5f7a6..85971ced 100644 --- a/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs +++ b/shadowsocks-csharp/Util/Sockets/WrappedSocket.cs @@ -12,17 +12,14 @@ namespace Shadowsocks.Util.Sockets * If the server address is host name, then it may have both ipv4 and ipv6 address * after resolving. The main idea is we don't want to resolve and choose the address * by ourself. Instead, Socket.ConnectAsync() do handle this thing internally by trying - * each address and returning an established socket connection. Such approach solves - * two problem: - * 1. Async DNS resolving. - * 2. + * each address and returning an established socket connection. */ public class WrappedSocket { public EndPoint LocalEndPoint => _activeSocket?.LocalEndPoint; + // Only used during connection and close, so it won't cost too much. private SpinLock _socketSyncLock = new SpinLock(); - // Only used during connection and close, so it won't cost too much. private volatile bool _disposed; private bool Connected => _activeSocket != null; From 390144351a8a03ca9d0dc832bf2c9bfac15263dd Mon Sep 17 00:00:00 2001 From: noisyfox Date: Sat, 17 Sep 2016 16:08:51 +1000 Subject: [PATCH 3/4] Fix #710 When using http proxy, this will cause distinct latency if we are trying to resolve localhost. Currently it's unnecessary to do that because our privoxy doesn't listern on ipv6 address, so it's ok to hard code the ipv4 address. There is a plan to add a cache to record the ipv6 connectivity so that we needn't to try both ipv4 and ipv6 every time when we attempt to connect a DnsEndPoint. --- shadowsocks-csharp/Controller/Service/PortForwarder.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shadowsocks-csharp/Controller/Service/PortForwarder.cs b/shadowsocks-csharp/Controller/Service/PortForwarder.cs index 54ecd49d..5245bb7e 100644 --- a/shadowsocks-csharp/Controller/Service/PortForwarder.cs +++ b/shadowsocks-csharp/Controller/Service/PortForwarder.cs @@ -46,7 +46,7 @@ namespace Shadowsocks.Controller this._local = socket; try { - EndPoint remoteEP = SocketUtil.GetEndPoint("localhost", targetPort); + EndPoint remoteEP = SocketUtil.GetEndPoint("127.0.0.1", targetPort); // Connect to the remote endpoint. _remote = new WrappedSocket(); From 063cf111cb2f45c55fcffd1c418ff3e90c33d9ae Mon Sep 17 00:00:00 2001 From: noisyfox Date: Sat, 17 Sep 2016 16:08:51 +1000 Subject: [PATCH 4/4] Fix a bug that a TCPHandler may be shutdown immediately after creation. Becuase we put handler.Start() call after timeout check and lastActivity is initialized in Start(), this will lead to a incorrect timeout. Also because we will call handler.Start() right after the constructor and timeout check, so it makes no difference where we put the initializtion of lastActivity. We won't do this again in Start() because DateTime.Now will consumes a lot of cpu time. --- shadowsocks-csharp/Controller/Service/TCPRelay.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/shadowsocks-csharp/Controller/Service/TCPRelay.cs b/shadowsocks-csharp/Controller/Service/TCPRelay.cs index 2a2b60a7..405d2795 100644 --- a/shadowsocks-csharp/Controller/Service/TCPRelay.cs +++ b/shadowsocks-csharp/Controller/Service/TCPRelay.cs @@ -173,6 +173,8 @@ namespace Shadowsocks.Controller this._config = config; this._tcprelay = tcprelay; this._connection = socket; + + lastActivity = DateTime.Now; } public void CreateRemote() @@ -195,7 +197,6 @@ namespace Shadowsocks.Controller _firstPacket = firstPacket; _firstPacketLength = length; HandshakeReceive(); - lastActivity = DateTime.Now; } private void CheckClose()