diff --git a/include/util/command.hpp b/include/util/command.hpp index f6d2cabf8..0d6818bb1 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 diff --git a/test/utils/command.cpp b/test/utils/command.cpp index 053a2b77a..87e888c07 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); +}