From 22055fe12af21f9be7f0a7384d7cebbb04343e7b Mon Sep 17 00:00:00 2001 From: vjan-nie Date: Tue, 7 Jul 2026 13:51:06 +0200 Subject: [PATCH 1/2] refactor(server): remove the two unpolled send()s from the kernel The subject requires poll() before every accept/recv/send. Two best-effort send()s violated this literally: - disconnectClient(): final flush of _out before close() - acceptClient(): 'Server full' rejection at MAX_CLIENTS Both are removed; the kernel now has no send() outside the EPOLLOUT-gated path in handleClientOutput(). Consistent with the errno-after-I/O removal. Accepted regressions (a client disconnected in the same tick its reply was queued gets zero bytes): ERR_PASSWDMISMATCH (464), the 001-005 welcome burst on immediate post-registration QUIT, and the 'Server full' ERROR. Compliant recovery = deferred teardown, tracked as T4. Characterization: WrongPassword/NoPassword now assert recv()==0 (EOF), replacing a tolerant check that guarded nothing. Suite: 139/139. --- src/Server.cpp | 20 ++++++-------------- tests/test_integration.cpp | 27 +++++++++++++++++++-------- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/src/Server.cpp b/src/Server.cpp index da93d88..534aa8f 100644 --- a/src/Server.cpp +++ b/src/Server.cpp @@ -235,11 +235,12 @@ void Server::acceptClient() // Connection cap: reject gracefully instead of exhausting fds if (_clients.size() >= MAX_CLIENTS) { - const char rejection[] = "ERROR :Server full\r\n"; - send(clientFd, rejection, sizeof(rejection) - 1, 0); // best effort - close(clientFd); - Log::warn("connection rejected: MAX_CLIENTS reached"); - return; + /* Reject by closing only — no unpolled send() here either (T3, same rule + ** as the removed disconnect flush). A courtesy "Server full" line would + ** need the deferred-teardown machinery tracked as T4. */ + close(clientFd); + Log::warn("connection rejected: MAX_CLIENTS reached"); + return; } // Set non-blocking @@ -486,15 +487,6 @@ void Server::disconnectClient(int fd, const std::string &reason) for (size_t i = 0; i < _extensions.size(); ++i) _extensions[i]->onClientDisconnect(*this, *client, reason); - // Best-effort flush of replies queued before this disconnect (e.g. the - // 001-005 burst when the client QUITs immediately after registering). - // Single non-blocking send; whatever the kernel refuses is dropped. - if (client->hasPendingData()) - { - const std::string &pending = client->getSendBuffer(); - send(fd, pending.c_str(), pending.size(), 0); - } - _epollMask.erase(fd); removeFromEpoll(fd); close(fd); diff --git a/tests/test_integration.cpp b/tests/test_integration.cpp index b06156b..5068802 100644 --- a/tests/test_integration.cpp +++ b/tests/test_integration.cpp @@ -45,11 +45,18 @@ TEST_F(IntegrationTest, WrongPassword) tc.sendCmd("USER wrongpw 0 * :Test"); std::this_thread::sleep_for(std::chrono::milliseconds(300)); - std::string reply = tc.recvAll(); - /* Server queues 464 but may disconnect before flushing. - Either we see 464 or the connection was simply closed. */ - EXPECT_TRUE(reply.empty() || tc.hasNumeric(reply, "464")) - << "Expected either ERR_PASSWDMISMATCH or closed connection"; + /* T3 (Option A): disconnectClient() no longer drains _out before + close(fd). sendReply(464) and disconnectClient() are called + synchronously in the same path (completeRegistration) with no + epoll_wait in between, so the 464 structurally never arrives. + Deterministic contract: zero bytes delivered and the connection + closed (EOF) -- recv() checked for n == 0 rather than recvAll(), + which can't distinguish EOF from a timeout on a still-open socket. */ + char buf[64]; + ssize_t n = recv(tc.fd(), buf, sizeof(buf), 0); + EXPECT_EQ(n, 0) + << "Expected connection closed with zero bytes delivered " + "(464 is queued but never flushed before close)"; } TEST_F(IntegrationTest, NoPassword) @@ -61,9 +68,13 @@ TEST_F(IntegrationTest, NoPassword) tc.sendCmd("USER nopw 0 * :Test"); std::this_thread::sleep_for(std::chrono::milliseconds(300)); - std::string reply = tc.recvAll(); - EXPECT_TRUE(reply.empty() || tc.hasNumeric(reply, "464")) - << "Expected either ERR_PASSWDMISMATCH or closed connection"; + /* Same path as WrongPassword (!hasPassSent() hits the same branch in + completeRegistration) -- same deterministic contract. */ + char buf[64]; + ssize_t n = recv(tc.fd(), buf, sizeof(buf), 0); + EXPECT_EQ(n, 0) + << "Expected connection closed with zero bytes delivered " + "(464 is queued but never flushed before close)"; } TEST_F(IntegrationTest, UnregisteredCommand) From b1b8d3b408cbc04b9b2805fd172bae506736cb9c Mon Sep 17 00:00:00 2001 From: vjan-nie Date: Tue, 7 Jul 2026 13:51:26 +0200 Subject: [PATCH 2/2] docs: record T3 tradeoff and reconcile CLAUDE/COVERAGE with main - CLAUDE.md: fds register EPOLLIN only with on-demand EPOLLOUT (since T1); no disconnect flush (since T3); suite 139/139. - tests/COVERAGE.md: errno-after-I/O row resolved (T2), poll-before-send row resolved (T3, both unpolled sends removed); drop hardcoded line numbers. --- CLAUDE.md | 8 +++++--- tests/COVERAGE.md | 21 +++++++++++++-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 60a2017..98b748e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -40,13 +40,13 @@ The **server** compiles under **C++98** (`Makefile`). The **test suite** compile The event loop and all command handling live in a single **`Server`** instance (`src/Server.cpp`, `include/Server.hpp`): -- `Server::run()` — the **single** `epoll_wait` call (annotated in `src/Server.cpp`; epoll lifecycle/ctl ops live behind `libcpp98::Reactor`). Client fds register for `EPOLLIN | EPOLLOUT`. `EPOLLIN` → `handleClientInput`, `EPOLLOUT` → `handleClientOutput`. The loop also runs `checkTimeouts()` (PING/PONG keepalive, SENDQ sweep) and fires `onTick` on extensions every pass. +- `Server::run()` — the **single** `epoll_wait` call (annotated in `src/Server.cpp`; epoll lifecycle/ctl ops live behind `libcpp98::Reactor`). Client fds register for `EPOLLIN` only; `EPOLLOUT` is armed on demand — a per-tick `_epollMask` reconcile sweep in `run()` sets `EPOLLIN | EPOLLOUT` while a client has queued output and drops back to `EPOLLIN` once `_out` drains (an always-armed `EPOLLOUT` is level-triggered and would busy-loop). `EPOLLIN` → `handleClientInput`, `EPOLLOUT` → `handleClientOutput`. The loop also runs `checkTimeouts()` (PING/PONG keepalive, SENDQ sweep) and fires `onTick` on extensions every pass. - `_clients` (`map`) and `_channels` (keyed by **casemapped** name — display case lives in `Channel::_name`) are the live state. `Server` owns and frees these pointers, plus all registered extensions (deleted in reverse order). - `main.cpp` ignores `SIGPIPE`, traps `SIGINT`/`SIGTERM` into `Server::isRunning`, and calls `registerExtensions(server)` (tier-dependent) before `run()`. **Extension seam** (`include/ext/IServerExtension.hpp`): everything optional plugs in through this observer interface — hooks for lifecycle (`onServerStart`, `onTick`), client events (`onClientRegistered`, `onClientDisconnect`), channel events (`onJoin`, `onPart`), interception (`onCommand` — fired only where ERR_UNKNOWNCOMMAND would go, so extensions can add commands like `FILE` but never shadow RFC ones; `onPrivmsg` — fired per non-channel target so virtual participants claim messages; `reservesNick`), foreign fds (`ownsFd`/`onFdEvent` + public `Server::registerExternalFd` — how PlatformBus multiplexes its socket into the same epoll), and `onAudit` fan-out (`Server::audit()` → AuditLog extension). The kernel never names a concrete extension. -**I/O is fully buffered, never blocking.** `Client` delegates to `libcpp98::BufferedSocket` (512-byte line cap, 64 KiB SENDQ — overflow latches and the client is disconnected at the next sweep point, never mid-broadcast). Every extracted line passes one sanitizer stripping stray `\r`/`\0` (kills IRC line injection; `\x01` CTCP/DCC bytes pass untouched). Handlers never call `send()` directly — they queue via `Server::sendToClient`/`sendReply`/`Client::queueMessage`, drained on `EPOLLOUT` (plus a best-effort flush in `disconnectClient`). When iterating extracted messages, code re-checks `_clients.find(fd)` after each because a handler may have disconnected the client. +**I/O is fully buffered, never blocking.** `Client` delegates to `libcpp98::BufferedSocket` (512-byte line cap, 64 KiB SENDQ — overflow latches and the client is disconnected at the next sweep point, never mid-broadcast). Every extracted line passes one sanitizer stripping stray `\r`/`\0` (kills IRC line injection; `\x01` CTCP/DCC bytes pass untouched). Handlers never call `send()` directly — they queue via `Server::sendToClient`/`sendReply`/`Client::queueMessage`, drained on `EPOLLOUT`; `disconnectClient` deliberately does **not** flush before closing (see Known traps). When iterating extracted messages, code re-checks `_clients.find(fd)` after each because a handler may have disconnected the client. **Command dispatch** (`Server::dispatchCommand`) is a linear `if (cmd == ...)` chain, split across files by category: - `CommandRegistration.cpp` — CAP, PASS, NICK, USER, `completeRegistration` (timing-safe password check via `libcpp::str::eq_consttime`) @@ -69,7 +69,7 @@ Dispatch enforces a **registration gate**: only CAP/PASS/NICK/USER/QUIT/PONG run ## Testing -Tests use Google Test but also feed every result into **PostMan** (`vendor/PostMan.cpp`), a styled Unicode-table reporter — `tests/test_main.cpp` bridges the two via a custom `TestEventListener`. `tests/Makefile` builds all of `src/` *except* `main.cpp` (linking `tier_full.cpp` as the one `registerExtensions` definition). Protocol-level suites share `tests/TestHarness.hpp` (TCP `TestClient` + `IrcServerTest` fixture; subclass and override `portBase()` per suite, `onServerReady()` to inject probe extensions). Test files: `test_message`, `test_client`, `test_channel`, `test_bot`, `test_integration`, `test_robustness`, `test_security`, `test_filetransfer`, `test_extensions`, `test_libcpp98`. Suite is 138/138; PostMan's leak counter is atomic and `assertNoLeaks` takes `const char*` (a `std::string` argument would count itself as a leak — keep it that way). +Tests use Google Test but also feed every result into **PostMan** (`vendor/PostMan.cpp`), a styled Unicode-table reporter — `tests/test_main.cpp` bridges the two via a custom `TestEventListener`. `tests/Makefile` builds all of `src/` *except* `main.cpp` (linking `tier_full.cpp` as the one `registerExtensions` definition). Protocol-level suites share `tests/TestHarness.hpp` (TCP `TestClient` + `IrcServerTest` fixture; subclass and override `portBase()` per suite, `onServerReady()` to inject probe extensions). Test files: `test_message`, `test_client`, `test_channel`, `test_bot`, `test_integration`, `test_robustness`, `test_security`, `test_filetransfer`, `test_extensions`, `test_libcpp98`. Suite is 139/139; PostMan's leak counter is atomic and `assertNoLeaks` takes `const char*` (a `std::string` argument would count itself as a leak — keep it that way). ## Known traps @@ -81,4 +81,6 @@ Tests use Google Test but also feed every result into **PostMan** (`vendor/PostM reaps an RST is **timing-dependent** when `EPOLLIN` and `EPOLLHUP` arrive in the same event — do NOT assume `EPOLLERR|HUP` alone covered RST before that errno removal (`RobustnessTest.AbruptDisconnectViaRST` guards it now). +- **No flush-on-disconnect**: `disconnectClient()` closes the fd without a + final best-effort `send()` of whatever is still queued in `_out` — two unpolled best-effort send()s were removed (T3): the flush in disconnectClient() and the MAX_CLIENTS "Server full" rejection in acceptClient(). The kernel now has no send() outside the EPOLLOUT-gated path in handleClientOutput(). Known accepted regressions: ERR_PASSWDMISMATCH (464) / the 001-005 burst on immediate post-registration QUIT, and the "Server full" ERROR when at MAX_CLIENTS, no longer reach the client (socket closes with zero bytes). Compliant recovery = deferred teardown (T4). \ No newline at end of file diff --git a/tests/COVERAGE.md b/tests/COVERAGE.md index 132774d..0ca6a9f 100644 --- a/tests/COVERAGE.md +++ b/tests/COVERAGE.md @@ -2,7 +2,7 @@ Cross-references every item on the 42 evaluation sheet against what the repo actually proves today. Built from a scan of `tests/`, `scripts/audit.sh`, and -`src/` on `main` (`4e78d69`). +`src/`, current as of the T1–T3 kernel-compliance work. **Legend:** ✅ covered · 🟡 partial · 🔴 gap / risk · 🧭 manual-only (not automatable) @@ -17,8 +17,8 @@ FancyLogSink, shrinking the surface an evaluator can question. |---|---|---|---| | Makefile; compiles `-Wall -Wextra -Werror -std=c++98`; exec `ircserv` | ✅ | `audit.sh` §compile | Also `make mandatory/bonus/full` all build | | **Only one** poll/epoll/select | ✅ | `audit.sh` §single-poll; verified 1 `epoll_wait` (Server.cpp:158) | PlatformBus multiplexes into the same epoll — no 2nd wait | -| poll called before each accept/recv/send | 🟡 | architectural (all I/O is epoll-event-driven) | **Exception:** best-effort `send()` in `disconnectClient` (Server.cpp:503) is not preceded by a write event → decide defend vs remove | -| **errno not used to trigger action after recv/send/accept** | 🔴 | — (unaudited, untested) | **TOP RISK.** `errno==EAGAIN` checks after recv (289), send (326), accept (234). No retry-loop, but errno branches control flow. Refactor recommended (EPOLLERR/HUP already handles real errors) | +| poll called before each accept/recv/send | ✅ | architectural (all I/O is epoll-event-driven) | **Resolved (T3):** both unpolled best-effort sends removed (`disconnectClient` flush + `acceptClient` "Server full") — zero exceptions remain in the kernel | +| **errno not used to trigger action after recv/send/accept** | ✅ | `RobustnessTest.AbruptDisconnectViaRST` | **Resolved (T2):** errno branching removed from recv/send/accept; real errors reaped by the `EPOLLERR\|EPOLLHUP` branch in `run()`. Also cured a latent `EINTR` false-positive disconnect | | fcntl only `F_SETFL, O_NONBLOCK` | ✅ | `audit.sh` §fcntl; verified (Server.cpp:104,250) | PlatformBus fcntl also compliant, but it's an extra | --- @@ -87,11 +87,16 @@ FancyLogSink, shrinking the surface an evaluator can question. ## Prioritized action list (defense order) **P0 — eliminatory, do first (a gap here = 0, everything else is moot):** -1. **errno-after-I/O** (A). Audit → refactor recv/send/accept to not branch on - `errno` (rely on `EPOLLERR|EPOLLHUP`), or prepare an airtight justification. - Guard: `Robustness.AbruptDisconnect` must stay green. -2. **Un-polled `send()` in `disconnectClient`** (A/B). Decide: remove (accept - losing app-buffered QUIT/ERROR text) or defend as a teardown flush. +1. ✅ **errno-after-I/O** (A) — resolved (T2): errno branching removed from + recv/send/accept; real errors handled by the `EPOLLERR|EPOLLHUP` branch. + Guard: `RobustnessTest.AbruptDisconnectViaRST`. +2. ✅ **Un-polled `send()` in `disconnectClient`** — resolved via Option A + (T3): the flush was removed, closing the last literal exception to + "poll before every send" in the kernel. Accepted regression: 464 / + welcome-burst no longer reach a client disconnected in the same tick + they were queued (see `CLAUDE.md` "Known traps"). Deferred-teardown + recovery (Option C) is tracked as future work (T4), out of this plan's + scope. **P1 — scored / high-value robustness:** 3. **Non-operator denial tests** (E) — the operator score is 0–5 and the sheet