From 726d78019593d5b5f192d2962f5bc975bfe85785 Mon Sep 17 00:00:00 2001 From: Tavian Barnes Date: Wed, 4 Nov 2020 11:36:47 -0500 Subject: spawn: Fix error pipe write failure handling A short history lesson: this code (from exec.c) used to just be write(...); In 649d85b, an empty if block was added to placate _FORTIFY_SOURCE's warn_unused_result: if (write(...) != sizeof(error)) { // ... } This is fine since the parent doesn't rely on receiving those writes (e.g. the child could die before writing), and the small write to a pipe won't fail anyway. But when bfs_spawn() was created, the implementation changed to this: while (write(...) < sizeof(error)); The while loop was presumably added to mirror musl's posix_spawn() implementation, which handles EINTR. But musl's is actually correct: while (write(...) < 0); whereas mine has a subtle bug: since sizeof(error) is unsigned, the bfs code did an unsigned comparison, meaning -1 from write() would *not* restart the loop. Fix it by comparing with 0 like musl, since short writes are impossible here. Perhaps it's time for -Wsign-compare, despite the other 18 instances being false positives. --- spawn.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/spawn.c b/spawn.c index b356c41..8c360d3 100644 --- a/spawn.c +++ b/spawn.c @@ -190,7 +190,14 @@ static void bfs_spawn_exec(const char *exe, const struct bfs_spawn *ctx, char ** fail: error = errno; - while (write(pipefd[1], &error, sizeof(error)) < sizeof(error)); + + while (write(pipefd[1], &error, sizeof(error)) < 0) { + if (errno != EINTR) { + // Parent will still see that we exited unsuccessfully, but won't know why + break; + } + } + close(pipefd[1]); _Exit(127); } -- cgit v1.2.3