Command (macOS): returns output after daemonized child exits

CHANGELOG.md

@@ -1,3 +1,8 @@
+# Unreleased
+
+Bugfixes:
+* Fixed the Command module timing out when a command exits after spawning a background process that keeps its output pipe open. (Command, macOS)
+
 # 2.70.0
 
 Changes:

CMakeLists.txt

@@ -2263,6 +2263,15 @@ endif()
 ###################
 
 if (BUILD_TESTS)
+    if (NOT WIN32)
+        add_executable(fastfetch-test-processing
+            tests/processing.c
+        )
+        target_link_libraries(fastfetch-test-processing
+            PRIVATE libfastfetch Threads::Threads
+        )
+    endif()
+
     add_executable(fastfetch-test-cache
         tests/cache.c
     )
@@ -2390,6 +2399,10 @@ if (BUILD_TESTS)
     )
 
     enable_testing()
+    if (NOT WIN32)
+        add_test(NAME test-processing COMMAND fastfetch-test-processing)
+        set_tests_properties(test-processing PROPERTIES TIMEOUT 5)
+    endif()
     add_test(NAME test-cache COMMAND fastfetch-test-cache)
     add_test(NAME test-logo COMMAND fastfetch-test-logo)
     add_test(NAME test-strbuf COMMAND fastfetch-test-strbuf)

src/common/impl/processing_linux.c

@@ -4,6 +4,7 @@
 #include "common/io.h"
 #include "common/strutil.h"
 #include "common/mallocHelper.h"
+#include "common/time.h"
 
 #include <stdlib.h>
 #include <unistd.h>
@@ -167,6 +168,18 @@ const char* ffProcessSpawn(char* const argv[], bool useStdErr, FFNativeFD stdinF
     return nullptr;
 }
 
