ILVerify does not check code that follows an unverifiable instruction #136177

TL;DR: ILVerify can miss invalid IL after an earlier error that only makes the IL unverifiable. Should it keep checking after those errors by default? In my scans, this changes only the errors reported for methods that already fail. The framework scan reports about 3.6 times as many errors, mostly InitLocals, while losing some errors that were reported before.

Repro

.method public hidebysig static int32 M() cil managed
{
    ldc.i4.8
    localloc
    pop
    br.s        Next

Next:
    ldnull      // invalid: an object reference returned as int32
    ret
}

ilverify on main reports only the localloc:

[IL]: Error [Unverifiable]: [Repro.dll : C::M()][offset 0x00000001] Instruction cannot be verified.
1 Error(s) Verifying Repro.dll

The ret at offset 7 is never checked. With the change proposed below:

[IL]: Error [Unverifiable]: [Repro.dll : C::M()][offset 0x00000001] Instruction cannot be verified.
[IL]: Error [StackUnexpected]: [Repro.dll : C::M()][offset 0x00000007][found Nullobjref 'NullReference'][expected Int32] Unexpected type on the stack.
2 Error(s) Verifying Repro.dll

Cause

PropagateControlFlow in ILImporter.Verify.cs returns early when _currentBasicBlock.ErrorCount > 0. Every report increments ErrorCount, including Unverifiable() and InitLocals. So a block with any error never gives its stack to its branch, fallthrough or switch targets, and code that can only be reached through it is not imported at all. leave targets and exception handlers are still imported.

Impact

I measured the .NET 10.0.12 shared framework (172 assemblies, 130,146 method bodies, linux-x64), with #136169 as the baseline. In verification mode ILVerify reports 84,235 errors in 35,248 methods. 78,246 of them are InitLocals, because most of the framework is compiled with SkipLocalsInit. In such a method, a path stops at the first block that reads a local; other paths, exception handlers and leave targets are still checked.

With the change below, the same 35,248 methods fail, and no other method does. They report 305,635 errors. InitLocals at further reads of locals accounts for a net increase of 218,804. The other new errors are mostly further unverifiable code (unmanaged pointers, localloc, conversions of &). Validation-only mode in #136169 also reports 236 of the newly reached errors: 149 BadTypeSpec (#136142), 57 MethodAccess and 30 StackUnexpected. The 236 reports in validation-only mode match the newly reached reports in verification mode by method and error code, and by offset when one is present. BadTypeSpec reports have no offset. The 57 MethodAccess errors are a separate ILVerify bug in the access check for protected interface members.

Four package sets that I scanned (388,490 method bodies) behave the same way: no method changes between passing and failing, and 39,774 errors become 100,188.

Proposal

Keep propagating a block's stack after the verifiability-only errors described below, and keep stopping after other errors.

#136169 already reports the rules that apply only to verifiable IL through UnverifiableError and CheckVerifiable. A second count on BasicBlock tracks those reports, and PropagateControlFlow stops if the block reported any other error. For most errors reported through these helpers, the importer pushes what the instruction always pushes, so the stack it passes on is sound. The exceptions are ldind.ref and ldelem.ref through a pointer or array whose element is not an object reference. In verification mode, these instructions push that element type instead of an object reference, so propagation still stops after this error. FatalCheck and AbortBasicBlockVerification end the method as before.

Some checks for IL that validation-only mode accepts, such as assignment mismatches, still report through VerificationError, so propagation still stops after them.

I have a draft PR, stacked on #136169, that does this, with tests for each part.

Questions

1. Should this be the default? In my scans it changes no method's pass/fail result, only the list of errors for methods that already fail, so I have not added an option.
2. InitLocals is reported at every read of a local, so the total error count on the framework rises to about 3.6 times its current value. Is that acceptable, or should InitLocals be reported once per method (as a separate change)?
3. If checking more code reaches an error that ends verification of the whole method, ILVerify can omit errors that it reported before. Compared with ILVerify: add a validation-only mode #136169, 425 errors in 136 framework methods are lost this way, and all of these methods still fail. In 126 of these methods, verification ends because a basic block is aborted ([ILVerify] Fix AbortBasicBlockVerification aborting the entire method #37392). Fixing [ILVerify] Fix AbortBasicBlockVerification aborting the entire method #37392 would let verification continue past that block. The other 10 end with BadTypeSpec, which abandons the method anyway. Is that loss acceptable?