fix(policies): roll back every failed load and detach it from shared inputs by leoparente · Pull Request #803 · netboxlabs/pktvisor

leoparente · 2026-10-28T18:58:21Z

Follows #801.

Summary

Two failure paths in PolicyManager::load that predate #801, plus a test showing that #801 removed a lock-order deadlock.

Every failed load is rolled back. load caught only std::runtime_error. Any other exception, such as the std::invalid_argument PcapPlusPlus threw for an interface name before #802, skipped the rollback: the policy's handlers and inputs stayed registered, and every retry failed with "already exists". Now both tries share one rollback, with these outcomes:

a std::runtime_error is wrapped in policy [x] failed to start: … as before;
anything else is rolled back and rethrown unchanged, so POST /api/v1/policies still answers a std::invalid_argument with 422.

A failed load no longer stays attached to a shared input.

The bug: a load that failed before its policy had started left the policy on every input it had joined. On a reused input, policies_count() stayed one too high, so removing that input's last real policy never tore down the input or its -resources policy.
The fix: the rollback now detaches the policy from every input it joined, whether or not it started. An input the failing iteration reached but never joined is left alone, because its lock is already released and a concurrent removal may have freed it. InputStream::remove_policy is now a no-op for a policy that isn't attached. Without that, a detach after stop() would signal a second RemovePolicy, and the resources handler would count the removal twice, because it counts those signals as deltas.

The rollback is best-effort. Each step runs on its own and logs a failure as a warning. A step that throws therefore neither skips the rest nor replaces the exception that started the rollback, which would turn a 422 into a 500. Resources policies are now stopped before they're unregistered.

The rollback survives a concurrent DELETE. load and remove_policy aren't serialised, so the API can remove things while a load is failing. The rollback keeps no pointer that a deletion could free:

Resources policies: tracked by name, and looked up under the policy map lock. A resources policy is attached to its input before it is registered, and never touched afterwards.
Inputs: each input the policy joined is detached by name, under the policy map lock, as remove_policy does. An input that is already gone is skipped.
Orphaned reused inputs: if a reused input's last other policy was deleted while the failing policy still counted on it, the rollback finishes the teardown that remove_policy skipped. That teardown is now one helper, _remove_input_if_unused, shared by both paths.
Failed registration: a resources policy whose registration fails is stopped at once, which detaches it from its input.

Lock order between load and remove_policy. This branch doesn't change any locking; the fix is in #801. Before #801's last commit, the two took the input and policy maps in opposite orders:

load held the input map (module_get_locked) while create_resources_policy registered the resources policy in the policy map;
remove_policy holds the policy map and then removes the input from the input map.

A POST and a DELETE could therefore deadlock. Since #801's 5f9d5e5, load registers the resources policy only after it has released the input map lock. The stress test below shows the difference.

Tests

Bad Config: a failed load leaves no reference on a shared input: second_view selects two taps. One is the input first_view already runs; the other has a missing pcap_file, so the load fails before second_view starts. Checks:
- the shared input's policies_count() is unchanged;
- second_view's handlers are gone, and only one input is left;
- removing first_view tears that input down.
Before the fix: 3 == 2, and the input stayed.
Bad Config: an exception that is not a runtime_error is rolled back too: the mock input's new except_on_start_logic throws a std::logic_error. Two loads must both throw it, unwrapped, and leave no policy, handler or input behind. Before the fix, the second load failed with module name 'default_view-anycast-default_net' already exists.
Bad Config: a resources policy that cannot be registered is rolled back: a user policy takes the name a load's resources policy needs, so the load fails after it has started. Nothing the load made may remain, and the policy that owns the name must keep running.
Loading and removing policies concurrently does not deadlock:
- Setup: two threads each load and remove up to 5,000 policies, every one on a new input of its own. That exercises both lock orders on every iteration.
- Time box: each thread stops after 15 s on a slow build, for example the Debug coverage job. A 180 s watchdog only fires on a hang: it prints and aborts, since stuck threads can't be joined. The logger is kept at warnings for the test.
- Afterwards: the policy, handler and input maps must be empty.
- On fix(policies): start a policy's handlers before its new inputs #801's first commit (cd73275): it deadlocked every time: 3 runs out of 3 at 5,000 iterations, 2 out of 2 with the final 15 s and 180 s settings, and 3 out of 3 at 20,000 iterations, where it stopped after 193, 1,299 and 1,641.
- On this branch: it passes 3 out of 3, in 1 to 2 s, and 20,000 iterations per thread pass in about 8 s.
- At -O0 with coverage: about 12 s on fast cores, and 15 s on slow cores, where it now stops at the time box. Before the time box, that run took nearly 60 s.
Suite: all 25 ctest targets pass on macOS (Release).

Still open (older than this change)

