Fail fast when FirstChanceExceptionEventArgs allocation fails#130505
Conversation
|
@copilot have the actual implementation got dropped from the changes somehow? |
Yes — the branch only had the empty "Initial plan" commit, so the implementation was missing. I've now pushed it. Note that since the original PR body's snippet was written, #124834 moved the So the fix moves the allocation back into the VM in EX_TRY
{
MethodTable *pMTEventArgs = CoreLibBinder::GetClass(CLASS__FIRSTCHANCE_EVENTARGS);
gc.oEventArgs = AllocateObject(pMTEventArgs);
MethodDescCallSite ctor(METHOD__FIRSTCHANCE_EVENTARGS__CTOR, &gc.oEventArgs);
ARG_SLOT ctorArgs[] = { ObjToArgSlot(gc.oEventArgs), ObjToArgSlot(gc.oThrowable) };
ctor.Call(ctorArgs);
}
EX_CATCH
{
EEPOLICY_HANDLE_FATAL_ERROR(COR_E_OUTOFMEMORY);
UNREACHABLE();
}
EX_END_CATCHThe pre-created event args is then passed to the managed handler-dispatch path (the CoreCLR Validated on a fresh clr+libs+host build: normal flow (handler called once with non-null args) and handler-throws (nested first-chance still delivered with valid args, no spurious fail-fast) both pass. Note This comment was generated by Copilot. |
f138cc3 to
f27edd1
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes first-chance exception notification delivery to be non-reentrant on the same thread, preventing recursive AppDomain.FirstChanceException dispatch when event-args allocation or a handler throws, and adds a regression test covering the “handler throws” scenario.
Changes:
- Add a
[ThreadStatic]reentrancy guard inSystem.AppContext.OnFirstChanceExceptionto skip nested first-chance notifications on the current thread. - Keep swallowing exceptions from individual first-chance handlers, but ensure the guard is always reset via
try/finally. - Add a
RemoteExecutortest that validates a throwing handler does not lead to recursion / stack overflow.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/AppDomainTests.cs | Adds a regression test ensuring a throwing FirstChanceException handler doesn’t recurse. |
| src/libraries/System.Private.CoreLib/src/System/AppContext.cs | Introduces a thread-local guard to prevent re-entrant first-chance dispatch on the same thread. |
| EventHandler<FirstChanceExceptionEventArgs> handler = (sender, e) => | ||
| { | ||
| count++; | ||
| throw new FirstChanceTestException("from handler"); | ||
| }; |
39abf8c to
efa983a
Compare
| catch (Exception allocationFailure) | ||
| { | ||
| // Failing to allocate the event args (e.g. OutOfMemoryException in a | ||
| // low-memory situation) leaves us unable to deliver the notification. | ||
| // Fail fast rather than proceeding with invalid state. |
efa983a to
694864a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
src/libraries/System.Private.CoreLib/src/System/AppContext.cs:146
- The PR description claims the fix is in CoreCLR native allocation (
AllocateObject(...)returning NULL) and that only the allocation-failure path changes. The actual change is entirely in managedAppContext.OnFirstChanceException, and it also introduces a general reentrancy suppression (t_deliveringFirstChanceNotification) that changes behavior whenever a FirstChanceException handler (or args allocation) throws. Please update the PR description to match the implemented behavior and scope so reviewers understand what is actually changing.
// Guard against reentrancy. Allocating the event args below or running a
// handler may itself throw (e.g. OutOfMemoryException in a low-memory
// situation). That exception would trigger another first-chance
// notification on this same thread, allocate again, throw again, and
// recurse until the stack overflows. Skip nested notifications to break
// the recursion.
if (t_deliveringFirstChanceNotification)
src/libraries/System.Private.CoreLib/src/System/AppContext.cs:165
Environment.FailFast(...)is annotated as non-returning; the subsequentthrow; // unreachableis redundant and makes the catch block look like it can continue. Prefer removing the unreachable rethrow.
Environment.FailFast("Failed to allocate FirstChanceExceptionEventArgs.", allocationFailure);
throw; // unreachable
Allocate the FirstChanceExceptionEventArgs in the VM during first-chance dispatch and fail fast if the allocation fails, instead of allocating in managed code (which would recurse to a stack overflow on OOM) or passing a null event args to handlers. Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
… MethodDescCallSite Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Co-authored-by: VSadov <8218165+VSadov@users.noreply.github.com>
694864a to
6602515
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/libraries/System.Private.CoreLib/src/System/AppContext.cs:166
- The catch block is intended to handle event-args allocation failure, but it currently catches all exceptions and then includes a
throw;that should never be reached becauseEnvironment.FailFastis[DoesNotReturn]. Narrowing the catch toOutOfMemoryExceptionbetter matches the intent, and removing the redundant rethrow avoids leaving dead/unreachable code in the method.
catch (Exception allocationFailure)
{
// Failing to allocate the event args (e.g. OutOfMemoryException in a
// low-memory situation) leaves us unable to deliver the notification.
// Fail fast rather than proceeding with invalid state.
Environment.FailFast("Failed to allocate FirstChanceExceptionEventArgs.", allocationFailure);
throw; // unreachable
}
|
@jkotas any concerns in the latest iteration? |
main PR N/A
Description
First-chance exception dispatch could invoke managed handlers with a null
FirstChanceExceptionEventArgswhen allocation failed, which can recurse and end in stack overflow. This change makes that allocation-failure path terminate via fail-fast instead of calling into managed handlers with invalid state.Behavior change
FirstChanceExceptionEventArgsfails, runtime now fail-fasts immediately.AppDomain.FirstChanceExceptionhandlers with a null event args instance.Implementation
FirstChanceExceptionEventArgsallocation failure as fatal.Customer Impact
Prevents unbounded recursion/stack overflow in low-memory exception paths and replaces it with deterministic fail-fast behavior.
Regression
Not identified as a recent regression; this addresses a longstanding edge case in an OOM path.
Testing
Covered by existing first-chance exception behavior tests for normal flow; this change is isolated to the fatal allocation-failure branch.
Risk
Low. Change is narrowly scoped to an already-fatal low-memory edge path and does not affect normal exception dispatch semantics.
Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.