ClickHouse / ClickHouse/clickhouse-cpp

Socket closed without `shutdown()` causes TIME_WAIT accumulation and data loss risk

Cerrado
#488 3 comentarios 0 reacciones 1 asignado Asignado a @slabko Ver en GitHub
Lenguaje dominante
C
Estrellas
382
Forks
209
Merge medio
1 d 22 h
PR fusionados (30 d)
13

Descripción

# Socket closed without `shutdown()` causes TIME_WAIT accumulation and data loss risk

## Summary

The clickhouse-cpp client calls `close()` / `closesocket()` directly on the TCP socket without first calling `shutdown()`. This is an improper socket teardown that causes two problems:

1. **TIME_WAIT socket accumulation** on the client side when the client initiates the close, which can exhaust ephemeral ports under high connection churn.
2. **Potential data loss** — `close()` on a socket with unread data in the receive buffer causes the OS to send a RST instead of a graceful FIN, which can discard in-flight data and confuse the peer.

The issue affects both plain TCP and SSL connections (no `SSL_shutdown()` either).

## Root Cause

`Socket::Close()` in `socket.cpp`:

```cpp
void Socket::Close() {
CloseSocket(handle_);
handle_ = INVALID_SOCKET;
}
```

Where `CloseSocket` is:

```cpp
void CloseSocket(SOCKET socket) {
if (socket == INVALID_SOCKET)
return;
#if defined(_win_)
closesocket(socket);
#else
close(socket);
#endif
}
```

There is **no** `shutdown(fd, SHUT_RDWR)` or `shutdown(fd, SHUT_WR)` call before `close()` anywhere in the library. A search of the entire `clickhouse/` source tree confirms zero uses of `shutdown()`.

For SSL sockets, `SSLSocket` inherits from `Socket` and its destructor chain also never calls `SSL_shutdown()` before closing the underlying file descriptor.

## Affected Code Paths

Every socket close goes through `Socket::Close()` → `CloseSocket()`:

- `Client::~Client()` → `~Impl()` → `~socket_` → `Socket::~Socket()` → `Close()`
- `Client::Impl::ResetConnection()` → `InitializeStreams()` → old socket swapped out → `~Socket()`
- `SocketRAIIWrapper::~SocketRAIIWrapper()` in `SocketConnect()` (on connection failure)

## Consequences

### 1. TIME_WAIT accumulation

When the client closes first without `shutdown()`, the TCP stack enters TIME_WAIT on the client side. Under high connection churn (many short-lived client instances), this can exhaust the available ephemeral port range (~28,000 ports on Linux default), causing `connect()` failures with `EADDRNOTAVAIL`.

### 2. RST instead of graceful FIN

Per POSIX/RFC 793, calling `close()` on a socket that has unread data in the kernel receive buffer causes the OS to send a TCP RST to the peer instead of a graceful FIN. This can happen when:

- The server sent data the client never read (e.g., client destroys `Client` object mid-query)
- The server sent an exception or log packet the client didn't consume

The RST causes the peer to see `ECONNRESET`, and any data the client sent that is still in-flight may be discarded by the peer's TCP stack.

### 3. SSL session not terminated properly

For SSL connections, `SSL_shutdown()` is never called. This means the TLS close_notify alert is never sent, which:
- Prevents SSL session resumption/caching
- May cause the server to log TLS errors
- Violates the TLS specification (RFC 5246 §7.2.1)

## Reproduction

```cpp
void tcp_server(std::atomic& running, int port) {
int server_fd = socket(AF_INET, SOCK_STREAM, 0);
// ... bind, listen ...
while (running) {
int client_fd = accept(server_fd, nullptr, nullptr);
if (client_fd >= 0) {
// Read Hello, then close properly from server side
recv(client_fd, buffer, sizeof(buffer), 0);
shutdown(client_fd, SHUT_RDWR);
close(client_fd);
}
}
}

// Client side — creates and destroys many connections
for (int i = 0; i < 15000; ++i) {
try {
clickhouse::Client client(opts);
} catch (...) {}
}

// Check TIME_WAIT count — will be very high
// Linux: grep "06 " /proc/net/tcp | wc -l
```

## Suggested Fix

Add `shutdown()` before `close()` in `Socket::Close()`:

```cpp
void Socket::Close() {
if (handle_ == INVALID_SOCKET)
return;

#if defined(_win_)
shutdown(handle_, SD_BOTH);
closesocket(handle_);
#else
shutdown(handle_, SHUT_RDWR);
close(handle_);
#endif
handle_ = INVALID_SOCKET;
}
```

For SSL sockets, add `SSL_shutdown()` in the `SSLSocket` destructor before the base `Socket::Close()` runs:

```cpp
SSLSocket::~SSLSocket() {
if (ssl_) {
SSL_shutdown(ssl_.get());
}
}
```

Note: `shutdown()` may fail if the socket is already in an error state — the return value can be safely ignored here since we are closing regardless.

## Environment

- **Library**: clickhouse-cpp (all current versions)
- **Affected platforms**: All (Linux, macOS, Windows)
- **Affected files**: `clickhouse/base/socket.cpp`, `clickhouse/base/sslsocket.cpp`

Guía de contribución

No hay ninguna guía de contribución indexada para este repositorio

Evaluación

Este issue todavía no se ha evaluado.

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.