Undercounted handlers: a multi-tap policy's AddPolicy carries the handler count as it stood for that tap, while RemovePolicy carries the full count. So a shared input's input_resources handler_count is undercounted after a multi-tap policy goes, whether by a normal removal or a failed load.
Tap DELETE deadlock: DELETE /api/v1/taps/{name} takes the tap map, then the policy map, then the tap map again, so two concurrent tap DELETEs can deadlock. It's the only lock cycle the review found, and it doesn't involve load, remove_policy or the readers.
Unserialised load and remove: load and remove_policy aren't serialised against each other; only loads share _load_mutex. That is safe with the lock order above, but worth keeping in mind for future changes to either.

🤖 Generated with Claude Code

…m shared inputs PolicyManager::load rolled back only on a std::runtime_error, so any other exception left the policy's handlers and inputs registered and every retry failed with "already exists". It now rolls back on any exception: a std::runtime_error is wrapped as before, anything else is rethrown as it was, so the API still answers a std::invalid_argument with 422. A load that failed before its policy had started also left the policy on every input it reached. On a reused input the policy count stayed one too high, so removing that input's last real policy never tore it down. The rollback now detaches the policy from every input it reached, and InputStream::remove_policy is a no-op for a policy that is not attached, so a removal is never signalled twice. The mock input gains except_on_start_logic, which throws a std::logic_error, for the test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…ned inputs Each rollback step now runs on its own and logs a failure as a warning, so one step that throws neither skips the rest nor replaces the exception that started the rollback, which would turn a 422 into a 500. The policy is detached only from the inputs it was attached to: an input the failing iteration reached but never joined is no longer touched after its lock is released, where a concurrent removal could already have freed it. Resources policies are stopped before they are removed. The concurrency test stops after 15 s on a slow build, its watchdog waits 180 s so it only fires on a hang, it keeps its log output to warnings, and the tests now check that a failed load leaves no input or handler. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

nbl-ai-review · 2026-10-28T18:58:57Z

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net positive. This head fixes the previously-flagged resources-policy dangling-pointer issue by stopping/detaching the still-owned policy before rethrowing on module_add failure, and the new rollback teardown for reused inputs reuses PolicyManager's existing lock order, so no new use-after-free window is introduced.

Resolved since last review

src/Policies.cpp — resources policy is now stopped (and thus detached from its input) before the module_add failure is rethrown, closing the dangling-pointer window on that path.

Previously raised — still open

None. The remaining prior finding (rollback dereferencing a possibly-freed InputStream*) was declined by the author with a concrete invariant argument and remains waived; this review's security pass found no regression against it.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 12 comment(s) · 1 prior AI review(s) · 3/3 thread(s) resolved · usage: $1.47 · 2332k in (94% cached) · 20k out}

Previous review · 2026-10-28 · 019e442

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net positive: this closes the rollback gaps described in the PR (non-runtime_error exceptions, stale references on shared inputs) and the two UAF windows Codex already flagged are fixed by name-based tracking. The reorder that closes one of those windows opens a new, narrower one on the resources-policy failure path.

Findings

[High] [security] Resources policy is attached to its input before registration succeeds, leaving a dangling pointer if module_add throws — src/Policies.cpp:211 — resources_policy->input_stream().back()->add_policy(resources_policy.get()) (line 210) now runs before module_add(std::move(resources_policy)) (line 211), so the TOCTOU window Codex flagged on the success path is closed. But module_add (src/AbstractManager.h:77) throws ModuleException if a policy named <input_name>-resources already exists — reachable if a user has a policy by that exact name — and module_add takes its argument by && reference, so the throw at line 80 happens before the internal std::move at line 83: the unique_ptr in the resources_policies vector is never actually moved-from. That exception is caught by the outer catch (const std::runtime_error &e) (around line 218), which calls rollback(). rollback() only stops/unregisters names already pushed into added_resources_policies, but this name is pushed only after module_add succeeds (line 212), so it's never in that list — nothing detaches it from the input. When the enclosing scope exits, resources_policies goes out of scope and destroys the still-owned Policy, leaving the InputStream holding a dangling raw pointer in its policy list (used, e.g., the next time that input signals AddPolicy/RemovePolicy or iterates _policies). Suggest calling add_policy() only after module_add() succeeds, or on module_add failure explicitly calling resources_policy->input_stream().back()->remove_policy(resources_policy.get()) before the unique_ptr is destroyed.

Resolved since last review

src/Policies.cpp — resources-policy pointer tracked by name, re-looked-up under the policy map lock before stopping (Codex finding, fixed in 5a1e1a2).
src/Policies.cpp — attached_inputs now tracked by name and re-acquired under the input map lock before detaching, with a no-op guard for an already-removed input (Codex finding, fixed in 019e442).

Previously raised — still open

