Properly handle TCP CLOSE-WAIT state

[zonyitoo]

Discussion continue from #1190 .

The original issue author doesn't want to have any effective discussion any further.

[zonyitoo]

For reference, in go-shadowsocks2:

https://github.com/shadowsocks/go-shadowsocks2/blob/828576df0a9415d1f49e9433577389821e862f3c/tcp.go#L145-L166

It calls io.Copy on both direction. Set a Read timeout when one of them ends.

[zonyitoo]

shadowsocks-libev:

https://github.com/shadowsocks/shadowsocks-libev/blob/d83ace0f0d9c05656c13d66aa4a449bf70143254/src/local.c#L966-L971

Close the whole tunnel directly when recv() returns EOF.

[database64128]

I posted a lengthy analysis yesterday at zhboner/realm#87 (comment).

There's no need for a read timeout. TCP keepalive will terminate the connection when TCP_KEEPCNT is reached. The default is 9 on my system. You can add an option for it if you want.

[zonyitoo]

Because it is already in CLOSE-WAIT state, I don't think TCP_KEEPCNT could solve the problem.

[zonyitoo]

CLIENT -> LOCAL -> SERVER -> REMOTE

Base on the original discussion, sockets that are in CLOSE-WAIT states are LOCAL -> SERVER.

Sockets must be held in here:

shadowsocks-rust/crates/shadowsocks/src/relay/tcprelay/utils.rs

Lines 193 to 217 in bf5d9a9

	fn transfer_one_direction<A, B>(
	cx: &mut Context<'_>,
	state: &mut TransferState,
	mut r: Pin<&mut A>,
	mut w: Pin<&mut B>,
	) -> Poll<io::Result<u64>>
	where
	A: AsyncRead + AsyncWrite + Unpin + ?Sized,
	B: AsyncRead + AsyncWrite + Unpin + ?Sized,
	{
	loop {
	match state {
	TransferState::Running(buf) => {
	let count = ready!(buf.poll_copy(cx, r.as_mut(), w.as_mut()))?;
	*state = TransferState::ShuttingDown(count);
	}
	TransferState::ShuttingDown(count) => {
	ready!(w.as_mut().poll_shutdown(cx))?;
	*state = TransferState::Done(*count);
	}
	TransferState::Done(count) => return Poll::Ready(Ok(*count)),
	}
	}
	}

[zonyitoo]

shadowsocks-rust/crates/shadowsocks/src/relay/tcprelay/utils.rs

Lines 49 to 87 in bf5d9a9

	loop {
	// If our buffer is empty, then we need to read some data to
	// continue.
	if self.pos == self.cap && !self.read_done {
	let me = &mut *self;
	let mut buf = ReadBuf::new(&mut me.buf);
	ready!(reader.as_mut().poll_read(cx, &mut buf))?;
	let n = buf.filled().len();
	if n == 0 {
	self.read_done = true;
	} else {
	self.pos = 0;
	self.cap = n;
	}
	}
	// If our buffer has some data, let's write it out!
	while self.pos < self.cap {
	let me = &mut *self;
	let i = ready!(writer.as_mut().poll_write(cx, &me.buf[me.pos..me.cap]))?;
	if i == 0 {
	return Poll::Ready(Err(io::Error::new(
	io::ErrorKind::WriteZero,
	"write zero byte into writer",
	)));
	} else {
	self.pos += i;
	self.amt += i as u64;
	}
	}
	// If we've written all the data and we've seen EOF, flush out the
	// data and finish the transfer.
	if self.pos == self.cap && self.read_done {
	ready!(writer.as_mut().poll_flush(cx))?;
	return Poll::Ready(Ok(self.amt));
	}
	}
	}

1. poll_read (recv) never returns EOF?

Since the socket is already in CLOSE-WAIT state, poll_read should return EOF.

2. poll_write (send) always returns Polling?

It should return EPIPE.

3. Blocking on poll_flush?

Really?

[zonyitoo]

I found a scenario:

1. Server closes by killing the whole process directly. So all the existing connections should send RST to LOCAL. (One way to trigger CLOST-WAIT in the original issue.)
   • Maybe FIN? It depends on whether the server close sockets gracefully when exiting.
2. LOCAL receives FIN, poll_read returns EOF, otherwise if it is RST, it should return ConnectionReset.
   • FIN: If CLIENT is still running properly, there should be nothing left in the read buffer. So it would continue to call shutdown(SHUT_WR) on the CLIENT -> LOCAL socket. The LOCAL should send FIN to CLIENT (reference).
   • RST: Exit the whole copy process directly, which will eventually close() both sockets immediately, no orphan CLOSE-WAIT sockets should be left in this situation.
