Skip to content

Protect HTTP cleanup callback handoff #957

Open
jhugard wants to merge 2 commits intomainfrom
fix-http-cleanup-callback-race
Open

Protect HTTP cleanup callback handoff #957
jhugard wants to merge 2 commits intomainfrom
fix-http-cleanup-callback-race

Conversation

@jhugard
Copy link
Copy Markdown
Collaborator

@jhugard jhugard commented Apr 10, 2026

Fixes #956

Summary

This change tightens the handoff between HTTP cleanup and HTTP completion so cleanup no longer touches the client-owned XAsyncBlock after the completion path has claimed callback ownership.

Problem

Cleanup and completion can overlap on the same HTTP perform. In that window, cleanup can still treat the request as cancelable through the client-owned XAsyncBlock even after the completion callback has started. The API documentation allows the callback to free that XAsyncBlock, which can turn a later XAsyncCancel into a use-after-free.

What changed

  • Track active HTTP performs by HttpPerformContext instead of raw client XAsyncBlock pointers.
  • Add an explicit handoff state for the client-owned async block during cleanup and completion.
  • Snapshot requests for cancellation under the NetworkState mutex and issue XAsyncCancel outside the lock.
  • Keep the completion path from publishing callback ownership until cleanup has finished its cancel propagation for that request.

Validation

  • Added the repro regression in the first commit on this branch.
  • Verified the repro state fails the Windows x64 Debug TAEF suite.
  • Verified this fix restores the suite to green: Summary: Total=83, Passed=83, Failed=0, Blocked=0, Not Run=0, Skipped=0.

Notes

This branch is intentionally split into two commits: the repro commit and the fix commit.

jhugard added 2 commits April 9, 2026 18:46
Add a unit-test-only NetworkState hook that reports whether cleanup can still cancel a request while the client's HTTP completion callback is running.

Add TestHttpCallCompletionCallbackIsNotCleanupCancelable in GlobalTests to capture the current behavior with a mocked HTTP call. The test asserts that the request should no longer be cleanup-cancelable once the client callback is executing, which reproduces the lifetime bug that will be addressed in a follow-up fix commit.
Track active HTTP performs by HttpPerformContext and add an explicit handoff state for the client-owned XAsyncBlock during cleanup and completion.

The cleanup path now snapshots requests under the NetworkState mutex, marks cancel-in-flight requests, and issues XAsyncCancel outside the lock. The completion path waits until cleanup finishes cancel propagation before publishing callback ownership and completing the client async block.

This preserves the repro commit as the first commit on the branch and records the fix as the second commit.
@jhugard jhugard force-pushed the fix-http-cleanup-callback-race branch from e9d7154 to 2e5386e Compare April 10, 2026 01:47
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses issue #956 by preventing HCCleanupAsync from interacting with a client-owned XAsyncBlock after the HTTP completion path has taken ownership of the callback (and the client may legally free that block). The change adds explicit state coordination between cleanup-driven cancellation and perform completion to eliminate a potential use-after-free race.

Changes:

  • Track active HTTP performs via HttpPerformContext* rather than raw client XAsyncBlock*.
  • Add an explicit atomic handoff state (CleanupMayCancel / CleanupCancelInFlight / CallbackStarted) to coordinate cleanup cancellation vs. completion callback ownership.
  • Snapshot active HTTP requests under the NetworkState mutex and issue XAsyncCancel outside the lock; add a unit test regression for the callback ownership guarantee.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
Tests/UnitTests/Tests/GlobalTests.cpp Adds a regression test asserting cleanup cannot treat an HTTP request as cancelable once the completion callback begins.
Source/Global/NetworkState.h Changes the active HTTP request tracking set to store perform contexts; exposes a unit-test-only query API.
Source/Global/NetworkState.cpp Implements the perform-context tracking and the cleanup/completion handoff state machine; adjusts cleanup cancellation to operate on snapshotted contexts.

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.

Potential use-after-free during HTTP cleanup/completion overlap

2 participants