None. The one prior AI-review finding (rollback dereferencing a possibly-freed InputStream* from attached_inputs) was declined by the author with a concrete invariant argument, and remains superseded by the 019e442 fix above; not re-raised.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 9 comment(s) · 1 prior AI review(s) · 2/2 thread(s) resolved · usage: $1.8 · 2693k in (95% cached) · 34.1k out}

Previous review · 2026-10-28 · 5a1e1a2

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net positive. This head fixes the rollback gaps for non-runtime_error exceptions and for stale shared-input references, and separately fixes the resources-policy use-after-free Codex flagged (now tracked by name and re-acquired under the policy map lock). No new findings from either dimension.

Resolved since last review

src/Policies.cpp — the resources-policy rollback no longer carries a raw pointer across a lock release; it now tracks the policy by name and re-looks it up under the policy map lock before stopping it (fixed in 5a1e1a2).

Previously raised — still open

None. The one remaining prior finding (src/Policies.cpp — rollback dereferencing a possibly-freed InputStream* from attached_inputs) was declined by the author with a concrete invariant argument (inputs in attached_inputs can't reach a zero policies_count() — and so can't be freed by remove_policy — until the rollback itself detaches them, and _load_mutex serializes loads against each other for the duration of the rollback). This review's security pass re-checked that invariant against the current code and found it holds, so the waiver stands.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 6 comment(s) · 1 prior AI review(s) · 1/1 thread(s) resolved · usage: $1.69 · 2734k in (94% cached) · 24.3k out}

Previous review · 2026-10-28 · 72cc961

AI Code Review — risk tier: Full · dimensions run: correctness, security

Advisory only. A human owns the merge decision.

Summary — Net positive: this closes two real rollback gaps (non-runtime_error exceptions skipping cleanup, and failed loads leaving stale references on shared inputs) and is backed by a solid concurrency stress test. One medium-severity memory-safety concern in the new rollback path is worth a look before merge.

Findings

[Medium] [security] Rollback may dereference a freed InputStream* after its protecting lock is released — src/Policies.cpp:103 — attached_inputs stores raw InputStream* pointers obtained under module_get_locked() inside the load loop, but that lock is released at the end of each iteration. InputStream objects are owned as unique_ptr in AbstractManager::_map, so a concurrent module_remove() on the input manager can destroy the object the rollback later touches at src/Policies.cpp:104 (input->remove_policy(policy_ptr)), called from rollback at lines 178/205/209. The PR description itself flags "load and remove_policy aren't serialised against each other" as a known, still-open issue — this diff is the first place that carries a raw pointer across the lock release and calls a method on it afterward, turning that known gap into a plausible use-after-free reachable from the same REST policy-management endpoints (an authenticated client racing a failing POST against a DELETE on a shared tap/input). Suggest re-validating the input is still alive (re-acquire via module_get_locked by name) at the start of each rollback step, or holding the relevant lock for the duration of that step, rather than trusting the pointer captured earlier in the loop.

Pre-existing / out-of-scope

The author's description already tracks two older, unaddressed issues worth keeping on record: undercounted handlers after a multi-tap policy's removal (count carried from AddPolicy time), and a DELETE /api/v1/taps/{name} lock-cycle (tap map → policy map → tap map) that can deadlock two concurrent tap deletions. Neither is introduced by this PR; no action needed here beyond tracking.

_{🤖 AI Code Review · run · prompts: code-review/v1 · models: claude-sonnet-5/medium · context: description · 0 comment(s) · 0 prior AI review(s) · usage: $1.59 · 2053k in (92% cached) · 28.9k out}

github-actions · 2026-10-28T19:11:58Z

LCOV of commit `eb1b6f8` during Debug Builds #258

  lines......: 86.0% (18147 of 21112 lines)
  functions..: 75.7% (1830 of 2418 functions)
  branches...: no data found

Files changed coverage rate: n/a

Full coverage report

leoparente · 2026-10-28T19:35:48Z

@codex review

chatgpt-codex-connector · 2026-10-28T19:36:05Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-28T20:24:26.322829Z	`eb1b6f8`	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 72cc961f2a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Once a resources policy is registered, DELETE /api/v1/policies can remove it while the load is still running, so the rollback must not keep a raw pointer to it. Resources policies are now attached to their input before they are registered and are not touched afterwards; the rollback keeps their names and looks each one up under the policy map lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

leoparente · 2026-10-28T19:44:44Z

Declined. AI review finding "Rollback may dereference a freed InputStream* after its protecting lock is released": an input in attached_inputs can't be freed while the rollback runs.

