Fix: tcp relay

[nange]

Fixes: #218

[nange]

@xjasonlyu PTAL.

[xjasonlyu]

#218 这个问题我没仔细看，理论上不应该会有问题🤔

不过需要注意的是，在tun2socks里relay的时候，有一侧并不是TCPConn，而且它似乎也不支持CloseWrite。

[nange]

嗯。 考虑了的，加上了这个方法：

func (tt *tcpTracker) CloseWrite() error {
	return tt.Conn.(*net.TCPConn).CloseWrite()
}

CloseWrite在本质上是给对方发送FIN，告诉对方，自己已经发送完毕了，是需要这样做的。 之前直接设置一个超时时间，可能提前退出了对远端连接的数据读取是不对的，应该需要等待对方发送FIN才能结束，即让io.CopyBuffer自己退出，而不是设置超时然后退出。

#218 这个问题我没仔细看，理论上不应该会有问题🤔

不过需要注意的是，在tun2socks里relay的时候，有一侧并不是TCPConn，而且它似乎也不支持CloseWrite。

[xjasonlyu]

当时加超时机制主要是因为，有时候有一端明明已经断开了，但是netstack的连接还是没有正确关闭，即出现了连接永久挂起的问题。不知道这个解决方案会不会有这个问题。

[nange]

当时加超时机制主要是因为，有时候有一端明明已经断开了，但是netstack的连接还是没有正确关闭，即出现了连接永久挂起的问题。不知道这个解决方案会不会有这个问题。

从你描述的问题以及代码看，应该是因为一方关闭断开后（其中一个io.CopyBuffer会退出），由于没有调用CloseWrite方法，造成对方不知道另一方已经关闭了，自己就一直没有关闭连接引起（另一个io.CopyBuffer可能一直未完成退出）。

目前的解决方案，不会有这个问题。

[xjasonlyu]

不过我参考类似v2ray里的relay代码，他们也是用这种的timeout机制的。大多数客户端一般就直接close，较少才会closeWrite/Read分开。

[nange]

我测试了v2ray也有同样的bug。 绝大部分应用层代码都只需要调用Close即可（因为大多数时候，都是在业务逻辑处理完成后，调用一次Close即可，后续就直接退出了，没有后续逻辑要处理了），只有做一些更底层的应用，一些较特殊的场景，可能会调用CloseWrite。 而Tun2socks是需要处理所有通用场景，因此需要考虑使用。

[xjasonlyu]

有道理，我再多测试一下！

[xjasonlyu]

看了一下现在的Clash relay代码，也是使用的类似设置超时的方式。我看看是否也存在这个问题。

https://github.com/Dreamacro/clash/blob/fbf2f265160819116dabfb2b2427d5c58aa11767/common/net/relay.go#L9-L24

[xjasonlyu]

OK，我测试了几次，感觉这么写似乎没有问题！

[xjasonlyu]

然后我觉得，可以在closeWrite的同时也setReadDeadline。

另外，我觉得不需要再在tracker里添加closeWrite方法，直接在relay里通过一个closeWriter接口判断能不能调用，可以的话直接调用就行了。

[nange]

然后我觉得，可以在closeWrite的同时也setReadDeadline。

另外，我觉得不需要再在tracker里添加closeWrite方法，直接在relay里通过一个closeWriter接口判断能不能调用，可以的话直接调用就行了。

1. 同时setReadDeadline我也OK，但我觉得应该是作为兜底方案来看待。所以我建议把这个超时时间设置为足够长，比如30min。 你觉得多少合适？
2. 不在tracker里添加closeWrite方法的话，这个功能如何生效呢？在relay判断的话， left和right肯定都是不满足接口断言的。

我按自己的理解重新优化了一下代码，你再看看。

[nange]

关于setReadDeadline，我想到一个更优的方法，你看看如何：

原理：如果tcpTracker内部的Conn支持CloseWrite能力，则relay里面就不需要调用setReadDeadline方法。反之则调用setReadDeadline方法，保持和你之前的实现一致。

实现：给tcpTracker添加CloseWrite方法，内部实现通过接口断言，看Conn字段是否满足CloseWrite接口，如果满足则正常调用，如果不满足，则返回一个错误。在relay中，调用CloseWrite方法，判断返回error不为空，则需要调用setReadDeadline，并且时间设置为之前的5s。

@xjasonlyu

[xjasonlyu]

我昨天一直在找有没有什么更优雅的solution，然后参考了一些代码，比如 cloudflared：

