From 3d38fbb9876c3d71b775c1f824d0cd6fa1eca467 Mon Sep 17 00:00:00 2001 From: Pure Tech Date: Wed, 2 Sep 2026 07:47:54 -0400 Subject: [PATCH 1/3] 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 dad6e4d3..32d859b1 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 ee531544..8689c14b 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/3] 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 8689c14b..f30cb934 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/3] 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 32d859b1..3ece9487 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 f30cb934..c275a07e 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_)