Skip to content

Fix ILVerify entry stacks for nested exception filters - #134749

Open
pkuyo wants to merge 4 commits into
dotnet:mainfrom
pkuyo:fix-ilverify-134711
Open

pkuyo wants to merge 4 commits into
dotnet:mainfrom
pkuyo:fix-ilverify-134711

Conversation

@pkuyo

@pkuyo pkuyo commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #134711.

Fix incorrect entry stack initialization for nested exception filters, which caused false verification errors.
Validate EH boundaries, overlaps, nesting, clause ordering, and filter termination, including unreachable regions. (And reject method entry inside a handler or filter.)

Add regression tests.

(Assisted by Codex)

@github-actions github-actions Bot added the area-Tools-ILVerification Issues related to ilverify tool and IL verification in general label Sep 27, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
13 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Sep 27, 2026
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The focused implementation addresses the root cause and includes comprehensive regression coverage.

Review effort: Balanced
Findings: None

What changed in this PR

Fixes ILVerify’s stack initialization for filters nested within exception handlers.

Changes:

  • Prioritizes a filter’s own region when initializing its entry stack.
  • Diagnoses conflicting filter/handler entry offsets.
  • Adds five regression tests covering valid and invalid nesting.
File Description
ILImporter.Verify.cs Corrects region selection and validates conflicting entry points.
VerifierError.cs Adds the new verifier error code.
Strings.resx Adds the diagnostic message.
ExceptionRegionTests.il Adds nested-filter regression coverage.

Comment thread src/coreclr/tools/ILVerification/ILImporter.Verify.cs Outdated
Comment thread src/coreclr/tools/ILVerification/ILImporter.Verify.cs Outdated
@pcshrosbree

Copy link
Copy Markdown
Contributor

Thanks for fixing this. In case it helps the rework, I built ILVerification and ilverify at 2e95101 and at its merge base (33baf8e, "main" below), on Linux, macOS and Windows:

  • The Debug.Fail from the review thread is reachable with malformed IL. A zero-length catch handler trips it in both Debug builds; both Release builds accept it. Inside a filter, the same handler passes on main but newly trips it here (Release: StackUnderflow).
  • A filter sharing its start with an enclosing catch: main accepts it; this branch reports FilterAndHandlerSameOffset. Without the pre-pass, the new selection verifies both reachable and unreachable shared starts, so that coverage matters if the check moves to an EH-layout pass.
  • For that pass, unchanged by this PR: a try starting at a catch's or filter's first instruction (exception on the stack) is accepted by both.

Happy to post the IL as test cases.

Catch-when inside finally from C# (plain, nested, lambda, async MoveNext) and VB goes from StackUnderflow to clean. On Linux, output is unchanged for all 313 NETCore.App and AspNetCore.App 11.0.0-preview.7 assemblies, and across 466 assemblies in my NuGet cache this branch drops two StackUnderflows (in FSharp.Compiler.Service and Microsoft.Build.Tasks.Core) and adds none.

Note

This comment was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Equal-end nested tries can lose their entry marker, and the new all-pairs validator has unbounded quadratic cost.

3 open findings

🧠 Review effort: Balanced

Comment thread src/coreclr/tools/ILVerification/ILImporter.ExceptionRegions.cs Outdated
Comment thread src/coreclr/tools/ILVerification/ILImporter.Verify.cs
Comment thread src/coreclr/tools/ILVerification/ILImporter.Verify.cs
@pkuyo

pkuyo commented Oct 11, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for fixing this. In case it helps the rework, I built ILVerification and ilverify at 2e95101 and at its merge base (33baf8e, "main" below), on Linux, macOS and Windows:

  • The Debug.Fail from the review thread is reachable with malformed IL. A zero-length catch handler trips it in both Debug builds; both Release builds accept it. Inside a filter, the same handler passes on main but newly trips it here (Release: StackUnderflow).
  • A filter sharing its start with an enclosing catch: main accepts it; this branch reports FilterAndHandlerSameOffset. Without the pre-pass, the new selection verifies both reachable and unreachable shared starts, so that coverage matters if the check moves to an EH-layout pass.
  • For that pass, unchanged by this PR: a try starting at a catch's or filter's first instruction (exception on the stack) is accepted by both.

Happy to post the IL as test cases.