https://github.com/cloudflare/cloudflared/blob/be64362fdb2a2da481f8e0414f75de3db2ccdf32/stream/stream.go#L43

它这个似乎直接就一侧完成以后就退出close掉了，我有点迷惑。

[nange]

我昨天一直在找有没有什么更优雅的solution，然后参考了一些代码，比如 cloudflared：

https://github.com/cloudflare/cloudflared/blob/be64362fdb2a2da481f8e0414f75de3db2ccdf32/stream/stream.go#L43

它这个似乎直接就一侧完成以后就退出close掉了，我有点迷惑。

看上去它也没有处理CloseWrite的问题，不知道是不是cloudflared大部分场景都是用于HTTP的请求协议，HTTP请求过来的话，本身也没办法使用CloseWrite（无法支持），然后它把到目标地址的连接直接关闭了，HTTP服务这边是可以自动重用连接的。

[nange]

关于setReadDeadline，我想到一个更优的方法，你看看如何：

原理：如果tcpTracker内部的Conn支持CloseWrite能力，则relay里面就不需要调用setReadDeadline方法。反之则调用setReadDeadline方法，保持和你之前的实现一致。

实现：给tcpTracker添加CloseWrite方法，内部实现通过接口断言，看Conn字段是否满足CloseWrite接口，如果满足则正常调用，如果不满足，则返回一个错误。在relay中，调用CloseWrite方法，判断返回error不为空，则需要调用setReadDeadline，并且时间设置为之前的5s。

@xjasonlyu

我按上面说的优化方案，更新了一版代码。

[xjasonlyu]

我看看。

同时setReadDeadline我也OK，但我觉得应该是作为兜底方案来看待。所以我建议把这个超时时间设置为足够长，比如30min。 你觉得多少合适？

另外，这里我觉得可以把tcp wait timeout变成一个cli 参数，这块我先去调整一下。

[xjasonlyu]

我发现目前的实现，似乎还是会出现这个PR #155 (comment) 里提到的问题。

[nange]

我发现目前的实现，似乎还是会出现这个PR #155 (comment) 里提到的问题。

怎么能重现能说一下吗，我去试试。

[xjasonlyu]

似乎是用的ftp命令行操作，不过具体我也不是很清楚。

[xjasonlyu]

等下，我忽然想到一个问题，不知道你之前测试是基于哪个代理的；我之前做的所有测试用的代理，其实都是direct://，但这理论上不能算是代理。

所以在我们本地实现的closeWrite/Read，如果对面代理还是直接关闭的话，似乎意义就不大了？

[nange]

等下，我忽然想到一个问题，不知道你之前测试是基于哪个代理的；我之前做的所有测试用的代理，其实都是direct://，但这理论上不能算是代理。

所以在我们本地实现的closeWrite/Read，如果对面代理还是直接关闭的话，似乎意义就不大了？

是的。如果对面代理不支持closeWrite就没用了。我之前测试是基于direct://，以及我自己写的代理：easyss。

但是首先要tun2socks这边支持，整个链条才可能通，如果对方代理不支持也是没有什么影响的，就回退到了原来的样子。

[xjasonlyu]

我发现目前的实现有个问题：

假设，服务端是读取来自客户端请求，然后关闭连接，

func server() {
	listener, err := net.Listen("tcp", ":9878")
	if err != nil {
		log.Fatal(err)
	}

	// defer listener.Close()

	conn, err := listener.Accept()
	if err != nil {
		log.Fatal("server", err)
		os.Exit(1)
	}
	fmt.Println("connected from:", conn.RemoteAddr())
	data := make([]byte, 1)
	for {
		if _, err := conn.Read(data); err != nil {
			log.Println("server:", err)
			break
		}
	}
	conn.(*net.TCPConn).Close()
}

客户端出于某种原因，连接上之后没有任何读写操作就直接关闭连接，

func client() {
	conn, err := net.Dial("tcp", "localhost:9878")
	if err != nil {
		log.Fatal("client", err)
	}
	// time.Sleep(3*time.Second)
	conn.Close()
}

正常直连情况下，服务端读到EOF，就会自动断开：

connected from: [::1]:57079
2023/04/02 21:22:49 server: EOF

但是经过tun2socks之后，按照目前的PR实现：copyBuffer(right, left)这侧由于left已关闭，会返回EOF，调用right.CloseWrite()并退出；另一侧copyBuffer(left, right)由于服务端不会主动关闭连接，则会一直卡着，造成tcp连接泄漏，这样肯定是不行的。

[nange]

