Skip to content

fix(coro): protect selected completion wakes - #1050

Merged
Coldwings merged 2 commits into
mainfrom
fix/completion-wake-lease
Aug 13, 2026
Merged

fix(coro): protect selected completion wakes#1050
Coldwings merged 2 commits into
mainfrom
fix/completion-wake-lease

Conversation

@Coldwings

Copy link
Copy Markdown
Owner

Description

Fixes a dequeue-to-schedule use-after-free in the common completion-waiter path. Completion previously copied a raw coroutine handle out of completion_waiter_slot, cleared the registration, and scheduled the copied handle after unlocking. A concurrent destruction of the suspended coroutine could then reclaim the frame before scheduling used that handle.

This change makes take() select a slot-owned wake and return a move-only lease. The producer must claim that lease before it obtains the coroutine handle. Destruction of the registered waiter before claim abandons the selected generation, so claim returns an empty handle instead of scheduling freed frame storage.

The supported boundary remains explicit: waiter destruction can revoke a registered or selected-but-unclaimed wake. Once claim() transfers scheduling ownership, external forced destruction is outside this contract.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality not to work as expected)
  • Performance improvement (optimization that improves speed/memory usage)
  • Documentation (changes to documentation, comments, or examples)
  • Refactoring (code changes that neither fix bugs nor add features)
  • Tests (adding or modifying tests)
  • Build/CI (changes to build system, CI configuration, or dependencies)

Related Issues

Closes #1049
Related to #390
Related to #762
Related to #1046

Changes Made

Core Changes

  • Added a move-only completion_wake_lease backed by slot-owned selected-handle state and an odd/even generation.
  • Changed completion_waiter_slot::take() from returning a raw coroutine handle to selecting one wake under the slot mutex and returning its lease.
  • Added generation-checked claim() and abandonment transitions. Waiter destruction and lease destruction can invalidate a selected wake before scheduling ownership transfers, while stale leases cannot affect a reused slot generation.
  • Preserved waiter move semantics in both registered and selected states, and made a repeated take() while a generation is already selected return an empty lease without disturbing the first producer.
  • Migrated join states, both task-state specializations, task-group completion, object-cache release handoff, and RDMA pump-exit completion to claim the lease before scheduling.
  • Preserved task-group scheduler-domain routing by moving the lease through its existing custom routing path and claiming immediately before routing.
  • Added deterministic regressions for join handles, task handles, task groups, and object-cache release waits, plus common state-machine tests for completion-before-registration, lease destruction, waiter moves, stale generations, slot reuse, and repeated selection.
  • Updated the object-lifetime contract and changelog.

API Changes (if applicable)

No public API changes. Public awaiter signatures and exception specifications are unchanged. The return-type change is confined to the internal coro::detail::completion_waiter_slot protocol.

Before:

std::coroutine_handle<> completion_waiter_slot::take() noexcept;

After:

completion_wake_lease completion_waiter_slot::take() noexcept;
std::coroutine_handle<> completion_wake_lease::claim() noexcept;

Migration Guide (if breaking change)

Not applicable.

Testing

Unit Tests

  • Added new tests for the changes
  • Updated existing tests if needed
  • All tests pass locally

Integration Tests

  • Tested with existing examples
  • Tested in real-world scenarios (if applicable)

Sanitizer Testing

  • Tested with ASAN (AddressSanitizer)
  • Tested with TSAN (ThreadSanitizer)
  • No new warnings or errors

Test Results

Focused final-head regressions:
  64 assertions / 5 cases passed

Full normal all-target build and CTest:
  837 / 837 tests passed

ASAN:
  823 / 823 tests passed; no diagnostics

TSAN:
  no ThreadSanitizer diagnostics
  821 / 822 selected CTest cases passed
  the remaining case explicitly SKIPs its fork-based regression under TSAN;
  CTest classifies that Catch2 skip as a non-pass

All builds were out of source with --parallel 2. The normal all-target run included enabled RDMA stub/CUDA lifetime and fork-helper targets. A direct libibverbs syntax build was unavailable because the host does not provide infiniband/verbs.h.

Checklist

Code Quality

  • My code follows the project's code style
  • I have added/updated comments for complex logic
  • I have removed any debug code, TODOs, or commented-out code
  • My changes generate no new warnings

Documentation

  • I have updated documentation (wiki, README, code comments)
  • I have added examples for new features (not applicable; there is no new public feature)
  • I have updated API documentation (if applicable)

Testing

  • I have added tests that prove my fix is effective or my feature works
  • New and existing unit tests pass locally with my changes
  • I have tested with ASAN and TSAN