Catch-when inside finally from C# (plain, nested, lambda, async MoveNext) and VB goes from StackUnderflow to clean. On Linux, output is unchanged for all 313 NETCore.App and AspNetCore.App 11.0.0-preview.7 assemblies, and across 466 assemblies in my NuGet cache this branch drops two StackUnderflows (in FSharp.Compiler.Service and Microsoft.Build.Tasks.Core) and adds none.

Note

This comment was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting.

Thanks! Could you share the IL cases? I'd like to check them against the updated EH validation and add any missing coverage.

@pcshrosbree

Copy link
Copy Markdown
Contributor

Sure. The eight methods below are in the test file's style and were run with the Release and Debug ilverify builds at 72a6735, plus RuntimeHelpers.PrepareMethod under .NET 10.0.12 as a second opinion on what the JIT accepts. I left out the try-at-catch-entry and empty-handler cases already covered by your tests, including the empty handler inside a filter. Several cases below are variants of existing coverage; I kept the catch-in-finally case as a JIT-accepted control for Finally.BeforeNestedFilterInTable_Valid.

IL cases (names carry the verdicts 72a6735 reports; all agree with the JIT)
    // Same conflict as Filter.SharesFinallyStart, but the enclosing handler is a catch:
    // both entries take one object, so only the layout pass can see it.
    .method public static void Filter.SharesCatchStart_Invalid_EHClauseOverlap() cil managed
    {
        .maxstack 1
    OuterTry:
        nop
        leave MethodEnd
    SharedStart:
        pop
        ldc.i4.1
        endfilter
    InnerHandler:
        pop
        leave MethodEnd
    InnerTry:
        nop
        leave MethodEnd
    MethodEnd:
        ret
        .try InnerTry to MethodEnd filter SharedStart handler InnerHandler to InnerTry
        .try OuterTry to SharedStart catch [System.Runtime]System.Exception handler SharedStart to MethodEnd
    }

    // Counterpart of Try.StartsAtCatchEntry with a filter: the try is also nested in the filter.
    .method public static void Try.StartsAtFilterEntry_Invalid_EHClauseNesting.TryNonEmptyStack() cil managed
    {
        .maxstack 1
    OuterTry:
        nop
        leave MethodEnd
    FilterStart:
        pop
        leave FilterEnd
    InnerHandler:
        pop
        leave FilterEnd
    FilterEnd:
        ldc.i4.1
        endfilter
    OuterHandler:
        pop
        leave MethodEnd
    MethodEnd:
        ret
        .try FilterStart to InnerHandler catch [System.Runtime]System.Exception handler InnerHandler to FilterEnd
        .try OuterTry to FilterStart filter FilterStart handler OuterHandler to MethodEnd
    }

    // A filter nested in another filter's handler.
    .method public static void Filter.NestedInFilterHandler_Valid() cil managed
    {
        .maxstack 1
        .try
        {
            nop
            leave MethodEnd
        }
        filter
        {
            pop
            ldc.i4.1
            endfilter
        }
        {
            pop
            .try
            {
                nop
                leave MethodEnd
            }
            filter
            {
                isinst [System.Runtime]System.Exception
                pop
                ldc.i4.1
                endfilter
            }
            {
                pop
                leave MethodEnd
            }
        }
    MethodEnd:
        ret
    }

    // Catch counterpart of Finally.BeforeNestedFilterInTable_Valid (see the note below on that test).
    .method public static void Finally.BeforeNestedCatchInTable_Valid() cil managed
    {
        .maxstack 1
    OuterTry:
        leave MethodEnd
    OuterHandler:
        nop
    InnerTry:
        leave InnerEnd
    InnerHandler:
        pop
        leave InnerEnd
    InnerEnd:
        endfinally
    MethodEnd:
        ret
        .try OuterTry to OuterHandler finally handler OuterHandler to MethodEnd
        .try InnerTry to InnerHandler catch [System.Runtime]System.Exception handler InnerHandler to InnerEnd
    }

    // The ordering constraint passes through a handler: the outer try must still come last.
    .method public static void Try.NestedInHandlerInTry_Valid() cil managed
    {
        .maxstack 1
    OuterTry:
        nop
    MidTry:
        leave MethodEnd
    MidHandler:
        nop
    InnerTry:
        leave InnerEnd
    InnerHandler:
        pop
        leave InnerEnd
    InnerEnd:
        endfinally
    MidEnd:
        leave MethodEnd
    OuterHandler:
        pop
        leave MethodEnd
    MethodEnd:
        ret
        .try InnerTry to InnerHandler catch [System.Runtime]System.Exception handler InnerHandler to InnerEnd
        .try MidTry to MidHandler finally handler MidHandler to MidEnd
        .try OuterTry to OuterHandler catch [System.Runtime]System.Exception handler OuterHandler to MethodEnd
    }

    .method public static void Try.NestedInHandlerAfterOuterTry_Invalid_EHClauseOrder() cil managed
    {
        .maxstack 1
    OuterTry:
        nop
    MidTry:
        leave MethodEnd
    MidHandler:
        nop
    InnerTry:
        leave InnerEnd
    InnerHandler:
        pop
        leave InnerEnd
    InnerEnd:
        endfinally
    MidEnd:
        leave MethodEnd
    OuterHandler:
        pop
        leave MethodEnd
    MethodEnd:
        ret
        .try MidTry to MidHandler finally handler MidHandler to MidEnd
        .try OuterTry to OuterHandler catch [System.Runtime]System.Exception handler OuterHandler to MethodEnd
        .try InnerTry to InnerHandler catch [System.Runtime]System.Exception handler InnerHandler to InnerEnd
    }

    .method public static void Handler.SharedByClausesWithSameBounds_Invalid_EHClauseOverlap() cil managed
    {
        .maxstack 1
    TryStart:
        leave MethodEnd
    Catch1:
        pop
        leave MethodEnd
    MethodEnd:
        ret
        .try TryStart to Catch1 catch [System.Runtime]System.ArgumentException handler Catch1 to MethodEnd
        .try TryStart to Catch1 catch [System.Runtime]System.InvalidOperationException handler Catch1 to MethodEnd
    }

    .method public static void Try.CrossesIntoHandler_Invalid_EHClauseOverlap() cil managed
    {
        .maxstack 1
    TryA:
        leave MethodEnd
    HandlerA:
        pop
        nop
    CrossEnd:
        leave MethodEnd
    HandlerB:
        pop
        leave MethodEnd
    MethodEnd:
        ret
        .try TryA to HandlerA catch [System.Runtime]System.Exception handler HandlerA to HandlerB
        .try TryA to CrossEnd catch [System.Runtime]System.Exception handler HandlerB to MethodEnd
    }