没明白呢，调用right.CloseWrite()后，服务端不就收到EOF，然后服务端就关闭连接了啊，然后copyBuffer(left, right)不就收到EOF，正常退出了？

[xjasonlyu]

Sorry我看走眼了。

不过我参考了一下其他的一些solution，如：https://www.excentis.com/blog/tcp-half-close-a-cool-feature-that-is-now-broken/

We suspect that some NAT devices, when seeing a FIN message, will delete the corresponding NAT entry after a short timeout even if no FIN is seen from the other side. The result is that TCP connections that are in a half–close state will stop working after a while.

I should mention that this behavior is not allowed by RFC 5382: The closing phase begins when both endpoints have terminated their half of the connection by sending a FIN packet. (The RFC also allows an idle-timeout for inactive connections after two hours and 4 minutes. But that rule did not apply in our situation because our TCP flow had only been running for a few seconds and it wasn’t idle.) So maybe this was just a bug in the NAT device implementation.

But it’s not only NAT devices that can cause problems. Many firewalls also implement a TCP half-close timeout. These timeouts can be very short: Cisco decreased the minimum half-close timeout to 30 seconds to provide better DoS protection.

And even the Linux operating system implements a half-close timeout. The default value is 60 seconds.

我觉得CloseWrite/Read可以增加进去，然后CloseWrite之后也必须要调用一次SetReadDeadline，时长使用Linux默认的60s。你觉得呢？

[xjasonlyu]

我按照上面的思路重新实现了一下，试了一下效果还不错，你可以试试

[nange]

我看了你给的参考链接，确实考虑到可能的拒绝攻击，设置一个相对较小的超时时间是合理的。
不过我在Linux上用这个PR关联issue里面的测试代码测试了，并不会出现文章里面说的，60s超时时间， 我把服务器的sleep时间调高到180s，客户端依旧能获取到服务器返回的信息。你可以试试看，是否我的测试有问题？

最新的代码，整体我也比较赞同，有一个点我觉得需要讨论，就是CloseRead的调用有必要吗？我目前没有看出有调用此方法的必要，因为这个方法的调用只是会影响代码层面对连接的读取，目前代码层面也没有和这个相关连的逻辑。

[xjasonlyu]

我看了你给的参考链接，确实考虑到可能的拒绝攻击，设置一个相对较小的超时时间是合理的。
不过我在Linux上用这个PR关联issue里面的测试代码测试了，并不会出现文章里面说的，60s超时时间， 我把服务器的sleep时间调高到180s，客户端依旧能获取到服务器返回的信息。你可以试试看，是否我的测试有问题？

这块考虑是测试的原因？不是很清楚。

最新的代码，整体我也比较赞同，有一个点我觉得需要讨论，就是CloseRead的调用有必要吗？我目前没有看出有调用此方法的必要，因为这个方法的调用只是会影响代码层面对连接的读取，目前代码层面也没有和这个相关连的逻辑。

考虑到对称一下，也没什么影响。

[nange]

考虑到对称一下，也没什么影响。

嗯，问题不大，主要是个审美偏好，我是如果这个代码不是必须的，就会删除。 看你，我都能接受。

[xjasonlyu]

嗯，另一方面go-tun2socks这个项目的这里也是有类似实现的：

func (h *tcpHandler) relay(lhs, rhs net.Conn) {
	upCh := make(chan struct{})

	cls := func(dir direction, interrupt bool) {
		lhsDConn, lhsOk := lhs.(duplexConn)
		rhsDConn, rhsOk := rhs.(duplexConn)
		if !interrupt && lhsOk && rhsOk {
			switch dir {
			case dirUplink:
				lhsDConn.CloseRead()
				rhsDConn.CloseWrite()
			case dirDownlink:
				lhsDConn.CloseWrite()
				rhsDConn.CloseRead()
			default:
				panic("unexpected direction")
			}
		} else {
			lhs.Close()
			rhs.Close()
		}
	}
...

[xjasonlyu]

感觉没什么问题，直接Merge了 :-P

[nange]

还想到一个点，io.CopyBuffer需要慎用，容易出现无谓的内存申请。

最好做一下接口断言，如果满足对应接口，则应该直接调用io.Copy。

[xjasonlyu]

io.Copy和io.CopyBuffer底层都是io.copyBuffer。

而且这里没什么需要担心的，因为tcpTracker和core.tcpConn都不存在ReadFrom和WriteTo方法；其次，调用这两个方法是为了实现ZeroCopy，但是前提是必须两端都是*net.TCPConn或*os.File，而gVisor那侧必定不是。

[nange]

Ok，明白。