Only two places remove an input: the rollback itself, which removes only the inputs this load created, by name; and PolicyManager::remove_policy, which removes an input only once its policies_count() is 0. Tap deletion (DELETE /api/v1/taps/{name}) never removes inputs.
attached_inputs holds exactly the inputs this policy was added to. It is filled right after add_policy. Until the rollback detaches it, the failed policy is in each of those inputs' policy list, so their policies_count() can't reach 0 and remove_policy won't free them, whatever policy a concurrent DELETE removes.
No other load can run meanwhile: loads are serialised by _load_mutex, which this load holds through its rollback.
The one input that wasn't protected is never touched: an input the failing iteration reached but never joined. That is why the rollback uses attached_inputs and not policy_ptr->input_stream().

The pointer that could go stale was the resources policies'. Codex flagged it, and 5a1e1a2 tracks them by name.

leoparente · 2026-10-28T19:44:49Z

@codex review

leoparente · 2026-10-28T19:44:51Z

/ai-review

leoparente · 2026-10-28T20:07:02Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5a1e1a28d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Once stop() has detached a started policy, nothing keeps its inputs alive, and a concurrent removal of an input's last policy can free it before the rollback's detach loop reaches it. The rollback now keeps the names of the inputs the policy joined and detaches each under the input map lock, skipping one that is already gone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

leoparente · 2026-10-28T20:11:51Z

@codex review

leoparente · 2026-10-28T20:11:53Z

/ai-review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 019e44237d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

If a reused input's last other policy is removed while a failing load still counts on it, remove_policy leaves the input and its resources policy running, and nothing rechecks them once the rollback detaches the policy. The rollback now detaches each attached input under the policy map lock, as remove_policy does, and for a reused input runs the same teardown, now shared as _remove_input_if_unused. A resources policy whose registration fails is stopped at once, which detaches it from its input, instead of staying attached until the rollback removes that input. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

leoparente · 2026-10-28T20:22:21Z

Fixed in eb1b6f8: AI review finding "Resources policy is attached to its input before registration succeeds, leaving a dangling pointer if module_add throws".

When module_add throws, the resources policy is still owned by the load: module_add throws before it takes ownership. So it is now stopped right there, which also detaches it from its input, before the error goes to the rollback.

In practice the pointer could not have been dereferenced, because that input is always one this load created and the rollback removes it. The fix costs nothing and closes the gap.

A new test, "Bad Config: a resources policy that cannot be registered is rolled back", covers that failure. It registers a user policy under the name the resources policy needs, then checks that the failed load leaves no policy, input or handler behind, and that the policy that owns the name keeps running.

leoparente · 2026-10-28T20:22:30Z

@codex review

leoparente · 2026-10-28T20:22:31Z

/ai-review

nbl-ai-review · 2026-10-28T20:23:01Z

🔁 AI Code Review updated for eb1b6f89 — see the review · 1 resolved · 0 open (run)

chatgpt-codex-connector · 2026-10-28T20:24:24Z

Codex Review: Didn't find any major issues. Keep it up!

Reviewed commit: eb1b6f8901

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

leoparente and others added 3 commits October 28, 2026 15:56

test(policies): load and remove policies concurrently without a deadlock

15315b9

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

chatgpt-codex-connector Bot reviewed Oct 28, 2026

View reviewed changes

Comment thread src/Policies.cpp Resolved

leoparente marked this pull request as ready for review October 28, 2026 20:07

leoparente self-assigned this Oct 28, 2026

chatgpt-codex-connector Bot reviewed Oct 28, 2026

View reviewed changes

Comment thread src/Policies.cpp Resolved

chatgpt-codex-connector Bot reviewed Oct 28, 2026

View reviewed changes

Comment thread src/Policies.cpp Resolved

samiura approved these changes Oct 28, 2026

View reviewed changes

leoparente merged commit 9ddfc65 into develop Oct 28, 2026
23 checks passed

leoparente deleted the fix/policy-load-rollback branch October 28, 2026 20:39

leoparente mentioned this pull request Oct 29, 2026

fix(inputs): fail a flow or dnstap listener that cannot bind instead of running without it #805

Merged

Conversation

leoparente commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Still open (older than this change)

Uh oh!

nbl-ai-review Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LCOV of commit eb1b6f8 during Debug Builds #258

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

leoparente commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

leoparente commented Oct 28, 2026

Uh oh!

nbl-ai-review Bot commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chatgpt-codex-connector Bot commented Oct 28, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

leoparente commented Oct 28, 2026 •

edited

Loading

nbl-ai-review Bot commented Oct 28, 2026 •

edited

Loading

github-actions Bot commented Oct 28, 2026 •

edited

Loading

LCOV of commit `eb1b6f8` during Debug Builds #258

chatgpt-codex-connector Bot commented Oct 28, 2026 •

edited

Loading

leoparente commented Oct 28, 2026 •

edited

Loading

nbl-ai-review Bot commented Oct 28, 2026 •

edited

Loading