Two things the JIT comparison turned up, neither a problem with this PR:

  • Finally.BeforeNestedFilterInTable_Valid is rejected by the .NET 10.0.12 JIT: a checked 10.0.12 build fails it with Unexpected endfilter right after fgSortEHTable swaps the two clauses. As far as I can tell, fgFindBasicBlocks assigns the filter blocks' handler index before the sort and the post-sort pass only fills unset indices, so the filter keeps the stale index. The catch counterpart above is accepted, and the spec only orders try blocks, so the test is right and this looks like a JIT bug.
  • MethodEntry.Finally_Invalid, MethodEntry.Fault_Invalid and MethodEntry.TryInFinally_Invalid are compiled by the .NET 10.0.12 release JIT (the tested catch and filter variants that begin with pop are rejected), and executing any of them overflows the stack; a checked JIT asserts in fgDebugCheckBBlist. So the new method-entry check rejects IL the runtime mishandles, which seems like the right call.

I filed them as #135592 and #135593.

Otherwise the head looks good in these checks: 425/425 tests passed, output was identical to d8f9151 across the 313 NETCore.App and AspNetCore.App 11.0.0-preview.7 assemblies, and the 466 NuGet assemblies differed only in stack-trace line numbers from two pre-existing crashes. For each of the eight IL cases above, ilverify and PrepareMethod agreed on acceptance or rejection.

Note

This comment was prepared with AI assistance (Anthropic Claude and OpenAI Codex), under my direction. I reviewed it before posting.

Comment on lines +2478 to +2479
Check(_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Finally ||
_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Fault, VerifierError.Endfinally);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Check(_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Finally ||
_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind == ILExceptionRegionKind.Fault, VerifierError.Endfinally);
Check(_exceptionRegions[_currentBasicBlock.HandlerIndex.Value].ILRegion.Kind
is ILExceptionRegionKind.Finally or ILExceptionRegionKind.Fault, VerifierError.Endfinally);

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The broad verifier behavior change and new EH-layout algorithm warrant final maintainer review.

0 open findings

3 resolved since last review

🧠 Review effort: Balanced

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Tools-ILVerification Issues related to ilverify tool and IL verification in general community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ILVerify false positive: StackUnderflow at the start of an exception filter nested inside a finally handler

4 participants