Fix subprocess registry ownership during close - #614
Open
somethingwithproof wants to merge 1 commit into
Open
Conversation
Detach the subprocess entry under ListMutex before exposing it to close, wait, or cancellation cleanup. A second closer now receives EBADF instead of sharing a pointer that can be freed concurrently. Keep cancellation disabled through descriptor close so cleanup cannot close a reused descriptor number. Cover concurrent closers and cancellation against the shipped nft_popen object. Closes Cacti#610 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
xmacan
approved these changes
Sep 4, 2026
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes correctly enforce exclusive registry ownership under ListMutex, add targeted tests for the new invariants (including cancellation), and include a safety guard against dangerous kill() PID values.
Pull request overview
This PR hardens nft_pclose()’s subprocess registry handling so that closing a nft_popen() descriptor transfers exclusive ownership to exactly one closer, preventing duplicate close/wait/free races and aligning behavior with issue #610’s acceptance criteria.
Changes:
- Refactors
nft_pclose()to atomically detach+close the registry entry underListMutexand free only the detached node via the cleanup handler. - Adds unit tests that exercise concurrent close ownership, cancellation cleanup behavior, and preservation of pthread cancellation state on early
EBADF. - Guards the timeout kill path in
exec_poll()against invalid/unsafe PIDs and records the fix inCHANGELOG.
File summaries
| File | Description |
|---|---|
| tests/unit/test_linked.c | Adds concurrency/cancellation-focused unit tests for nft_pclose()/registry ownership semantics. |
| poller.c | Prevents kill() from being called with invalid PIDs on NIFTY POPEN timeout. |
| nft_popen.c | Introduces atomic detach-and-take helper and updates nft_pclose() cleanup ownership model. |
| CHANGELOG | Documents issue #610 fix. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bmfmancini
approved these changes
Sep 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ListMutexEBADFbefore descriptor reuseSIGKILLfor an invalid child PIDphp.cunchangedValidation
cppcheckonnft_popen.candpoller.cgit diff --checkThe cancellation test uses child readiness and the registry transition rather than a fixed sleep. It deliberately cleans up the pre-existing cancelled-child lifecycle; bounded child shutdown remains in PR #597 and is not duplicated here. PR #597 should rebase after this invariant lands.
Closes #610