From 3d38fbb9876c3d71b775c1f824d0cd6fa1eca467 Mon Sep 17 00:00:00 2001 From: Pure Tech Date: Wed, 2 Sep 2026 07:47:54 -0400 Subject: [PATCH 1/4] Fix stale errno handling when recv returns 0 When recv returns 0 on a clean peer close, errno is not required to be updated. Avoid surfacing a stale value by reporting ECONNRESET on POSIX and WSAECONNRESET on Windows. Add a regression test covering the stale errno case. Fixes #487 --- clickhouse/base/socket.cpp | 6 ++++- ut/socket_ut.cpp | 53 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/clickhouse/base/socket.cpp b/clickhouse/base/socket.cpp index dad6e4d32..32d859b17 100644 --- a/clickhouse/base/socket.cpp +++ b/clickhouse/base/socket.cpp @@ -436,7 +436,11 @@ size_t SocketInput::DoRead(void* buf, size_t len) { } if (ret == 0) { - throw std::system_error(getSocketErrorCode(), getErrorCategory(), "closed"); +#if defined(_win_) + throw std::system_error(WSAECONNRESET, getErrorCategory(), "connection closed by peer"); +#else + throw std::system_error(ECONNRESET, getErrorCategory(), "connection closed by peer"); +#endif } throw std::system_error(getSocketErrorCode(), getErrorCategory(), "can't receive string data"); diff --git a/ut/socket_ut.cpp b/ut/socket_ut.cpp index ee5315441..8689c14bd 100644 --- a/ut/socket_ut.cpp +++ b/ut/socket_ut.cpp @@ -129,3 +129,56 @@ TEST(Socketcase, connecttimeout) { // auto input = socket.makeInputStream(); // input->Read(buffer, sizeof(buffer)); //} + +#if !defined(_win_) +# include +# include + +// Regression test for issue #487. +// +// On a clean peer close, `recv()` returns 0, which is EOF, not an error. +// POSIX does NOT require `errno` to be set when `recv()` returns 0, so reading +// `errno` at that point yields a stale value from a previous syscall. Prior +// to the fix, `SocketInput::DoRead` surfaced that stale `errno` to the caller +// (e.g. "Operation now in progress" if the last failing call was the +// non-blocking `connect()`), making the exception message non-deterministic +// and misleading. +// +// The fix reports `ECONNRESET` with a fixed message instead. This test +// drives a clean close via `socketpair(2)`, seeds `errno` to a known value +// that is NOT `ECONNRESET`, and asserts the resulting exception's +// `error_code` is exactly `ECONNRESET`. +TEST(Socketcase, recvReturnsZeroReportsConnResetNotStaleErrno) { + int sv[2]; + ASSERT_EQ(0, ::socketpair(AF_UNIX, SOCK_STREAM, 0, sv)); + + // Seed `errno` to a known stale value that must NOT leak into the + // exception. On Linux, closing an invalid fd sets `errno = EBADF` (9), + // which is clearly distinct from `ECONNRESET` (104). + if (::close(-1) != -1) { + // Sanity guard: `close(-1)` must fail; if it doesn't, the test + // cannot guarantee `errno` is set as expected. + ::close(sv[0]); + ::close(sv[1]); + FAIL() << "close(-1) unexpectedly succeeded; cannot seed errno"; + } + ASSERT_EQ(EBADF, errno); + + SocketInput input(sv[0]); + // Close the peer side: the next `recv()` on sv[0] returns 0 (EOF). + ::close(sv[1]); + + char buf[16]; + try { + input.Read(buf, sizeof(buf)); + ::close(sv[0]); + FAIL() << "expected std::system_error on clean peer close"; + } catch (const std::system_error& e) { + ::close(sv[0]); + EXPECT_EQ(ECONNRESET, e.code().value()) + << "stale errno leaked into the exception: " << e.code().value(); + EXPECT_NE(EBADF, e.code().value()) + << "regression: stale errno was surfaced instead of ECONNRESET"; + } +} +#endif // !defined(_win_) From d2fd4006b8ba7ac4913b028214cc2ea69b3e2ce3 Mon Sep 17 00:00:00 2001 From: Pure Tech Date: Wed, 2 Sep 2026 09:44:00 -0400 Subject: [PATCH 2/4] Fix missing errno header in socket test --- ut/socket_ut.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/ut/socket_ut.cpp b/ut/socket_ut.cpp index 8689c14bd..f30cb9341 100644 --- a/ut/socket_ut.cpp +++ b/ut/socket_ut.cpp @@ -131,6 +131,7 @@ TEST(Socketcase, connecttimeout) { //} #if !defined(_win_) +# include # include # include From d2ed02a4e8f306b9891b71faaf19278ce4c29aba Mon Sep 17 00:00:00 2001 From: Pure Tech Date: Thu, 3 Sep 2026 09:29:19 -0400 Subject: [PATCH 3/4] Represent SocketInput::DoRead EOF as ProtocolError, not system_error When recv() returns 0 the connection was closed cleanly by the peer. The recv() syscall itself succeeded, so this is not an OS-level error and surfacing it as std::system_error was misleading. At the application level the decoder expected more protocol bytes and the connection ended instead, which is a truncated-data / protocol decoding failure. Throw clickhouse::ProtocolError on recv() == 0. The recv() < 0 branch is unchanged and continues to use std::system_error with the real errno/Winsock code, which is the legitimate use of system_error for actual syscall failures. The regression test is updated to expect ProtocolError and to fail hard if a future change re-introduces std::system_error on this path. The errno-seeding step is no longer needed. Refs: #487 --- clickhouse/base/socket.cpp | 12 +++++---- ut/socket_ut.cpp | 50 ++++++++++++++++---------------------- 2 files changed, 28 insertions(+), 34 deletions(-) diff --git a/clickhouse/base/socket.cpp b/clickhouse/base/socket.cpp index 32d859b17..3ece9487d 100644 --- a/clickhouse/base/socket.cpp +++ b/clickhouse/base/socket.cpp @@ -1,6 +1,7 @@ #include "socket.h" #include "singleton.h" #include "../client.h" +#include "../exceptions.h" #include #include @@ -436,11 +437,12 @@ size_t SocketInput::DoRead(void* buf, size_t len) { } if (ret == 0) { -#if defined(_win_) - throw std::system_error(WSAECONNRESET, getErrorCategory(), "connection closed by peer"); -#else - throw std::system_error(ECONNRESET, getErrorCategory(), "connection closed by peer"); -#endif + // Clean peer close (EOF) before the requested number of bytes + // arrived: the underlying `recv()` succeeded, so this is not a + // syscall error. The decoder expected more protocol data and the + // connection ended instead, so surface this as a truncated-data + // / protocol decoding failure rather than a system_error. + throw ProtocolError("connection closed by peer while reading"); } throw std::system_error(getSocketErrorCode(), getErrorCategory(), "can't receive string data"); diff --git a/ut/socket_ut.cpp b/ut/socket_ut.cpp index f30cb9341..c275a07e1 100644 --- a/ut/socket_ut.cpp +++ b/ut/socket_ut.cpp @@ -137,34 +137,22 @@ TEST(Socketcase, connecttimeout) { // Regression test for issue #487. // -// On a clean peer close, `recv()` returns 0, which is EOF, not an error. -// POSIX does NOT require `errno` to be set when `recv()` returns 0, so reading -// `errno` at that point yields a stale value from a previous syscall. Prior -// to the fix, `SocketInput::DoRead` surfaced that stale `errno` to the caller -// (e.g. "Operation now in progress" if the last failing call was the -// non-blocking `connect()`), making the exception message non-deterministic -// and misleading. +// On a clean peer close, `recv()` returns 0, which is EOF, not a syscall +// error. POSIX does NOT require `errno` to be set when `recv()` returns 0, +// so reading `errno` at that point would yield a stale value from a previous +// syscall. The proper representation is a protocol-level / truncated-data +// failure, not a `std::system_error`: the underlying `recv()` succeeded; +// the decoder expected more protocol bytes and the connection ended instead. // -// The fix reports `ECONNRESET` with a fixed message instead. This test -// drives a clean close via `socketpair(2)`, seeds `errno` to a known value -// that is NOT `ECONNRESET`, and asserts the resulting exception's -// `error_code` is exactly `ECONNRESET`. -TEST(Socketcase, recvReturnsZeroReportsConnResetNotStaleErrno) { +// The fix throws `clickhouse::ProtocolError` on `recv() == 0`. The other +// `recv() < 0` path still uses `std::system_error` because that is an +// actual syscall failure. This test drives a clean close via `socketpair(2)` +// and asserts the resulting exception is exactly `ProtocolError` with a +// message indicating a peer-closed connection. +TEST(Socketcase, recvReturnsZeroReportsProtocolErrorNotStaleErrno) { int sv[2]; ASSERT_EQ(0, ::socketpair(AF_UNIX, SOCK_STREAM, 0, sv)); - // Seed `errno` to a known stale value that must NOT leak into the - // exception. On Linux, closing an invalid fd sets `errno = EBADF` (9), - // which is clearly distinct from `ECONNRESET` (104). - if (::close(-1) != -1) { - // Sanity guard: `close(-1)` must fail; if it doesn't, the test - // cannot guarantee `errno` is set as expected. - ::close(sv[0]); - ::close(sv[1]); - FAIL() << "close(-1) unexpectedly succeeded; cannot seed errno"; - } - ASSERT_EQ(EBADF, errno); - SocketInput input(sv[0]); // Close the peer side: the next `recv()` on sv[0] returns 0 (EOF). ::close(sv[1]); @@ -173,13 +161,17 @@ TEST(Socketcase, recvReturnsZeroReportsConnResetNotStaleErrno) { try { input.Read(buf, sizeof(buf)); ::close(sv[0]); - FAIL() << "expected std::system_error on clean peer close"; + FAIL() << "expected ProtocolError on clean peer close"; + } catch (const ProtocolError& e) { + ::close(sv[0]); + const std::string what = e.what(); + EXPECT_NE(what.find("closed"), std::string::npos) + << "expected message to mention 'closed', got: " << what; } catch (const std::system_error& e) { ::close(sv[0]); - EXPECT_EQ(ECONNRESET, e.code().value()) - << "stale errno leaked into the exception: " << e.code().value(); - EXPECT_NE(EBADF, e.code().value()) - << "regression: stale errno was surfaced instead of ECONNRESET"; + FAIL() << "recv()==0 must be reported as ProtocolError, not " + "std::system_error (got errno-style code " + << e.code().value() << "); stale errno regression"; } } #endif // !defined(_win_) From 6b74001a0746de5107c40be5a0194a2f14462176 Mon Sep 17 00:00:00 2001 From: Andrew Slabko Date: Wed, 16 Sep 2026 17:09:39 +0200 Subject: [PATCH 4/4] Handle `::recv` return value correctly & make `ReadEofThrowsProtocolError` cross-platform --- clickhouse/base/socket.cpp | 22 ++++---- ut/socket_ut.cpp | 113 +++++++++++++++++++++++-------------- 2 files changed, 84 insertions(+), 51 deletions(-) diff --git a/clickhouse/base/socket.cpp b/clickhouse/base/socket.cpp index 3ece9487d..8499e66b5 100644 --- a/clickhouse/base/socket.cpp +++ b/clickhouse/base/socket.cpp @@ -430,22 +430,24 @@ SocketInput::SocketInput(SOCKET s) SocketInput::~SocketInput() = default; size_t SocketInput::DoRead(void* buf, size_t len) { - const ssize_t ret = ::recv(s_, (char*)buf, (int)len, 0); - if (ret > 0) { - return (size_t)ret; + ssize_t ret = 0; + do { + ret = ::recv(s_, (char*)buf, (int)len, 0); + } while (ret < 0 && errno == EINTR); + + if (ret < 0) { + throw std::system_error(getSocketErrorCode(), getErrorCategory(), "can't receive string data"); } - if (ret == 0) { - // Clean peer close (EOF) before the requested number of bytes - // arrived: the underlying `recv()` succeeded, so this is not a - // syscall error. The decoder expected more protocol data and the - // connection ended instead, so surface this as a truncated-data - // / protocol decoding failure rather than a system_error. + if (ret == 0 && len != 0) { + // Server closed connection, the protocol-aware consumers must not read past EOF + // If that happens, this is probably an error either in the client or the server closed + // the connection prematurely. throw ProtocolError("connection closed by peer while reading"); } - throw std::system_error(getSocketErrorCode(), getErrorCategory(), "can't receive string data"); + return (size_t)ret; } bool SocketInput::Skip(size_t /*bytes*/) { diff --git a/ut/socket_ut.cpp b/ut/socket_ut.cpp index c275a07e1..f77924d52 100644 --- a/ut/socket_ut.cpp +++ b/ut/socket_ut.cpp @@ -1,8 +1,11 @@ #include "tcp_server.h" #include +#include +#include #include +#include #include #include #include @@ -13,6 +16,7 @@ # include #else # include +# include #endif using namespace clickhouse; @@ -130,48 +134,75 @@ TEST(Socketcase, connecttimeout) { // input->Read(buffer, sizeof(buffer)); //} -#if !defined(_win_) -# include -# include -# include +namespace { -// Regression test for issue #487. -// -// On a clean peer close, `recv()` returns 0, which is EOF, not a syscall -// error. POSIX does NOT require `errno` to be set when `recv()` returns 0, -// so reading `errno` at that point would yield a stale value from a previous -// syscall. The proper representation is a protocol-level / truncated-data -// failure, not a `std::system_error`: the underlying `recv()` succeeded; -// the decoder expected more protocol bytes and the connection ended instead. -// -// The fix throws `clickhouse::ProtocolError` on `recv() == 0`. The other -// `recv() < 0` path still uses `std::system_error` because that is an -// actual syscall failure. This test drives a clean close via `socketpair(2)` -// and asserts the resulting exception is exactly `ProtocolError` with a -// message indicating a peer-closed connection. -TEST(Socketcase, recvReturnsZeroReportsProtocolErrorNotStaleErrno) { - int sv[2]; - ASSERT_EQ(0, ::socketpair(AF_UNIX, SOCK_STREAM, 0, sv)); - - SocketInput input(sv[0]); - // Close the peer side: the next `recv()` on sv[0] returns 0 (EOF). - ::close(sv[1]); +// RAII wrapper for the socket +class ScopedSocket { +public: + explicit ScopedSocket(SOCKET socket) : handle(socket) {} - char buf[16]; - try { - input.Read(buf, sizeof(buf)); - ::close(sv[0]); - FAIL() << "expected ProtocolError on clean peer close"; - } catch (const ProtocolError& e) { - ::close(sv[0]); - const std::string what = e.what(); - EXPECT_NE(what.find("closed"), std::string::npos) - << "expected message to mention 'closed', got: " << what; - } catch (const std::system_error& e) { - ::close(sv[0]); - FAIL() << "recv()==0 must be reported as ProtocolError, not " - "std::system_error (got errno-style code " - << e.code().value() << "); stale errno regression"; + ~ScopedSocket() { + if (handle != static_cast(-1)) { +#if defined(_win_) + ::closesocket(handle); +#else + ::close(handle); +#endif + } } + + ScopedSocket(const ScopedSocket&) = delete; + ScopedSocket& operator=(const ScopedSocket&) = delete; + + const SOCKET handle; +}; + +} // namespace + +TEST(Socketcase, ReadEofThrowsProtocolError) { + const ScopedSocket listener(::socket(AF_INET, SOCK_STREAM, 0)); + ASSERT_NE(static_cast(-1), listener.handle); + + sockaddr_in address{}; + address.sin_family = AF_INET; + address.sin_addr.s_addr = htonl(INADDR_LOOPBACK); + + ASSERT_EQ(0, ::bind(listener.handle, reinterpret_cast(&address), sizeof(address))); + ASSERT_EQ(0, ::listen(listener.handle, 1)); + + socklen_t address_size = sizeof(address); + ASSERT_EQ(0, ::getsockname(listener.handle, reinterpret_cast(&address), &address_size)); + + const NetworkAddress client_address("127.0.0.1", std::to_string(ntohs(address.sin_port))); + const auto timeout = std::chrono::seconds(5); + Socket client(client_address, SocketTimeoutParams{timeout, timeout, timeout}); + + // The listener's backlog lets connect complete before accept, without a thread. + const ScopedSocket peer(::accept(listener.handle, nullptr, nullptr)); + ASSERT_NE(static_cast(-1), peer.handle); + + const std::string payload = "hello"; + SocketOutput output(peer.handle); + WireFormat::WriteBytes(output, payload.data(), payload.size()); + + auto input = client.makeInputStream(); + char buf[16]; + ASSERT_TRUE(WireFormat::ReadBytes(*input, buf, payload.size())); + ASSERT_EQ(payload, std::string(buf, payload.size())); + + // All data has been read; after FIN, the client's next nonempty recv returns 0 (EOF). +#if defined(_win_) + ASSERT_EQ(0, ::shutdown(peer.handle, SD_SEND)); +#else + ASSERT_EQ(0, ::shutdown(peer.handle, SHUT_WR)); +#endif + + // Seed an unrelated error; EOF must still be reported as ProtocolError. +#if defined(_win_) + ::WSASetLastError(WSAECONNRESET); +#else + errno = EIO; +#endif + + EXPECT_THROW(input->Read(buf, sizeof(buf)), ProtocolError); } -#endif // !defined(_win_)