dotnet / dotnet/aspnetcore

IIS OutOfProcess ANCM: GetProcess() hands out SERVER_PROCESS* without a caller-owned reference

Open
#68,366 0 comments 0 reactions 0 assignees View on GitHub
area-networking feature-iis
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 5h
Merged PRs (30d)
276

Description

## Summary

The out-of-process IIS ASP.NET Core Module (ANCM) process manager hands out a raw pointer to a reference-counted `SERVER_PROCESS` object to its caller without incrementing the reference count, breaking the ownership invariant the reference-counting scheme is meant to enforce.

## What is wrong

* `SERVER_PROCESS` uses manual reference counting (`ReferenceServerProcess()` / `DereferenceServerProcess()`, the latter calling `delete this` at refcount zero) to manage its own lifetime across multiple owners (the process-manager's list slot and the process-exit wait registration).
* All three success paths of `PROCESS_MANAGER::GetProcess()` return the process-list-slot pointer to the caller without calling `ReferenceServerProcess()` on the caller's behalf. One of the three call sites even has the reference call present in the source but commented out, suggesting it was intended and later dropped or never completed.
* The only consumer of `GetProcess()`, the out-of-process forwarding handler, retains the returned pointer across an asynchronous request lifetime without ever calling `ReferenceServerProcess()` or `DereferenceServerProcess()` on it.
* The process list is protected by a lock, but that lock is released before the caller uses the returned pointer, so it does not protect the pointed-to object's lifetime once the pointer has been handed out. Lifetime protection for the object itself depends entirely on reference counting, which this path bypasses.

## Why it matters (defense in depth)

* Independent of any attacker model, this is a genuine ownership/lifetime bug: a consumer holds an unreferenced pointer to a reference-counted object across an asynchronous operation, with no other mechanism keeping the object alive for the duration of that use.
* Fixing it restores the invariant "every pointer to a reference-counted `SERVER_PROCESS` handed to a caller carries a matching reference for that caller's holding period," which the commented-out call indicates was the original intent.
* Hardens the object-lifetime boundary between the process manager (which owns process lifecycle/recycling) and the forwarding handler (which only borrows the pointer to service a request), reducing the risk of latent lifetime bugs if process recycling or shutdown logic changes in the future.

## Affected code

* src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/processmanager.cpp:96-104 - fast (shared-lock) path returns the list-slot pointer without taking a reference for the caller
* src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/processmanager.cpp:118-129 - exclusive-lock "already ready" path has the reference call present but commented out
* src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/processmanager.cpp:170-179 - newly created process is handed to the caller without a caller-owned reference
* src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/forwardinghandler.cpp:139-157 - consumer never calls `ReferenceServerProcess`/`DereferenceServerProcess` on the pointer it receives from `GetProcess()`
* src/Servers/IIS/AspNetCoreModuleV2/OutOfProcessRequestHandler/serverprocess.h:91-108 - reference-counting primitives (`ReferenceServerProcess`, `DereferenceServerProcess`, `delete this` at refcount zero)

## Recommended fix

Selected approach: call `ReferenceServerProcess()` on the selected process in all three `GetProcess()` success paths, before the lock is released, so the returned pointer always carries a reference owned by the caller; add a matching `DereferenceServerProcess()` call in the forwarding handler once it is finished using the process, covering every early-return/failure path as well as the asynchronous completion path.

Alternatives considered:
* Uncommenting the existing reference call alone — rejected, because the forwarding handler never releases any reference today, so this would convert the missing-reference bug into a per-request reference leak (the object would never reach refcount zero and be freed) instead of fixing the underlying issue.
* Copying only the specific immutable data the caller needs (e.g., the connection handle) while still holding the lock, instead of retaining the object pointer across the request lifetime — viable, but requires auditing every field the forwarding handler reads through the pointer to confirm none of them are needed later in the request pipeline.

Compatibility/migration: this is an internal native-code change with no public API surface, so there is no migration or versioning impact. Because the forwarding handler has many early-return paths plus a separate asynchronous completion callback, this should not be treated as a quick patch — implementation needs a full audit of every path that stops using the pointer, to guarantee the new reference is released exactly once on every path (no leak, no double-release).

## Acceptance criteria

* [ ] Every successful `GetProcess()` call that hands a `SERVER_PROCESS*` to a caller results in a matching reference held for that caller's lifetime.
* [ ] Every code path that stops using a `SERVER_PROCESS*` obtained from `GetProcess()` (including early failure returns and the async-completion path) releases exactly one reference, with no leaks and no double-releases.
* [ ] A concurrency/stress test exercises request dispatch racing against process exit under a memory-safety sanitizer (or equivalent tooling) without reference leaks or use-after-free.
* [ ] Existing OutOfProcess hosting-model tests continue to pass with no behavior change under normal operation.

> Note for implementers: this fix touches lifetime management in a complex native async state machine with limited existing test coverage for this scenario. Prioritize adding the race/stress test coverage described above before or alongside the fix, rather than landing a quick patch to the reference calls alone.

Contributor guide

Open the contributing guide

Research direction

Start with processmanager.cpp, forwardinghandler.cpp, and serverprocess.h, tracing each GetProcess() success path and every asynchronous or early-return path that uses the SERVER_PROCESS pointer. Run the existing OutOfProcess hosting-model tests and add the described concurrency/stress coverage; done means balanced ownership on every path, no leaks or use-after-free, and unchanged normal behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
backend, testing-qa
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.