3. CLIENT's recv() should return EOF. But if it doesn't shutdown or close its socket, then the sockets in CLIENT and LOCAL will never reach its destruction process, which will eventually stay in CLOSE-WAIT state forever?

[database64128]

Because it is already in CLOSE-WAIT state, I don't think TCP_KEEPCNT could solve the problem.

Actually, on a second thought, there really is no problem to solve. We are all overthinking this because of the aggressive behavior of the spammer.

It is common for HTTP servers to actively close idle connections to save resources. When the proxy server receives a FIN from the remote server, it calls shutdown(SHUT_WR) on the accepted proxy connection and changes its state to FIN_WAIT1/2. Now, enter tcp_fin_timeout:

       tcp_fin_timeout (integer; default: 60; since Linux 2.2)
              This specifies how many seconds to wait for a final FIN packet before the socket is forcibly closed.  This is strictly a violation of the TCP specification, but required to prevent denial-of-service attacks.  In Linux 2.2, the default value was 180.

If the proxy server hasn't received a FIN from the client in tcp_fin_timeout, the connection will be forcibly terminated.

[database64128]

3. `CLIENT`'s `recv()` should return EOF. But if it doesn't `shutdown` or `close` its socket, then the sockets in `CLIENT` and `LOCAL` will never reach its destruction process, which will eventually stay in `CLOSE-WAIT` state forever?

Not for LOCAL. LOCAL has sent FIN and from its perspective the accepted connection is in FIN_WAIT2 and thus is subject to tcp_fin_timeout.

[zonyitoo]

Yes it is true, which is exactly the behavior of transfer_one_direction:

shadowsocks-rust/crates/shadowsocks/src/relay/tcprelay/utils.rs

Lines 204 to 215 in bf5d9a9

	match state {
	TransferState::Running(buf) => {
	let count = ready!(buf.poll_copy(cx, r.as_mut(), w.as_mut()))?;
	*state = TransferState::ShuttingDown(count);
	}
	TransferState::ShuttingDown(count) => {
	ready!(w.as_mut().poll_shutdown(cx))?;
	*state = TransferState::Done(*count);
	}
	TransferState::Done(count) => return Poll::Ready(Ok(*count)),
	}

It will call shutdown(SHUT_WR) on the writer stream (CLIENT <- LOCAL).

So if sockets are forcibly closed if CLIENT doesn't respond with FIN or RST, then another call of transfer_one_direction which manages CLIENT -> LOCAL should ends as expected.

If so, why did those CLOSE-WAIT sockets exist?

[database64128]

So if sockets are forcibly closed if CLIENT doesn't respond with FIN or RST, then another call of transfer_one_direction which manages CLIENT -> LOCAL should ends as expected.

If so, why did those CLOSE-WAIT sockets exist?

Because transfer_one_direction is still waiting to read from CLIENT, and it won't call shutdown(SHUT_WR) on LOCAL -> SERVER socket until the CLIENT -> LOCAL socket receives a FIN or runs out of tcp_fin_timeout.

[zonyitoo]

Since sockets are in CLOSE-WAIT state, the copy function should have already ended (the shutdown(SHUT_WR) must be called on both writer stream).

If these sockets are somehow still holding by some structs in the program, then it will stay in CLOSE-WAIT indefinitely.

Because transfer_one_direction is still waiting to read from CLIENT, and it won't call shutdown(SHUT_WR) on LOCAL -> SERVER socket until the CLIENT -> LOCAL socket receives a FIN or runs out of tcp_fin_timeout.

Yes. And it is 60 seconds by default.

But still, it should stay in FIN-WAIT* state for 60 seconds but not CLOSE-WAIT.

[database64128]

But still, it should stay in FIN-WAIT* state for 60 seconds but not CLOSE-WAIT.

FIN_WAIT1/2 is when our side has sent FIN and is waiting for more data or a FIN from the remote side.

From LOCAL's perspective, on LOCAL -> SERVER, we have already received a FIN, and we haven't called shutdown(SHUT_WR) yet, so it's in CLOSE_WAIT.

[zonyitoo]

So if a LOCAL -> SERVER socket stays in CLOSE-WAIT indefinitely, then the connection of CLIENT -> LOCAL must not exit properly. FIN haven't sent to CLIENT, otherwise the kernel behavior of tcp_fin_timeout will forcibly close the CLIENT -> LOCAL connection, which will eventually close the LOCAL -> SERVER socket in the current implementation.

In current implementation, it only happens if there are still data in the read buffer, but couldn't write() for some unknown reasons.

[f4nff]

But still, it should stay in FIN-WAIT state for 60 seconds but not CLOSE-WAIT.*

yes~!