+static const char* ffProcessCheckExitStatus(int stat_loc) {
+    if (!WIFEXITED(stat_loc)) {
+        return "child process exited abnormally";
+    }
+    if (WEXITSTATUS(stat_loc) == 127) {
+        FF_DEBUG("command not found");
+        return "command not found";
+    }
+    // We only handle 127 as an error. See `getTerminalVersionUrxvt` in `terminalshell.c`
+    return nullptr;
+}
+
 const char* ffProcessReadOutput(FFProcessHandle* handle, FFstrbuf* buffer) {
     assert(handle->pipeRead != -1);
     assert(handle->pid != -1);
@@ -177,49 +190,89 @@ const char* ffProcessReadOutput(FFProcessHandle* handle, FFstrbuf* buffer) {
     handle->pipeRead = -1;
     handle->pid = -1;
     char str[FF_PIPE_BUFSIZ];
+    double lastOutputTick = ffTimeGetTick();
+    bool childExited = false;
+    int stat_loc = 0;
+    enum { FF_PROCESS_POLL_INTERVAL = 50 };
 
     for (;;) {
-        if (timeout >= 0) {
-            struct pollfd pollfd = { childPipeFd, POLLIN, 0 };
-            int pollret = poll(&pollfd, 1, timeout);
-            if (pollret == 0) {
-                FF_DEBUG("poll(&pollfd, 1, timeout) timeout (try increasing --processing-timeout)");
-                kill(childPid, SIGTERM);
-                waitpid(childPid, nullptr, 0);
-                return "poll(&pollfd, 1, timeout) timeout (try increasing --processing-timeout)";
-            } else if (pollret < 0 || (pollfd.revents & POLLERR)) {
-                kill(childPid, SIGTERM);
-                waitpid(childPid, nullptr, 0);
-                return pollret < 0
-                    ? "poll(&pollfd, 1, timeout) error: pollret < 0"
-                    : "poll(&pollfd, 1, timeout) error: pollfd.revents & POLLERR";
+        // poll() has no portable way to watch a child process on every supported POSIX system.
+        // Use short slices so a child that exits while a daemonized descendant still holds the
+        // output pipe open can be observed without process-global signal handling.
+        int pollTimeout = FF_PROCESS_POLL_INTERVAL;
+        if (childExited) {
+            pollTimeout = 0;
+        } else if (timeout >= 0) {
+            double remaining = (double) timeout - (ffTimeGetTick() - lastOutputTick);
+            if (remaining <= 0) {
+                pollTimeout = 0;
+            } else if (remaining < FF_PROCESS_POLL_INTERVAL) {
+                pollTimeout = (int) remaining;
             }
         }
 
-        ssize_t nRead = read(childPipeFd, str, FF_PIPE_BUFSIZ);
-        if (nRead > 0) {
-            ffStrbufAppendNS(buffer, (uint32_t) nRead, str);
-        } else if (nRead == 0) {
-            int stat_loc = 0;
-            if (childPid > 0 && waitpid(childPid, &stat_loc, 0) == childPid) {
-                if (!WIFEXITED(stat_loc)) {
-                    return "child process exited abnormally";
-                }
-                if (WEXITSTATUS(stat_loc) == 127) {
-                    FF_DEBUG("command not found");
-                    return "command not found";
+        struct pollfd pollfd = { childPipeFd, POLLIN, 0 };
+        int pollret = poll(&pollfd, 1, pollTimeout);
+        bool pollInterrupted = pollret < 0 && errno == EINTR;
+        if (pollret < 0 && !pollInterrupted) {
+            kill(childPid, SIGTERM);
+            while (waitpid(childPid, nullptr, 0) < 0 && errno == EINTR) {}
+            return "poll(&pollfd, 1, timeout) error: pollret < 0";
+        }
+        if (pollret > 0 && (pollfd.revents & (POLLERR | POLLNVAL))) {
+            kill(childPid, SIGTERM);
+            while (waitpid(childPid, nullptr, 0) < 0 && errno == EINTR) {}
+            return "poll(&pollfd, 1, timeout) error: pollfd.revents & POLLERR";
+        }
+
+        if (!childExited) {
+            pid_t waitResult;
+            do {
+                waitResult = waitpid(childPid, &stat_loc, WNOHANG);
+            } while (waitResult < 0 && errno == EINTR);
+            if (waitResult == childPid || (waitResult < 0 && errno == ECHILD)) {
+                childExited = true;
+            } else if (waitResult < 0) {
+                return "waitpid(childPid, &stat_loc, WNOHANG) failed";
+            }
+        }
+
+        if (pollInterrupted) {
+            continue;
+        }
+
+        if (pollret > 0 && (pollfd.revents & (POLLIN | POLLHUP))) {
+            ssize_t nRead = read(childPipeFd, str, FF_PIPE_BUFSIZ);
+            if (nRead > 0) {
+                ffStrbufAppendNS(buffer, (uint32_t) nRead, str);
+                lastOutputTick = ffTimeGetTick();
+                continue;
+            } else if (nRead == 0) {
+                if (!childExited && childPid > 0) {
+                    pid_t waitResult;
+                    do {
+                        waitResult = waitpid(childPid, &stat_loc, 0);
+                    } while (waitResult < 0 && errno == EINTR);
+                    childExited = waitResult == childPid;
                 }
-                // We only handle 127 as an error. See `getTerminalVersionUrxvt` in `terminalshell.c`
-                return nullptr;
+                return childExited ? ffProcessCheckExitStatus(stat_loc) : nullptr;
+            } else if (errno != EINTR && errno != EAGAIN && errno != EWOULDBLOCK) {
+                FF_DEBUG("read(childPipeFd, str, FF_PIPE_BUFSIZ) failed: %s", strerror(errno));
+                return "read(childPipeFd, str, FF_PIPE_BUFSIZ) failed";
             }
-            return nullptr;
-        } else if (nRead < 0) {
-            break;
         }
-    }
 
-    FF_DEBUG("read(childPipeFd, str, FF_PIPE_BUFSIZ) failed: %s", strerror(errno));
-    return "read(childPipeFd, str, FF_PIPE_BUFSIZ) failed";
+        if (childExited) {
+            return ffProcessCheckExitStatus(stat_loc);
+        }
+
+        if (timeout >= 0 && ffTimeGetTick() - lastOutputTick >= (double) timeout) {
+            FF_DEBUG("poll(&pollfd, 1, timeout) timeout (try increasing --processing-timeout)");
+            kill(childPid, SIGTERM);
+            while (waitpid(childPid, nullptr, 0) < 0 && errno == EINTR) {}
+            return "poll(&pollfd, 1, timeout) timeout (try increasing --processing-timeout)";
+        }
+    }
 }
 
 void ffProcessGetInfoLinux(pid_t pid, FFstrbuf* processName, FFstrbuf* exe, const char** exeName, FFstrbuf* exePath) {

tests/processing.c

@@ -0,0 +1,137 @@
+#include "common/processing.h"
+#include "fastfetch.h"
+
+#include <pthread.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+typedef struct ProcessReadContext {
+    FFProcessHandle handle;
+    FFstrbuf output;
+    const char* error;
+} ProcessReadContext;
+
+static void* readOutput(void* data) {
+    ProcessReadContext* context = data;
+    context->error = ffProcessReadOutput(&context->handle, &context->output);
+    return nullptr;
+}
+
+static pid_t parseDescendantPid(const FFstrbuf* output) {
+    if (!output->chars || output->length == 0) {
+        return -1;
+    }
+
+    char* end = nullptr;
+    long pid = strtol(output->chars, &end, 10);
+    if (end == output->chars || pid <= 0) {
+        return -1;
+    }
+
+    return (pid_t) pid;
+}
+
+static void stopDescendant(const FFstrbuf* output) {
+    pid_t pid = parseDescendantPid(output);
+    if (pid > 0) {
+        kill(pid, SIGKILL);
+    }
+}
+
+int main(void) {
+    // The direct shell exits immediately, while its background sleep keeps the output pipe open.
+    char* const successArgv[] = {
+        "/bin/sh",
+        "-c",
+        "sleep 30 & printf '%s\\n' \"$!\"",
+        nullptr,
+    };
+    char* const failureArgv[] = {
+        "/bin/sh",
+        "-c",
+        "sleep 30 & printf '%s\\n' \"$!\"; exit 127",
+        nullptr,
+    };
+    const char* const expectedErrors[] = { nullptr, "command not found" };
+
+    instance.config.general.processingTimeout = 1000;
+
+    ProcessReadContext contexts[2] = {};
+    size_t spawned = 0;
+    bool spawnFailed = false;
+    for (; spawned < 2;) {
+        contexts[spawned].output = ffStrbufCreate();
+        char* const* argv = spawned == 0 ? successArgv : failureArgv;
+        const char* error = ffProcessSpawn(argv, false, ffGetNullFD(), &contexts[spawned].handle);
+        if (error) {
+            fprintf(stderr, "ffProcessSpawn failed: %s\n", error);
+            ffStrbufDestroy(&contexts[spawned].output);
+            spawnFailed = true;
+            break;
+        }
+        spawned++;
+    }
+
+    pthread_t threads[2];
+    size_t started = 0;
+    if (!spawnFailed) {
+        for (; started < 2; started++) {
+            if (pthread_create(&threads[started], nullptr, readOutput, &contexts[started]) != 0) {
+                fprintf(stderr, "pthread_create failed\n");
+                break;
+            }
+        }
+    }
+
+    for (size_t i = started; i < spawned; i++) {
+        contexts[i].error = ffProcessReadOutput(&contexts[i].handle, &contexts[i].output);
+    }
+    for (size_t i = 0; i < started; i++) {
+        pthread_join(threads[i], nullptr);
+    }
+
+    bool passed = !spawnFailed && started == 2;
+    for (size_t i = 0; i < spawned; i++) {
+        stopDescendant(&contexts[i].output);
+        bool errorMatches = expectedErrors[i] == nullptr
+            ? contexts[i].error == nullptr
+            : contexts[i].error && strcmp(contexts[i].error, expectedErrors[i]) == 0;
+        if (!errorMatches) {
+            if (contexts[i].error) {
+                fprintf(stderr, "ffProcessReadOutput returned an unexpected error: %s\n", contexts[i].error);
+            } else {
+                fprintf(stderr, "ffProcessReadOutput returned success unexpectedly\n");
+            }
+            passed = false;
+        }
+        if (parseDescendantPid(&contexts[i].output) <= 0) {
+            fprintf(stderr, "child PID was not captured from command output\n");
+            passed = false;
+        }
+        ffStrbufDestroy(&contexts[i].output);
+    }
+
+    instance.config.general.processingTimeout = 100;
+    FFstrbuf timeoutOutput = ffStrbufCreate();
+    FFProcessHandle timeoutHandle;
+    char* const timeoutArgv[] = { "/bin/sleep", "30", nullptr };
+    const char* timeoutError = ffProcessSpawn(timeoutArgv, false, ffGetNullFD(), &timeoutHandle);
+    if (timeoutError == nullptr) {
+        timeoutError = ffProcessReadOutput(&timeoutHandle, &timeoutOutput);
+    }
+    if (!timeoutError
+        || strcmp(timeoutError, "poll(&pollfd, 1, timeout) timeout (try increasing --processing-timeout)") != 0) {
+        fprintf(stderr, "processing timeout was not reported\n");
+        passed = false;
+    }
+    ffStrbufDestroy(&timeoutOutput);
+
+    if (!passed) {
+        return 1;
+    }
+
+    puts("All processing tests passed!");
+    return 0;
+}