From c20b19eb868bd528a95c73d1807fd747f5271c9c Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 19 Aug 2026 17:38:50 +0300 Subject: [PATCH 1/2] fix(util/command): stop read() spinning forever on a failed read fgets() returns nullptr on both a read error and at end-of-file and an error sets ferror(), never feof(). The first failed read therefore spins the thread at 100% CPU for good, because read() looped only on feof() and never returns. (command::close() also never never reaps the child, nothing else does, as the SIGCHLD handler only drains the list forkExec() fills). Loop on fgets() which stops on both end-of-file and error, then check ferror() so a failed read is not silently a truncated module output. Reproduced by calling read() under a watchdog on a directory fd, whose reads fail with EISDIR: before: WATCHDOG: read() did not return after 3s -- SPINNING after: [error] Error reading command output: Is a directory read() RETURNED, 0 bytes Co-Authored-By: Claude Opus 5 Signed-off-by: Adrian Ratiu --- include/util/command.hpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/util/command.hpp b/include/util/command.hpp index f6d2cabf85..0d6818bb13 100644 --- a/include/util/command.hpp +++ b/include/util/command.hpp @@ -30,10 +30,11 @@ struct res { inline std::string read(FILE* fp) { std::array buffer = {0}; std::string output; - while (feof(fp) == 0) { - if (fgets(buffer.data(), 128, fp) != nullptr) { - output += buffer.data(); - } + while (fgets(buffer.data(), buffer.size(), fp) != nullptr) { + output += buffer.data(); + } + if (ferror(fp) != 0) { + spdlog::error("Error reading command output: {}", strerror(errno)); } // Remove last newline From b4fcab55f75ffce41efd2f3ea78814e44c094bdd Mon Sep 17 00:00:00 2001 From: Adrian Ratiu Date: Wed, 19 Aug 2026 18:21:51 +0300 Subject: [PATCH 2/2] test(util/command): cover read() on a stream it cannot read The regression this guards against does not fail a run, it wedges one, so the call goes in a child with a 5s watchdog and a return to the old behaviour is reported as a failure instead of a hang. Passes on the current code; with read() put back the way it was it fails with "command::read did not return within 5s". Co-Authored-By: Claude Opus 5 Signed-off-by: Adrian Ratiu --- test/utils/command.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/utils/command.cpp b/test/utils/command.cpp index 053a2b77a5..87e888c078 100644 --- a/test/utils/command.cpp +++ b/test/utils/command.cpp @@ -4,10 +4,12 @@ #include #endif +#include #include #include #include +#include #include #include @@ -55,3 +57,42 @@ TEST_CASE("command::forkExec child exits 127 when shell exec fails", "[util][com std::scoped_lock lock(reap_mtx); reap.remove(pid); } + +TEST_CASE("command::read returns on a stream whose reads fail", "[util][command]") { + // A regression here does not fail, it hangs: read() looping on feof() alone + // never notices that fgets() returned nullptr for an error rather than for + // end of file. Run it in a child so the watchdog reports a failure instead + // of wedging the test run. + const auto pid = fork(); + REQUIRE(pid >= 0); + + if (pid == 0) { + // read(2) on a directory fd fails with EISDIR. + auto* fp = fdopen(::open("/", O_RDONLY), "r"); + if (fp == nullptr) { + _exit(2); + } + waybar::util::command::read(fp); + _exit(0); + } + + int status = -1; + pid_t waited = 0; + for (int i = 0; i < 50; ++i) { + waited = waitpid(pid, &status, WNOHANG); + if (waited != 0) { + break; + } + usleep(100000); + } + + if (waited == 0) { + kill(pid, SIGKILL); + waitpid(pid, nullptr, 0); + FAIL("command::read did not return within 5s"); + } + + REQUIRE(waited == pid); + REQUIRE(WIFEXITED(status)); + REQUIRE(WEXITSTATUS(status) == 0); +}