fix: cap Solaris reap budget, bound EINTR retries, kill before parking on waitpid error (#635)

## What
Follow-up to #633 (already merged), from the code review findings on the
equivalent 1.2.x change (#634):

- **SOLAR_THREAD sleep budget**: `spine_reap_child_bounded()` has no
`usleep()`-paced spin phase on `SOLAR_THREAD` builds - it sleeps a flat
1s per attempt. #633 raised the shared attempt budget
(`NFT_PCLOSE_SPIN_ATTEMPTS`) to 100 for `nft_pclose()`'s brief
allowance, which made it sleep up to **~100 seconds** on that platform
instead of the intended ~20ms, reintroducing the exact poller stall #633
was fixing. `NFT_PCLOSE_SPIN_ATTEMPTS` is now platform-specific (100
elsewhere, 2 under `SOLAR_THREAD`).
- **Unbounded EINTR retries**: bounded the `EINTR` retry loop in
`spine_reap_child_bounded()` and in `nft_sweep_abandoned()` (which runs
with `ListMutex` held), so a stream of caught signals can no longer spin
either indefinitely.
- **Kill before park on waitpid() error**: `nft_pclose()` now SIGKILLs
the child before parking it when `waitpid()` itself errors (previously
only done when the child was confirmed still running), so a
still-running child isn't left outside the abandoned-pid sweep's reach.
Preserves `waitpid()`'s errno across the call.
- **Docs**: `nft_popen.h`'s `nft_pclose()` comment still described the
old blocking-wait behavior and didn't mention `ETIMEDOUT` - #633 updated
`nft_popen.c`'s comment but missed the header. Both now document the
bounded-check/parked-child contract.

## Why
These were flagged by review of #634 (the same fix ported to 1.2.x,
where the affected code showed up as new rather than a diff against
pre-existing lines). Fixing here too since develop has the identical
code.

## What was tested
- `./bootstrap && ./configure --enable-warnings && make` - clean build,
no new warnings.
- `make check` - all 11 suites pass.
- `cppcheck --enable=all --std=c11` on `nft_popen.c` - no new errors.

Closes #632.

---------

Signed-off-by: TheWitness <thewitness@cacti.net>
This commit is contained in:
TheWitness authored and GitHub committed 2026-09-28 15:21:22 -04:00
1 parent fcfd944a4a
commit ca662c1be7
5 files changed
+134 -15

No files matched your search

+69
View File
@@ -38,6 +38,23 @@
/* provided by tests/fuzz/stubs.c, as spine.c would */
extern int *debug_devices;
#ifdef SPINE_TEST_WRAP_WAITPID
/* Arms __wrap_waitpid() to fail every call for one pid; -1/unset disables it
* so every other test in this binary keeps hitting the real waitpid(). */
static pid_t forced_waitpid_error_pid = -1;
static int forced_waitpid_error_errno = 0;
extern pid_t __real_waitpid(pid_t pid, int *status, int options);
pid_t __wrap_waitpid(pid_t pid, int *status, int options) {
if (pid == forced_waitpid_error_pid) {
errno = forced_waitpid_error_errno;
return -1;
}
return __real_waitpid(pid, status, options);
}
#endif
/* --- strncopy(): issue#447, the off-by-one when src fills the destination -- */
static void test_strncopy_truncates_within_the_buffer(void **state) {
@@ -950,6 +967,55 @@ static void test_abandoned_children_are_swept_and_capacity_is_bounded(void **sta
assert_int_equal(nft_abandoned_pending(), 0);
}
#ifdef SPINE_TEST_WRAP_WAITPID
/* waitpid() failing with anything other than ECHILD/EINTR (never true for a
* real call with WNOHANG) must still kill the child before parking it,
* rather than leaving a still-running process outside the sweep's reach. */
static void test_nft_pclose_kills_child_when_waitpid_errors(void **state) {
int fd;
int status;
pid_t child;
pid_t reaped;
int real_status;
int attempts;
(void) state;
fd = nft_popen("printf x; while :; do sleep 1; done", "r");
assert_true(fd >= 0);
child = nft_pchild(fd);
assert_true(child > 0);
forced_waitpid_error_pid = child;
forced_waitpid_error_errno = EINVAL;
errno = 0;
status = nft_pclose(fd);
forced_waitpid_error_pid = -1;
assert_int_equal(status, -1);
assert_int_equal(errno, EINVAL);
/* Reap for real (bypassing the wrap) to prove the child was killed
* rather than merely parked. */
reaped = -1;
for (attempts = 0; attempts < 100 && reaped != child; attempts++) {
reaped = __real_waitpid(child, &real_status, WNOHANG);
if (reaped == 0) {
usleep(10000);
reaped = -1;
}
}
assert_int_equal(reaped, child);
assert_true(WIFSIGNALED(real_status));
assert_int_equal(WTERMSIG(real_status), SIGKILL);
/* nft_abandon_child() still parked it; sweep it back out so it does not
* leak into a later test's nft_abandoned_pending() count. */
assert_int_equal(nft_abandoned_pending(), 0);
}
#endif
/* The descriptor must not survive an exec. A child that inherits the write end
keeps the pipe open, so the polling thread never sees EOF and blocks to
script_timeout for a data source that already answered. */
@@ -1129,6 +1195,9 @@ int main(void) {
cmocka_unit_test(test_reap_reports_an_already_reaped_child),
cmocka_unit_test(test_nft_pclose_does_not_block_on_a_lingering_child),
cmocka_unit_test(test_abandoned_children_are_swept_and_capacity_is_bounded),
#ifdef SPINE_TEST_WRAP_WAITPID
cmocka_unit_test(test_nft_pclose_kills_child_when_waitpid_errors),
#endif
};
return cmocka_run_group_tests(tests, NULL, NULL);