Replace native PE inspection with PEReader in StrongNameUtils#13731
Conversation
GetAssemblyStrongNameLevel previously walked the PE image by hand via
CreateFile / CreateFileMapping / MapViewOfFile / ImageNtHeader / ImageRvaToVa
plus a 32/64-bit dispatch over IMAGE_NT_HEADERS{32,64} / IMAGE_COR20_HEADER
structs. The whole approach exists only to read
CorHeader.StrongNameSignatureDirectory plus CorFlags.StrongNameSigned, which
System.Reflection.PortableExecutable already gives us in ~20 lines.
PEReader is cross-platform and is already a dependency of Tasks. Switch to
it and delete the now-unused native interop:
- Tasks/NativeMethods.cs: drop [DllImport] CreateFile / GetFileType /
CloseHandle / CreateFileMapping / MapViewOfFile / UnmapViewOfFile,
dbghelp ImageNtHeader / ImageRvaToVa, the IMAGE_* PE constants
(GENERIC_READ, PAGE_READONLY, FILE_MAP_READ, FILE_TYPE_DISK,
IMAGE_NT_OPTIONAL_HDR* magics, IMAGE_DIRECTORY_ENTRY_COMHEADER,
COMIMAGE_FLAGS_STRONGNAMESIGNED), InvalidIntPtr, and the NT header
structs (IMAGE_FILE_HEADER, IMAGE_DATA_DIRECTORY,
IMAGE_OPTIONAL_HEADER32/64, IMAGE_NT_HEADERS32/64, IMAGE_COR20_HEADER).
~260 lines of native interop removed; result works on every platform without
FEATURE_WINDOWSINTEROP.
There was a problem hiding this comment.
Pull request overview
This PR modernizes strong-name state detection in MSBuild Tasks by replacing Windows-only native PE inspection (CreateFile/MapViewOfFile/dbghelp) with System.Reflection.PortableExecutable.PEReader, improving portability and removing unsafe/native interop dependencies.
Changes:
- Rewrote
StrongNameUtils.GetAssemblyStrongNameLevelto usePEReader/CorHeader/CorFlagsinstead of manual PE walking. - Removed now-dead native interop constants/structs/PInvokes from
NativeMethods.cs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Tasks/StrongNameUtils.cs | Switches strong-name detection to PEReader and simplifies the logic/error handling. |
| src/Tasks/NativeMethods.cs | Deletes unused PE-parsing-related native interop after the PEReader migration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Expert MSBuild Review — PR #13731: Replace P/Invoke with PEReader in StrongNameUtils
Excellent cleanup. This PR replaces ~200 lines of unsafe Windows-only P/Invoke (kernel32 file mapping, dbghelp PE parsing, manual struct marshaling) with ~25 lines of clean, cross-platform managed code using System.Reflection.PortableExecutable.PEReader. The corresponding dead code in NativeMethods.cs (8 P/Invoke declarations, 7 structs, and 4 constants) is correctly removed with no remaining references.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Backwards Compatibility | 🟡 1 MODERATE |
| 4 | Test Coverage | 🟡 1 MODERATE |
| 22 | Correctness | i️ 1 NIT |
✅ 21/24 dimensions clean.
Key findings:
- Backwards Compatibility —
StrongNameLevel.Unknown→StrongNameLevel.Nonefor non-.NET PE files (no COR header). This is semantically more correct but is a behavioral change. Practical impact is negligible since the only caller (AxTlbBaseReference.SigningRequirementsMatchExistingWrapper) deals with COM wrappers which are managed assemblies. - Test Coverage — No tests exist for
GetAssemblyStrongNameLevel(pre-existing debt). The rewrite is a good opportunity to add coverage in a follow-up. - Correctness — Old code checked both
VirtualAddress == 0andSize == 0; new code only checksSize == 0. Equivalent for well-formed PEs.
Strengths:
- Eliminates 8 P/Invoke declarations to
kernel32.dllanddbghelp.dll— no longer Windows-only - Removes unsafe pointer arithmetic and
Marshal.PtrToStructure— reduced attack surface - Exception handling is more precise:
IOException/UnauthorizedAccessExceptionfor I/O failures,BadImageFormatExceptionfor corrupt PEs - Clean
usingdeclaration pattern with proper resource disposal - Removed
#endregion/#regionpair is correctly balanced in the resulting file
Note
🔒 Integrity filter blocked 1 item
The following item were blocked because they don't meet the GitHub integrity level.
- #13731
pull_request_read: has lower integrity than agent requires. The agent cannot read data with integrity below "approved".
To allow these resources, lower min-integrity in your GitHub frontmatter:
tools:
github:
min-integrity: approved # merged | approved | unapproved | noneGenerated by Expert Code Review (on open) for issue #13731 · ● 5.5M
…gnature VA - Return StrongNameLevel.Unknown when CorHeader is null (native / non-managed image), matching the previous behavior. The old code returned Unknown when ImageNtHeader produced no COR20 RVA; the PEReader rewrite was conflating that case with None. - Treat StrongNameSignatureDirectory as 'no signature' when either the RVA or the Size is zero, matching the previous (VA == 0 || Size == 0) check.
|
Addressed in 56ed289:
On tests: investigating using existing files on the system to get some coverage. |
Covers all five classification outcomes the refactor cares about: - FullySigned: BCL (mscorlib). - DelaySigned: emitted at test time via AssemblyBuilder with a real public key attached and [assembly: AssemblyDelaySign(true)], producing a managed PE with a populated StrongNameSignatureDirectory and CorFlags.StrongNameSigned cleared. - None: emitted at test time via AssemblyBuilder with no public key, producing a managed PE with an empty StrongNameSignatureDirectory. - Unknown: native PE (kernel32.dll, Windows only), non-existent file, empty file, garbage bytes, and null input. Gated on NETFRAMEWORK to match StrongNameUtils itself.
|
Failure was #13546 |
Co-authored-by: Jeremy Kuhne <jeremy.kuhne@microsoft.com>
rainersigwald
left a comment
rainersigwald
left a comment
There was a problem hiding this comment.
Love the simplification/deletion. I am slightly paranoid though--do you think there's any reason to keep the old code under a changewave to allow opting out of the new codepath, in case there's any observable difference?
Also, will this cause a new assembly load? That's caused us problems in VS before . . .
I don't think so. The PEReader package is heavily used so I'm pretty confident in it. https://www.nuget.org/packages/System.Reflection.Metadata
It is already a dependency for this assembly, and I see the PEReader assembly loaded by VS with a simple project when I attach. |
|
Error was #13732 |
Summary
GetAssemblyStrongNameLevelpreviously walked the PE image by hand viaCreateFile/CreateFileMapping/MapViewOfFile/ImageNtHeader/ImageRvaToVaplus a 32/64-bit dispatch overIMAGE_NT_HEADERS{32,64}/IMAGE_COR20_HEADERstructs. The whole approach exists only to readCorHeader.StrongNameSignatureDirectoryplusCorFlags.StrongNameSigned— whichSystem.Reflection.PortableExecutablealready gives us in ~20 lines.PEReaderis cross-platform and is already a dependency of Tasks. Switch to it and delete the now-unused native interop.Changes
src/Tasks/StrongNameUtils.cs: rewriteGetAssemblyStrongNameLevelusingPEReader/CorHeader/CorFlags. Cross-platform; nounsafeneeded.src/Tasks/NativeMethods.cs: drop now-unused interop[DllImport]:CreateFile,GetFileType,CloseHandle,CreateFileMapping,MapViewOfFile,UnmapViewOfFile,dbghelp.ImageNtHeader,dbghelp.ImageRvaToVaGENERIC_READ,PAGE_READONLY,FILE_MAP_READ,FILE_TYPE_DISK,IMAGE_NT_OPTIONAL_HDR32_MAGIC,IMAGE_NT_OPTIONAL_HDR64_MAGIC,IMAGE_DIRECTORY_ENTRY_COMHEADER,COMIMAGE_FLAGS_STRONGNAMESIGNED,InvalidIntPtrIMAGE_FILE_HEADER,IMAGE_DATA_DIRECTORY,IMAGE_OPTIONAL_HEADER32,IMAGE_OPTIONAL_HEADER64,IMAGE_NT_HEADERS32,IMAGE_NT_HEADERS64,IMAGE_COR20_HEADERStats
~260 lines of native interop removed. Net diff: +16 / -352.
Validation
MSBuild.Dev.slnf(net10 + net472): 0 warnings, 0 errorsMSBuild.SourceBuild.slnf/p:DotNetBuildSourceOnly=true: 0 warnings, 0 errorsThis unblocks one of the AnyCPU limitations called out in the CsWin32 migration thread: dbghelp
ImageNtHeadercannot be CsWin32-generated for AnyCPU because the metadata is architecture-specific. WithPEReaderwe no longer need it.