Fix portable PDB lifetime management to prevent AccessViolationException#1188
Fix portable PDB lifetime management to prevent AccessViolationException#1188Sasinkas wants to merge 11 commits into
Conversation
…n during SourceLink extraction
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Replace foreach-with-early-return pattern with LINQ FirstOrDefault() for clearer intent when getting the first PDB document - Replace manual try/finally dispose with using statements for cleaner resource management Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cks and clarify regression test comments
…ion during SourceLink extraction and add regression tests
martin-reznik
left a comment
martin-reznik
left a comment
There was a problem hiding this comment.
Thanks for the fix — the lifetime model is correct and the IcM 798776046 root cause is well captured in code comments. Two small change requests before this lands; both are self-contained and won't change the fix itself.
1. Silent return on missing test fixture (×2 occurrences). The .csproj deploys FunctionalTestData\**\* via wildcard, so a missing DLL/PDB means a broken deployment. Silently passing the test in that case green-washes a real CI problem. Replace with an explicit assert so the failure mode is visible. Inline suggestions on the two lines below.
2. Document the lambda's stream-ownership contract in PE.cs. The lambda writes to this.portablePdbStream as a side effect, and the fact that this doesn't leak across multiple PEReader invocations relies on PEReader internally disposing rejected streams. Future maintainers will look at this and wonder. A short comment makes the contract explicit. Inline suggestion below.
Non-blocking nit (description): The PR body is just a truncated repeat of the title. Please add a paragraph covering: root cause (missing root + dispose, latent since #657 in 2022), the .NET 9 + Dynamic PGO trigger that surfaced the latent bug, the IcM reference, and what's covered by the new tests (file-handle leak) vs. what isn't (the original AVE — Dynamic PGO + GC race, impractical to reproduce deterministically). Helps anyone landing on this PR without IcM access.
Once these are in I'm happy to approve.
Sasinkas
changed the title
…imeTests.cs Co-authored-by: Martin Řezník <marez@microsoft.com>
…imeTests.cs Co-authored-by: Martin Řezník <marez@microsoft.com>
Co-authored-by: Martin Řezník <marez@microsoft.com>
3 participants
IcM 798776046 — BinSkim crashes with an AV in rule BA2027 (EnableSourceLink) when scanning large codebases with --threads 10: System.AccessViolationException: Attempted to read or write protected memory.
at System.Text.Ascii.WidenAsciiToUtf16(Byte*, Char*, UIntPtr)
at Microsoft.CodeAnalysis.BinaryParsers.PortableExecutable.PE.ManagedPdbGetSourceLinkDocument(Pdb)
RootCause:
TryGetPortablePdbMetadataReader opens a MetadataReaderProvider (which owns the unmanaged memory backing MetadataReader, / BlobReader but never roots it past the method return. Under .NET 9's Dynamic PGO with aggressive GC, the provider is collected while BlobReader.ReadUTF8 still points at its memory. Additionally, the FileStream used to open the PDB is never disposed, leaking file handles for the process lifetime.
Fix:
The fix addresses a GC lifetime bug where TryGetPortablePdbMetadataReader opened a portable PDB's MetadataReaderProvider and FileStream as local variables that were never rooted on the PE instance—allowing the GC (especially under .NET 9 Dynamic PGO) to collect the provider while BlobReader still pointed into its memory, causing AccessViolationException, and simultaneously leaking the file handle. The solution promotes the provider, stream, and metadata reader to PE instance fields with lazy-init semantics (guarded by a portablePdbInitialized flag), so the PDB data remains valid for the entire analysis lifetime of the PE object, and PE.Dispose() deterministically releases both the provider and the stream.