From 91fe50d8be105649b79bf0b06391b5823f2e67a8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:56:01 +0000 Subject: [PATCH] fix(daemon): SIGABRT on shutdown, SIGCHLD=SIG_IGN breaking every exec, in-process runCrate on HTTP workers, fd CLOEXEC, IPv6-route gateway detect, stack DNS never stopped, physmem 500 (1.1.26) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven correctness fixes from a correctness-lens audit — "the daemon works": - server.cpp: the Unix-socket httplib::Server lived only in its thread's lambda; stop() never stopped it and ~Impl destroyed a joinable thread -> std::terminate on every clean `service crated stop`. Now an Impl member, stopped and joined. - ws_console.cpp: signal(SIGCHLD, SIG_IGN) was process-wide; on FreeBSD that makes waitpid() return ECHILD for ALL children, so every Util::execCommand*/execPipeline threw "waitpid failed" the moment console.port was enabled. Removed; session shells are reaped explicitly (SIGTERM, grace, SIGKILL + blocking wait). - routes.cpp: POST .../start and .../restart called runCrate() in-process on the httplib worker: blocked the worker for the jail's lifetime (~8 starts -> whole API incl. /healthz dead), installed SIGINT/SIGTERM handlers process-wide (`service crated stop` hung), and keyed jailXname/FwSlots/FwUsers on getpid() so all daemon-started jails shared one key (slot overwrite, NAT rule removed while jails ran). Now fork+setsid+exec `crate run` like the control-socket path; the response is async ({"started":true,"async":true,"pid":N}). - util.cpp/jail_query.cpp/control_socket.cpp/privops_listener.cpp/ ws_console.cpp: pipe2(O_CLOEXEC) for capture pipes and SOCK_CLOEXEC for the hand-rolled listeners, so long-lived forked children no longer inherit a pipe write end (request thread hung on EOF) or a LISTEN fd (EADDRINUSE on restart). - run_net.cpp detectGateway: `netstat -rn -f inet` and `>= 4` tokens — an IPv6 default route doubled the `default` lines and every NAT-mode `crate run` failed. - stack.cpp: unbound now foreground (do-daemonize: no) under setsid, and stopStackDns signals the pid from the per-network unbound.pid — DNS was never actually stopped before (orphan bound to :53). - routes.cpp/util.{h,cpp}: hw.physmem (ULONG) read via new getSysctlUInt64; the 4-byte read made GET /api/v1/host a permanent 500. All runtime-only, compile-gated by FreeBSD lite. Remaining audit items (httplib/accept fd hygiene, run.cpp teardown-after-throw leaks, FwUsers GC, listener timeouts, `stack up` in-process runCrate, LOW batch) recorded in TODO. Bumps to 1.1.26; CHANGELOG + trust-model. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01X6t6tzVypHye5bDGLxzmZK --- CHANGELOG.md | 80 +++++++++++++++++++++++++++++++++++++ TODO | 63 +++++++++++++++++++++++++++++ cli/args.cpp | 4 +- daemon/control_socket.cpp | 6 ++- daemon/privops_listener.cpp | 4 +- daemon/routes.cpp | 74 ++++++++++++++++++++++------------ daemon/server.cpp | 30 ++++++++++---- daemon/ws_console.cpp | 36 +++++++++++++++-- docs/trust-model.md | 4 +- docs/trust-model.uk.md | 4 +- lib/jail_query.cpp | 6 ++- lib/run_net.cpp | 10 ++++- lib/stack.cpp | 36 ++++++++++++++--- lib/util.cpp | 24 +++++++++-- lib/util.h | 4 ++ 15 files changed, 329 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c3f00b..cc5891c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,86 @@ Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). --- +## [1.1.26] — 2026-09-14 + +**Correctness: seven daemon/runtime fixes from a correctness-lens audit +(exception safety, process/fd hygiene, threading) — "the daemon works".** + +- **`crated` shut down with SIGABRT on every clean stop — `daemon/server.cpp` + (HIGH).** The Unix-socket `httplib::Server` lived only inside its + thread's lambda, so `Server::stop()` could never stop it, `listen()` + never returned, and `~Impl` destroyed a still-joinable `std::thread` → + `std::terminate`. With the default `unixSocket` configured this hit + every `service crated stop`. The UDS server is now an `Impl` member; + `stop()` stops it and joins its thread. + +- **Enabling `console.port` broke every exec in the daemon — + `daemon/ws_console.cpp` (HIGH).** `WsConsole::start` set + `signal(SIGCHLD, SIG_IGN)` process-wide. On FreeBSD that flags + `PS_NOCLDWAIT`: children are auto-reaped and `waitpid()` returns + `ECHILD` for **all** of crated's children, so every + `Util::execCommand*`/`execPipeline*` (privops verbs, stats, export/ + import, control-socket ops) threw "waitpid failed" even on success, and + `JailExec` decoded an uninitialized status. The SIG_IGN is gone; + session shells are reaped explicitly (SIGTERM, ~2s grace, then SIGKILL + + blocking wait). + +- **`POST …/start` and `…/restart` ran the container in-process — + `daemon/routes.cpp` (HIGH).** Both called `runCrate()` on the httplib + worker thread: (a) the worker blocked for the jail's lifetime — a + service-only container never returned, and after ~8 starts the whole + HTTP API incl. `/healthz` stopped responding; (b) `runCrate` installed + SIGINT/SIGTERM handlers process-wide, so `service crated stop` hung; + (c) `jailXname`/`FwSlots`/`FwUsers` are keyed on `getpid()`, so every + daemon-started jail shared one key — a later start overwrote the first + jail's firewall slot and the first teardown removed the shared NAT rule + while other jails still ran. Both routes now fork+setsid+exec + `crate run -f ` as its own process (the control-socket start + path already did this). **API note:** the response is now async — + `{"started":true,"async":true,"pid":N,…}` is returned as soon as the + child is spawned; poll `GET /api/v1/containers/:name` for state. + +- **fds leaked into long-lived children — `lib/util.cpp`, + `lib/jail_query.cpp`, `daemon/{control_socket,privops_listener,ws_console}.cpp` + (HIGH).** Capture pipes were created without `O_CLOEXEC`, so a pipe's + write end leaked into any child forked concurrently on another daemon + thread (the control-socket `crate run` supervisor, ws-console shells — + both long-lived), and the reading request thread waited for an EOF that + only came when *that* child exited. Listening sockets leaked the same + way (→ `EADDRINUSE` on `service crated restart`; a jailed shell holding + the host daemon's listener). Pipes now use `pipe2(O_CLOEXEC)` (dup2 + clears it on the intended child's stdio) and the three hand-rolled + listeners use `SOCK_CLOEXEC`. Not yet covered: httplib's own TCP + listener and `accept()`ed connection fds — recorded in `TODO`. + +- **NAT-mode `crate run` failed on any host with an IPv6 default route — + `lib/run_net.cpp` (MED).** `detectGateway` ran `netstat -rn` without + `-f inet`, so the inet6 table's `default` line doubled the token count + and the strict `!= 4` check threw "Unable to determine host's gateway". + Now `-f inet` and `>= 4`, mirroring the IPv6 query. + +- **Stack DNS was never stopped — `lib/stack.cpp` (MED).** unbound ran + with `do-daemonize: yes`, so the pid `startStackDns` returned was the + short-lived launcher (its 200ms liveness check then reported "exited + immediately" on *success*), and `stack down` passed `-1` because it is + a different process anyway → an orphan unbound stayed bound to + `:53` and the next `up` failed to bind. unbound now runs in the + foreground under `setsid`, and `stopStackDns` signals the pid from the + per-network `unbound.pid` file. + +- **`GET /api/v1/host` was a permanent 500 on 64-bit — `daemon/routes.cpp`, + `lib/util.cpp` (MED).** `hw.physmem` is `CTLTYPE_ULONG` (8 bytes) but + was read through the 4-byte `getSysctlInt` → `ENOMEM`. New + `Util::getSysctlUInt64`. + +All seven are runtime-only (daemon / FreeBSD runtime; no pure unit +surface) and are compile-gated by the FreeBSD lite build. Exercising +them on a live host is recommended, especially the async start/restart +semantics. The remaining correctness-audit items (fd hygiene for +httplib/accept fds, teardown-after-throw leaks in `run.cpp`, `FwUsers` +dead-pid GC, listener read timeouts, and a LOW batch) are listed in +`TODO`. + ## [1.1.25] — 2026-07-11 **Security & robustness: five fixes from a third-pass audit of the diff --git a/TODO b/TODO index da544ee..e7d4f75 100644 --- a/TODO +++ b/TODO @@ -100,6 +100,69 @@ harden BEFORE wiring this code up: for vmName/vol.tag before any of this gets a caller. memoryToKiB (vm_spec.cpp:84) also has an unchecked `val*1024*1024` overflow. +Remaining from the 2026-09 correctness-lens audit (its seven HIGH/MED +daemon items shipped as 1.1.26; these are what is left, all runtime-only +and best validated on a live FreeBSD host): + +* (correctness, MED) lib/stack.cpp `stack up` still calls runCrate() + IN-PROCESS and sequentially per container (the same defect 1.1.26 + fixed for the daemon's HTTP start/restart): a service-only first + container never returns, so the second is never started. Spawn each + `crate run` as its own process like daemon/routes.cpp spawnCrateRun. + +* (correctness, MED) fd hygiene, part 2: 1.1.26 put O_CLOEXEC on the + capture pipes and SOCK_CLOEXEC on the three hand-rolled listeners. + Still inherited by forked children: httplib's own TCP listener + (server.cpp — needs httplib::Server::set_socket_options or an + fcntl(FD_CLOEXEC) after bind) and every accept()ed connection fd in + control_socket.cpp / privops_listener.cpp / ws_console.cpp (use + accept4(SOCK_CLOEXEC)). Consequence today: a long-lived child can + pin another client's connection open. + +* (correctness, MED) lib/run.cpp registers several teardown handlers + AFTER the operations that can throw, so the error path leaks host + resources (distinct from the documented teardown-ORDER item above): + NetworkLease::allocateFor succeeds (~:1153) but configureStaticIp/ + copyFile throws before releaseLeaseAtEnd is set → the lease row for + the unique jail-- stays in network-leases.txt forever + (nothing reclaims it; clean.cpp does not touch leases); same for + IPv6 (~:1184). Bridge auto-created (~:1090) then setUp/ + createBridgeEpair throws before destroyBridgeEpairAtEnd.reset + (~:1115) → bridge + epair leak; run_net.cpp createBridgeEpair / + the extras loop (~:1664) leak an epairN pair per failed attempt. + Fix shape: register the RunAtEnd immediately after each successful + acquisition, before the next fallible step. + +* (correctness, MED) lib/ctx.cpp FwUsers has no dead-pid garbage + collection (FwSlots does, ctx.cpp:~200) and lib/clean.cpp:~120 + claims to GC it but only locks/unlocks. A `crate run` killed without + teardown (SIGKILL/OOM/panic) leaves its pid in ctx-fw-users, so + FwUsers::isEmpty() is never true again and the shared NAT rule 50000 + + net.inet.ip.forwarding are never restored (run_net.cpp:~488). Add + kill(pid,0)-based GC mirroring FwSlots and make clean.cpp call it. + +* (robustness, MED/LOW) daemon/privops_listener.cpp nvlist_recv and + daemon/control_socket.cpp read() block forever with no timeout, and + both accept loops spawn one detached thread per connection with no + cap — an idle client pins a thread + fd; N of them exhaust the + daemon. Add a recv timeout (SO_RCVTIMEO) and a concurrency cap. + +* (LOW batch) daemon/routes.cpp import tmp file is .crate.tmp. + — constant across threads, so two concurrent imports of one name + interleave writes; no fsync before rename. lib/util.cpp writeFile(data, + fd) closes the CALLER's fd on error (ctx.cpp then double-closes). + execPipelineImpl: fork() failure for child i>0 throws without reaping + children 0..i-1 (zombies). Capture loops treat read()==-1 as EOF + (EINTR) and jail_query.cpp ignores waitpid failure / decodes an + uninitialized status. lib/util.cpp ~:631/662 test `st_mode & S_IFREG/ + S_IFDIR` instead of S_ISREG/S_ISDIR (a socket matches S_IFDIR → + directory_iterator throws out of findElfFiles). daemon/routes.cpp + gmtime() (static buffer) in worker threads. ws_console.cpp select() + with fds >= FD_SETSIZE. run.cpp hcThread joinable while execInJail + can throw on fork EAGAIN → std::terminate skips every RunAtEnd. + run_services.cpp ignores fork() failure for socat. stack.cpp + inet_pton results unchecked for ip_range. + === High priority — blocking production use === (High-priority items above are now complete; the next batch is diff --git a/cli/args.cpp b/cli/args.cpp index a2dc906..b2cac0e 100644 --- a/cli/args.cpp +++ b/cli/args.cpp @@ -753,7 +753,7 @@ Args parseArguments(int argc, char** argv, unsigned &processed) { args.noColor = true; break; } else if (strEq(argv[a], "--version")) { - std::cout << "crate 1.1.25" << std::endl; + std::cout << "crate 1.1.26" << std::endl; exit(0); } else if (auto argShort = isShort(argv[a])) { switch (argShort) { @@ -764,7 +764,7 @@ Args parseArguments(int argc, char** argv, unsigned &processed) { args.logProgress = true; break; case 'V': - std::cout << "crate 1.1.25" << std::endl; + std::cout << "crate 1.1.26" << std::endl; exit(0); default: err("unsupported short option '%s'", argv[a]); diff --git a/daemon/control_socket.cpp b/daemon/control_socket.cpp index 64b8df7..ddb7b15 100644 --- a/daemon/control_socket.cpp +++ b/daemon/control_socket.cpp @@ -559,7 +559,11 @@ struct SocketRuntime { // printed but the bind continues. int bindSocketOrThrow(const ControlSocketPure::ControlSocketSpec &spec, long expectedGid) { - int fd = ::socket(AF_UNIX, SOCK_STREAM, 0); + // 1.1.26: SOCK_CLOEXEC — this listener must not be inherited by the + // long-lived `crate run` children handleStart forks (they held the + // LISTEN fd → EADDRINUSE on `service crated restart`, and the jail + // supervisor carried the host daemon's control socket). + int fd = ::socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); if (fd < 0) throw std::runtime_error(std::string("socket(AF_UNIX): ") + std::strerror(errno)); diff --git a/daemon/privops_listener.cpp b/daemon/privops_listener.cpp index 7ece00c..d875cec 100644 --- a/daemon/privops_listener.cpp +++ b/daemon/privops_listener.cpp @@ -178,7 +178,9 @@ void acceptLoop(Runtime *rt) { #ifdef __FreeBSD__ int openListener(const std::string &path, const std::string &group, unsigned mode) { - int fd = ::socket(AF_UNIX, SOCK_STREAM, 0); + // 1.1.26: SOCK_CLOEXEC so forked children never inherit the privops + // listener (see control_socket.cpp bindSocketOrThrow). + int fd = ::socket(AF_UNIX, SOCK_STREAM | SOCK_CLOEXEC, 0); if (fd < 0) { throw std::runtime_error(std::string("socket: ") + std::strerror(errno)); } diff --git a/daemon/routes.cpp b/daemon/routes.cpp index 5b3dd35..bb4afe7 100644 --- a/daemon/routes.cpp +++ b/daemon/routes.cpp @@ -83,6 +83,36 @@ static std::string getClientId(const httplib::Request &req) { return req.remote_addr.empty() ? "tcp" : req.remote_addr; } +// 1.1.26: start a container by spawning `crate run` as its OWN process +// (fork + setsid + exec) — exactly what the control-socket start path +// already does. The previous code called runCrate() IN-PROCESS on the +// httplib worker thread, which: +// (a) blocked that worker for the jail's whole lifetime — a +// service-only container never returns, and after ~8 starts the +// entire HTTP API, /healthz included, stopped responding; +// (b) installed runCrate's SIGINT/SIGTERM handlers process-wide, so +// `service crated stop` hung until SIGKILL; +// (c) keyed jailXname / FwSlots / FwUsers on getpid(), so every +// daemon-started jail shared ONE key — a later start overwrote the +// first jail's firewall slot and the first teardown removed the +// shared NAT rule while other jails still ran. +// A child process has its own pid, signal handlers and lifetime. +// Returns the child pid, or -1 on fork failure. +static pid_t spawnCrateRun(const std::string &crateFile) { + pid_t pid = ::fork(); + if (pid < 0) return -1; + if (pid == 0) { + ::setsid(); // no controlling tty; survives crated teardown + ::close(0); ::close(1); ::close(2); + ::open("/dev/null", O_RDONLY); + ::open("/dev/null", O_WRONLY); + ::open("/dev/null", O_WRONLY); + ::execl(CRATE_PATH_CRATE, "crate", "run", "-f", crateFile.c_str(), nullptr); + ::_exit(127); + } + return pid; +} + // Cap constants live in daemon/rate_limit.h since 0.7.15. // (Pre-0.8.8 this file had local constexpr aliases; removed in // the routes.cpp rate-limit refactor.) @@ -149,7 +179,10 @@ static void handleHostInfo(const httplib::Response &, httplib::Response &res) { auto hostname = Util::getSysctlString("kern.hostname"); auto machine = Util::getSysctlString("hw.machine"); auto ncpu = Util::getSysctlInt("hw.ncpu"); - auto physmem = Util::getSysctlInt("hw.physmem"); + // 1.1.26: hw.physmem is CTLTYPE_ULONG (8 bytes on amd64/arm64); + // reading it through the 4-byte getSysctlInt made sysctlbyname + // fail with ENOMEM, so this endpoint was a permanent 500 on 64-bit. + auto physmem = Util::getSysctlUInt64("hw.physmem"); std::ostringstream ss; ss << "{\"hostname\":\"" << hostname << "\"" @@ -298,19 +331,16 @@ static void handleContainerStart(const httplib::Request &req, httplib::Response return; } - try { - Args runArgs; - runArgs.cmd = CmdRun; - runArgs.runCrateFile = crateFile; - int returnCode = 0; - if (runCrate(runArgs, 0, nullptr, returnCode)) { - jsonOk(res, "{\"started\":true,\"name\":\"" + name + "\"}"); - } else { - jsonError(res, 500, "failed to start container"); - } - } catch (const std::exception &e) { - jsonError(res, 500, e.what()); + pid_t pid = spawnCrateRun(crateFile); + if (pid < 0) { + jsonError(res, 500, "failed to start container: fork failed"); + return; } + // The child is the jail's foreground supervisor and outlives this + // request: "started" now means "spawned". Poll + // GET /api/v1/containers/:name for the running state. + jsonOk(res, "{\"started\":true,\"async\":true,\"pid\":" + std::to_string(pid) + + ",\"name\":\"" + name + "\"}"); } // --- F2: DELETE /api/v1/containers/:name --- @@ -608,19 +638,13 @@ static void handleContainerRestart(const httplib::Request &req, httplib::Respons jsonError(res, 404, "no saved .crate file for '" + name + "'; container stopped but cannot restart"); return; } - try { - Args runArgs; - runArgs.cmd = CmdRun; - runArgs.runCrateFile = crateFile; - int returnCode = 0; - if (runCrate(runArgs, 0, nullptr, returnCode)) { - jsonOk(res, "{\"restarted\":true,\"name\":\"" + name + "\"}"); - } else { - jsonError(res, 500, "failed to start container after stop"); - } - } catch (const std::exception &e) { - jsonError(res, 500, e.what()); + pid_t pid = spawnCrateRun(crateFile); + if (pid < 0) { + jsonError(res, 500, "failed to start container after stop: fork failed"); + return; } + jsonOk(res, "{\"restarted\":true,\"async\":true,\"pid\":" + std::to_string(pid) + + ",\"name\":\"" + name + "\"}"); } // --- Snapshot helpers --- diff --git a/daemon/server.cpp b/daemon/server.cpp index 339daa0..8f4be37 100644 --- a/daemon/server.cpp +++ b/daemon/server.cpp @@ -24,7 +24,13 @@ namespace Crated { struct Server::Impl { - std::unique_ptr httpSrv; + std::unique_ptr httpSrv; // TCP (optionally TLS) listener + // 1.1.26: the Unix-socket server is a member, not a lambda-local. + // It used to live only inside unixThread's closure, so stop() could + // never call its stop(), listen() never returned, and ~Impl destroyed + // a still-joinable std::thread → std::terminate (SIGABRT) on every + // clean shutdown with the default unixSocket configured. + std::unique_ptr udsSrv; std::thread tcpThread; std::thread unixThread; }; @@ -65,13 +71,14 @@ void Server::start() { // Remove stale socket ::unlink(config_.unixSocket.c_str()); + // This is the Unix-socket listener (local, root-owned socket) — + // isUnixListener = true, so its peers are treated as trusted. + // Built here (not inside the thread) so stop() can reach it. + impl_->udsSrv = std::make_unique(); + registerRoutes(*impl_->udsSrv, config_, /*isUnixListener=*/true); + impl_->udsSrv->set_address_family(AF_UNIX); impl_->unixThread = std::thread([this]() { - httplib::Server udsSrv; - // This is the Unix-socket listener (local, root-owned socket) — - // isUnixListener = true, so its peers are treated as trusted. - registerRoutes(udsSrv, config_, /*isUnixListener=*/true); - udsSrv.set_address_family(AF_UNIX); - udsSrv.listen(config_.unixSocket, 0); + impl_->udsSrv->listen(config_.unixSocket, 0); }); // 0.8.19: enforce post-bind filesystem perms. @@ -157,7 +164,14 @@ void Server::stop() { impl_->httpSrv->stop(); if (impl_->tcpThread.joinable()) impl_->tcpThread.join(); - // Unix socket thread will exit when server stops + // 1.1.26: stop the Unix-socket server too and JOIN its thread. The + // old comment "Unix socket thread will exit when server stops" was + // false — nothing ever stopped that server, and destroying the + // joinable thread aborted the process. + if (impl_->udsSrv) + impl_->udsSrv->stop(); + if (impl_->unixThread.joinable()) + impl_->unixThread.join(); if (!config_.unixSocket.empty()) ::unlink(config_.unixSocket.c_str()); } diff --git a/daemon/ws_console.cpp b/daemon/ws_console.cpp index b79e8d5..94db245 100644 --- a/daemon/ws_console.cpp +++ b/daemon/ws_console.cpp @@ -11,6 +11,7 @@ #include +#include #include #include #include @@ -209,9 +210,25 @@ void runJailSession(int wsFd, int jid) { } } - // Best-effort: terminate the shell, reap the zombie. + // Terminate the shell and reap it HERE, synchronously. 1.1.26: this + // used to rely on a process-wide `signal(SIGCHLD, SIG_IGN)` set in + // WsConsole::start — which on FreeBSD sets PS_NOCLDWAIT and makes + // EVERY waitpid() in the daemon fail with ECHILD, so every + // Util::execCommand* (privops verbs, stats, export, ...) threw + // "waitpid failed" the moment console.port was enabled. Grace period + // on SIGTERM, then SIGKILL — which cannot be ignored — so the final + // blocking wait is guaranteed to return. ::kill(pid, SIGTERM); - ::waitpid(pid, nullptr, WNOHANG); + bool reaped = false; + for (int i = 0; i < 20 && !reaped; i++) { // ~2s grace + pid_t r = ::waitpid(pid, nullptr, WNOHANG); + if (r == pid || (r == -1 && errno == ECHILD)) reaped = true; + else ::usleep(100000); + } + if (!reaped) { + ::kill(pid, SIGKILL); + ::waitpid(pid, nullptr, 0); + } ::close(master); } @@ -316,7 +333,12 @@ int openListenSocket(const std::string &host, unsigned port) { addrinfo *pick = ipv6 ? ipv6 : ipv4; if (!pick) { ::freeaddrinfo(res); return -1; } - int fd = ::socket(pick->ai_family, pick->ai_socktype, pick->ai_protocol); + // 1.1.26: SOCK_CLOEXEC so the listening socket is not inherited by + // the long-lived children crated forks (jexec shells, `crate run`). + // An inherited LISTEN fd made `service crated restart` fail with + // EADDRINUSE while any such child lived, and let a jailed shell hold + // the host daemon's listener. + int fd = ::socket(pick->ai_family, pick->ai_socktype | SOCK_CLOEXEC, pick->ai_protocol); if (fd < 0) { ::freeaddrinfo(res); return -1; } int one = 1; ::setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &one, sizeof(one)); @@ -339,7 +361,13 @@ bool WsConsole::start(const Config &config) { int fd = openListenSocket(config.consoleWsBind, config.consoleWsPort); if (fd < 0) return false; - ::signal(SIGCHLD, SIG_IGN); // auto-reap session children + // 1.1.26: do NOT set SIGCHLD to SIG_IGN here. It is process-wide: on + // FreeBSD it flags PS_NOCLDWAIT, children are auto-reaped by the + // kernel and waitpid() returns -1/ECHILD for ALL of crated's children + // — every Util::execCommand*/execPipeline call (privops verbs, stats, + // export/import, control-socket ops) then threw "waitpid failed" even + // when the child succeeded, and JailExec decoded an uninitialized + // status. Session shells are now reaped explicitly in runSession(). g_listenFd = fd; g_running.store(true); g_thread = std::thread(acceptLoop, config, fd); diff --git a/docs/trust-model.md b/docs/trust-model.md index b9cec9d..8ad93f4 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -4,7 +4,7 @@ operators on one machine) and contributors extending the privileged surface. -**Applies to:** 1.1.25 (rootless model + per-tenant authz series 1.1.12 → +**Applies to:** 1.1.26 (rootless model + per-tenant authz series 1.1.12 → 1.1.17 covering every privops verb that carries an operator-controlled ownership signal). For the ≤ 0.9.x setuid model and the migration, see [`rootless-migration.md`](rootless-migration.md). @@ -62,7 +62,7 @@ surface; 1.0.0 removed the setuid bit (`Makefile`, comment at the operator and delegates privileged operations to crated(8)"*). The single-trust-domain property did **not** disappear — it relocated. -Reasoning about isolation on 1.1.25 means reasoning about who can reach +Reasoning about isolation on 1.1.26 means reasoning about who can reach **privops**, not who can run `crate(1)`. --- diff --git a/docs/trust-model.uk.md b/docs/trust-model.uk.md index f41fa84..3a26664 100644 --- a/docs/trust-model.uk.md +++ b/docs/trust-model.uk.md @@ -4,7 +4,7 @@ (кілька операторів на одній машині), і контрибʼютори, які розширюють привілейовану поверхню. -**Стосується:** 1.1.25 (rootless-модель + серія per-tenant authz 1.1.12 → +**Стосується:** 1.1.26 (rootless-модель + серія per-tenant authz 1.1.12 → 1.1.17 покриває кожен privops-верб з operator-controlled ownership- сигналом). Про ≤ 0.9.x setuid-модель і міграцію див. [`rootless-migration.md`](rootless-migration.md). @@ -62,7 +62,7 @@ privops-сокета, ніколи admin-токен і ніколи Unix-сок privileged operations to crated(8)»*). Властивість «єдиний домен довіри» **не зникла** — вона переїхала. -Міркувати про ізоляцію на 1.1.25 — це міркувати про те, хто має доступ +Міркувати про ізоляцію на 1.1.26 — це міркувати про те, хто має доступ до **privops**, а не хто може запустити `crate(1)`. --- diff --git a/lib/jail_query.cpp b/lib/jail_query.cpp index d50b4ea..59dd488 100644 --- a/lib/jail_query.cpp +++ b/lib/jail_query.cpp @@ -17,6 +17,7 @@ extern "C" { #include #include +#include // 1.1.26: O_CLOEXEC for pipe2() #include #include #include @@ -330,7 +331,10 @@ std::string execInJailGetOutput(int jid, const std::vector &argv, const std::string &user, const std::string &what) { // For output capture, use pipe int pipefd[2]; - if (::pipe(pipefd) == -1) + // 1.1.26: O_CLOEXEC — the write end must not leak into unrelated + // children forked concurrently on other daemon threads, or the read + // loop below waits for an EOF that never comes (see Util::execCommandGetOutput). + if (::pipe2(pipefd, O_CLOEXEC) == -1) ERR("pipe failed: " << strerror(errno)) pid_t pid = ::fork(); diff --git a/lib/run_net.cpp b/lib/run_net.cpp index 2fb0a47..993bccc 100644 --- a/lib/run_net.cpp +++ b/lib/run_net.cpp @@ -219,13 +219,19 @@ GatewayInfo detectGateway() { GatewayInfo gw; // determine host's gateway interface + // 1.1.26: restrict to the inet table (`-f inet`). Without it netstat + // also prints the inet6 table, so any host with an IPv6 default route + // (SLAAC/RA is common) produced TWO `default` lines → 8 tokens → the + // strict `!= 4` check below failed and every NAT-mode `crate run` + // died with "Unable to determine host's gateway". The IPv6 query in + // run.cpp already passes `-f inet6` and tolerates >= 4; mirror that. auto elts = Util::splitString( Util::execPipelineGetOutput( - {{CRATE_PATH_NETSTAT, "-rn"}, {CRATE_PATH_GREP, "^default"}, {CRATE_PATH_SED, "s| *| |"}}, + {{CRATE_PATH_NETSTAT, "-rn", "-f", "inet"}, {CRATE_PATH_GREP, "^default"}, {CRATE_PATH_SED, "s| *| |"}}, "determine host's gateway interface"), " " ); - if (elts.size() != 4) + if (elts.size() < 4) ERR("Unable to determine host's gateway IP and interface"); elts[3] = Util::stripTrailingSpace(elts[3]); gw.iface = elts[3]; diff --git a/lib/stack.cpp b/lib/stack.cpp index c27aabc..dfb96ca 100644 --- a/lib/stack.cpp +++ b/lib/stack.cpp @@ -226,7 +226,13 @@ static std::string generateUnboundConf( conf << "server:\n"; conf << " interface: " << listenIp << "\n"; conf << " port: 53\n"; - conf << " do-daemonize: yes\n"; + // 1.1.26: run in the foreground. With `do-daemonize: yes` the process + // we fork()ed was only the short-lived launcher: it exited within ms, + // so the 200ms liveness check in startStackDns saw "exited + // immediately" on SUCCESS, the returned pid never pointed at unbound, + // and nothing could stop it. Now our child IS unbound (setsid'd in + // startStackDns); `down` stops it via the pidfile below. + conf << " do-daemonize: no\n"; // Use per-stack pidfile to avoid clashes when multiple stacks run DNS if (!networkName.empty()) conf << " pidfile: \"" << dnsBaseDir() << "/dns-" << networkName << "/unbound.pid\"\n"; @@ -281,6 +287,10 @@ static pid_t startStackDns( pid_t pid = ::fork(); if (pid == 0) { + // 1.1.26: own session so unbound (now foreground, see the conf) + // survives `crate stack up` exiting / the terminal hanging up. It + // logs to syslog and never uses stdio, so stdio is left as-is. + ::setsid(); ::execl(CRATE_PATH_UNBOUND, "unbound", "-c", confPath.c_str(), nullptr); ::_exit(127); } @@ -303,13 +313,29 @@ static pid_t startStackDns( // Stop the per-stack unbound DNS service static void stopStackDns(const std::string &networkName, pid_t unboundPid) { - if (unboundPid > 0) { - ::kill(unboundPid, SIGTERM); + auto confDir = STR(dnsBaseDir() << "/dns-" << networkName); + // 1.1.26: `stack down` runs in a DIFFERENT process from `stack up`, so + // the pid `up` got from fork() is unavailable here (the caller passes + // -1) — and under the old `do-daemonize: yes` that pid was the + // short-lived launcher anyway, never unbound itself. Net effect: DNS + // was NEVER stopped; an orphan unbound stayed bound to :53 + // and the next `up` failed to bind ("exited immediately"). Read the + // pidfile unbound writes into confDir (set in generateUnboundConf) + // and signal that; fall back to the passed pid. + pid_t target = -1; + { + std::ifstream pf(confDir + "/unbound.pid"); + long v = 0; + if ((pf >> v) && v > 1) target = static_cast(v); + } + if (target <= 1) target = unboundPid; + if (target > 1) { + ::kill(target, SIGTERM); + // Not our child when called from `down` — reap only if it is. int status; - ::waitpid(unboundPid, &status, 0); + ::waitpid(target, &status, WNOHANG); } // Clean up config directory - auto confDir = STR(dnsBaseDir() << "/dns-" << networkName); std::filesystem::remove_all(confDir); } diff --git a/lib/util.cpp b/lib/util.cpp index 08c5af8..81d1c7a 100644 --- a/lib/util.cpp +++ b/lib/util.cpp @@ -176,7 +176,14 @@ std::string execCommandGetOutput(const std::vector &argv, const std if (argv.empty()) ERR2("exec command", "empty argv for: " << what) int pipefd[2]; - if (::pipe(pipefd) == -1) + // 1.1.26: O_CLOEXEC. Without it the pipe's WRITE end leaked into every + // other child crated forked concurrently on another thread (the + // control-socket `crate run` supervisor, ws-console shells — both + // long-lived), so the read loop below waited for an EOF that only + // arrived when THAT unrelated child exited: request threads hung for + // the lifetime of a jail. dup2() below clears CLOEXEC on the target + // fd, so the intended child still gets its stdout. + if (::pipe2(pipefd, O_CLOEXEC) == -1) ERR2("exec command", "pipe failed for '" << what << "': " << strerror(errno)) UniqueFd pipeRead(pipefd[0]), pipeWrite(pipefd[1]); auto cargv = toExecArgv(argv); @@ -222,7 +229,7 @@ static std::string execPipelineImpl(const std::vector> // Create n-1 pipes std::vector pipefds(2 * (n - 1)); for (int i = 0; i < n - 1; i++) { - if (::pipe(&pipefds[2*i]) == -1) { + if (::pipe2(&pipefds[2*i], O_CLOEXEC) == -1) { // 1.1.26: CLOEXEC, see execCommandGetOutput // Close already-created pipes on failure for (int j = 0; j < 2*i; j++) ::close(pipefds[j]); ERR2("exec pipeline", "pipe() failed for '" << what << "': " << strerror(errno)) @@ -232,7 +239,7 @@ static std::string execPipelineImpl(const std::vector> // Capture pipe for last process stdout (when capture=true) int capturePipe[2] = {-1, -1}; if (capture) { - if (::pipe(capturePipe) == -1) { + if (::pipe2(capturePipe, O_CLOEXEC) == -1) { // 1.1.26: CLOEXEC for (auto fd : pipefds) ::close(fd); ERR2("exec pipeline", "pipe() failed for capture: " << strerror(errno)) } @@ -368,6 +375,17 @@ int getSysctlInt(const char *name) { return value; } +unsigned long long getSysctlUInt64(const char *name) { + // Zero-initialised so a 4-byte node (e.g. hw.physmem on i386) fills + // only the low bytes and the high bytes stay 0 (little-endian). + unsigned long long value = 0; + size_t size = sizeof(value); + + SYSCALL(::sysctlbyname(name, &value, &size, nullptr, 0), "sysctlbyname (get u64)", name); + + return value; +} + void setSysctlInt(const char *name, int value) { SYSCALL(::sysctlbyname(name, nullptr, nullptr, &value, sizeof(value)), "sysctlbyname (set int)", name); } diff --git a/lib/util.h b/lib/util.h index bd20627..e23c2aa 100644 --- a/lib/util.h +++ b/lib/util.h @@ -125,6 +125,10 @@ std::string tmSecMs(); std::string filePathToBareName(const std::string &path); std::string filePathToFileName(const std::string &path); int getSysctlInt(const char *name); +// 1.1.26: for CTLTYPE_ULONG/U64 nodes (e.g. hw.physmem, 8 bytes on +// 64-bit). getSysctlInt's 4-byte buffer makes sysctlbyname fail with +// ENOMEM on those, which turned GET /api/v1/host into a permanent 500. +unsigned long long getSysctlUInt64(const char *name); void setSysctlInt(const char *name, int value); std::string getSysctlString(const char *name); void ensureKernelModuleIsLoaded(const char *name);