Compatibility

  • My changes are backward compatible (or I've documented breaking changes)
  • I have considered the impact on existing users
  • I have updated CHANGELOG.md (if applicable)

Performance (if applicable)

  • I have considered the performance impact
  • I have benchmarked the changed ownership mechanism outside the repository

Screenshots / Diagrams

Not applicable.

Additional Notes

The fix adds no allocation. A same-source Release mechanism benchmark measured 5,000,000 register -> take -> ownership transitions per sample, pinned to one CPU, with 12 interleaved pairs. Both implementations reported zero allocations.

Baseline raw take median:       14.50 ns
Lease take + claim median:      22.28 ns
Candidate / baseline median:    approximately 1.54x

The approximately 54% direct mechanism cost is expected: eliminating the UAF requires a second arbitration after selection so waiter destruction can abandon the selected generation before scheduling ownership transfers. This is a deliberately narrow mechanism diagnostic, not an end-to-end workload result. A one-child task-group spawn/join control measured approximately +0.8% by median (+1.2% paired median), where scheduling and state management amortize the extra arbitration.

On the tested GCC 12 x86_64 ABI:

Type Before After
completion_waiter_slot 48 B 64 B
completion_waiter 16 B 16 B
completion_wake_lease n/a 16 B
join_state_base 192 B 192 B
task_state<void> / task_state<int> 256 B 256 B
task_group_completion_state 104 B 120 B

The cache-line-aligned join/task states absorb the slot growth without changing total size. Other one-shot states embedding an unaligned slot may grow by 16 bytes depending on ABI and surrounding layout.

Reviewer Guidance

Areas requiring special attention:

  • The registered -> selected -> claimed/abandoned generation transitions in completion_waiter.hpp.
  • Waiter move/destruction and stale-lease behavior while a wake is selected.
  • Slot-owner lifetime through claim in task-group, object-cache, and RDMA consumers.
  • Preservation of task-group scheduler-domain routing after the lease is claimed.
  • The explicit ownership boundary after claim().

Questions for reviewers:

  • Does the lease transfer make the destroy-before-claim boundary explicit and race-free for every migrated consumer?
  • Is the zero-allocation size and direct mechanism cost acceptable for closing the UAF?
  • Are there any additional completion-waiter consumers that should be migrated before merge?

Thank you for contributing to Elio! 🎉

Copilot AI balanced review requested due to automatic review settings August 13, 2026 09:40

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes completion-waiter use-after-free races by introducing a generation-checked wake lease before scheduling.

Changes:

  • Adds slot-owned wake selection, claiming, and abandonment.
  • Migrates all completion-waiter consumers.
  • Adds lifetime regressions and updates documentation.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
include/elio/coro/detail/completion_waiter.hpp Implements wake leases and generation transitions.
include/elio/coro/task.hpp Claims join completion wakes.
include/elio/coro/task_handle.hpp Claims task completion wakes.
include/elio/coro/task_group.hpp Preserves routed scheduling with leases.
include/elio/sync/object_cache.hpp Protects release handoff wakes.
include/elio/rdma_ibverbs/endpoint.hpp Protects pump-exit wakes.
tests/unit/test_task.cpp Tests lease and task lifetime transitions.
tests/unit/test_task_group.cpp Tests task-group abandonment.
tests/unit/test_object_cache.cpp Tests release-wait abandonment.
wiki/API-Contracts.md Documents the ownership boundary.
CHANGELOG.md Records the lifetime fix.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread include/elio/rdma_ibverbs/endpoint.hpp
@Coldwings

Copy link
Copy Markdown
Owner Author

@copilot Please review the updated pull request only. The new commit adds the requested deterministic RDMA pump-exit destroy-after-selection regression.

Copilot AI commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

@copilot Please review the updated pull request only. The new commit adds the requested deterministic RDMA pump-exit destroy-after-selection regression.

Reviewed ffe253a. The new section in test_rdma_ibverbs_endpoint.cpp deterministically pauses pump-exit completion after wake selection (via pause_before_completion_wake_claim_for_test), destroys the suspended waiter frame, then verifies that is_exited() is set without the continuation being resumed. This directly mirrors the pattern used for other migrated consumers and fully covers the destroy-before-claim path for the RDMA pump-exit consumer. No further changes needed.

@Coldwings
Coldwings merged commit a653e35 into main Aug 13, 2026
11 checks passed
@Coldwings
Coldwings deleted the fix/completion-wake-lease branch August 13, 2026 10:14
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.

[Bug] Completion waiter dequeue can schedule a destroyed coroutine frame

3 participants