Skip to content

Restore the close-on-exec and bounded reap removed by #542 - #591

Closed
somethingwithproof wants to merge 1 commit into
Cacti:developfrom
somethingwithproof:fix/restore-child-process-hardening
Closed

Restore the close-on-exec and bounded reap removed by #542#591
somethingwithproof wants to merge 1 commit into
Cacti:developfrom
somethingwithproof:fix/restore-child-process-hardening

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Closes #581.

PR #542 landed three commits after #557 and removed the process hardening #557 had just added. #542 has the lower number, so its branch predates #557 and the base appears to have been folded in rather than rebased. git grep CLOEXEC over develop currently returns nothing.

What was lost

Pipes are inherited across exec. nft_popen() creates its pipe before taking ListMutex, and the entry joins PidList only after posix_spawn returns. A thread spawning inside that window produces a child holding another thread's pipe write end. The first thread never sees EOF, so it blocks to script_timeout and records U for a data source that answered. The script-server pipes are worse: they are never in PidList at all, so every script child inherits cacti2php_pdes[1], and since php_close() signals the server by closing that descriptor, a child holding a duplicate keeps the server alive past shutdown.

The bounded reap became an unbounded block. exec_poll() only sends SIGKILL on the select() timeout branch; the normal branch reads one line and calls nft_pclose() directly. A script that prints its value and then exits slowly pins the poller thread with no timeout, across cycles, holding its available_scripts token throughout.

What this restores, and what it does not

php.c kept its bounded reap through #542 as php_terminate_and_reap(), and that version is better than the one that was removed, so it is untouched. Only its pipes needed the flag. nft_popen.c needed both halves.

The two files had separate copies of the same close-on-exec helper. There is now one, spine_open_pipe_cloexec(), used by both.

Also restored: the close(cacti2php_pdes[...]) pair on the second pipe's failure path, dropped by the same commit, which leaked two descriptors.

Kept from #542: the free(cur) additions on two nft_popen() error paths, and the RESULTS_BUFFER bounds work in php.c. This reverts nothing that commit got right.

pipe2(pdes, O_CLOEXEC) would close the window between pipe() and fcntl() atomically, but it needs _GNU_SOURCE on glibc and spine defines no feature macro. Adding one is a portability decision that does not belong in a restore, so the fcntl() pair stays and the comment says why.

Tests

The previous guard was tests/regression/test_child_process_safety.sh, which grepped the source for the strings it wanted and was deleted alongside the code it protected. Six cmocka cases replace it, against the shipped nft_popen.o:

  • the flag is set on both ends
  • the pipe still works as a pipe
  • the descriptor does not survive an exec, checked by having a child look for it in /proc/self/fd
  • a live child returns "still running" rather than blocking
  • an exited child is collected with its status
  • a child someone else reaped reports success with no status

Reverting just the fcntl() calls, which is what #542 did, fails two of them:

[  ERROR   ] --- (flags & FD_CLOEXEC) != 0
[  FAILED  ] test_cloexec_is_set_on_both_pipe_ends
[  ERROR   ] --- 0 != 0x1
[  FAILED  ] test_pipe_is_not_inherited_across_exec

The second is the interesting one: 0 is the child reporting it found the descriptor.

43 of 43 pass with the fix, make check is green on all four binaries, and the build carries the same four warnings as develop.

No CHANGELOG entry here; #578 is the changelog PR for this batch.

PR Cacti#542 removed both while adding unrelated poller fixes; its branch predated
Cacti#557 and the base looks folded in rather than rebased. Nothing caught it: the
only guard was a shell script that grepped the source, and it was deleted in
the same commit.

An inherited pipe write end keeps a script's reader from seeing EOF, so the
thread blocks to script_timeout for a device that answered. The unbounded
waitpid has no timeout at all, and the thread holds its available_scripts token
while it waits.

The php.c reap survived Cacti#542 and is better than what was removed, so only its
pipes needed the flag. Both callers now share one helper, covered by six tests
against the shipped object: two of them fail if the close-on-exec goes away
again.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Consolidated into #597, which carries this branch's commits unchanged.

Every pair of these ten conflicted on tests/unit/test_linked.c because each appended to the same registration array, so whichever merged first would have forced a rebase on the other nine. #597 is one review and one approval for the same 20 commits, still one logical change each.

Nothing here is dropped. Reopen this if you would rather review it separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PR #542 reverted the FD_CLOEXEC and bounded-reap hardening added by #557

1 participant