-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix runtime architecture detection logic in ANCM #63652
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
||
if (fIsCurrentProcess64Bit == IsX64(struDotnetSubstring.QueryStr())) | ||
ProcessorArchitecture dotnetArch = GetFileProcessorArchitecture(struDotnetSubstring.QueryStr()); | ||
if (dotnetArch == currentProcessArch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if both currentProcessArch
and dotnetArch
equal ProcessorArchitecture::Unknown
? I cant determine the behavior in such a case - maybe we should check it and at least log in such scenario?
if (dotnetArch == currentProcessArch)
{
if (dotnetArch == ProcessorArchitecture::Unknown)
{
LOG_INFOF(L"dotnet.exe and current process architectures are unknown. Behavior can be unexpected");
}
// continue
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetCurrentProcessArchitecture
will never be unknown. If we don't know it at build time there's a static assert.
{ | ||
LOG_INFOF(L"%ls is 64-bit", dotnetPath); | ||
return true; | ||
case IMAGE_FILE_MACHINE_I386: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if I understand docs correctly, then there are only 3 valid combinations --
if IMAGE_FILE_MACHINE_ARM64 then magic should be IMAGE_NT_OPTIONAL_HDR64_MAGIC
if IMAGE_NT_OPTIONAL_HDR64_MAGIC then magic should be IMAGE_NT_OPTIONAL_HDR64_MAGIC
if IMAGE_FILE_MACHINE_I386 then magic should be IMAGE_NT_OPTIONAL_HDR32_MAGIC
assuming that's true, would it be worth asserting magic in these various cases, to record our understanding? given the confusion that we've already had here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems correct to my understanding and clearer as well. The proof is in the testing :)
Testing on x64 machineWorks for in-process hosting for both x64 and x86 processes (with IIS configuration). logs for x64 process:
logs for x86 process:
Testing on arm64 machineARM64 process on ARM64 machine just works. For X64 process aspnetcorev2.dll fails before patching. It uses
After patching dll it skips ARM64 dotnet, and instead finds one via registry. Correct behavior.
Same goes for x86 - it was finding ARM64 dotnet before patching dll, but after it skips the
Testing on x86 machineThere is no pure-x86 azure machine, but we've seen x86 working on x64 machine, so should work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes runtime architecture detection logic in the ASP.NET Core Module (ANCM) to address a regression where the wrong binary architecture could be loaded. The change replaces the previous bitness-only detection with precise processor architecture matching to ensure compatibility between the current process and dotnet.exe binaries.
Key changes:
- Introduces a ProcessorArchitecture enum with specific architecture types (x86, AMD64, ARM64)
- Replaces binary bitness detection with precise architecture detection using PE header analysis
- Updates the dotnet.exe selection logic to match exact architectures rather than just 32/64-bit compatibility
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
ProcessorArchitecture.h | New header defining ProcessorArchitecture enum and string conversion utility |
HostFxrResolver.h | Updates method signature from IsX64 to GetFileProcessorArchitecture |
HostFxrResolver.cpp | Replaces bitness checking with architecture-specific detection and matching logic |
Environment.h | Adds GetCurrentProcessArchitecture method declaration |
Environment.cpp | Implements compile-time architecture detection for current process |
#elif defined(_M_IX86) | ||
return ProcessorArchitecture::x86; | ||
#else | ||
static_assert(false, "Unknown target architecture"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace static_assert(false, ...)
with static_assert(false, "Unknown target architecture")
or use a dependent false expression like static_assert(sizeof(void*) == 0, "Unknown target architecture")
to avoid potential compilation issues with newer compilers that may reject unconditional static_assert(false).
static_assert(false, "Unknown target architecture"); | |
static_assert(sizeof(void*) == 0, "Unknown target architecture"); |
Copilot uses AI. Check for mistakes.
/backport to release/9.0 |
/backport to release/8.0 |
Started backporting to release/9.0: https://github.com/dotnet/aspnetcore/actions/runs/17803075915 |
Started backporting to release/8.0: https://github.com/dotnet/aspnetcore/actions/runs/17803080862 |
@adityamandaleeka backporting to "release/9.0" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix runtime architecture detection logic in ANCM.
Using index info to reconstruct a base tree...
M src/Servers/IIS/AspNetCoreModuleV2/CommonLib/Environment.cpp
M src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/Servers/IIS/AspNetCoreModuleV2/CommonLib/Environment.cpp
Auto-merging src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp
CONFLICT (content): Merge conflict in src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix runtime architecture detection logic in ANCM.
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
@adityamandaleeka backporting to "release/8.0" failed, the patch most likely resulted in conflicts: $ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix runtime architecture detection logic in ANCM.
Using index info to reconstruct a base tree...
M src/Servers/IIS/AspNetCoreModuleV2/CommonLib/Environment.cpp
M src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp
Falling back to patching base and 3-way merge...
Auto-merging src/Servers/IIS/AspNetCoreModuleV2/CommonLib/Environment.cpp
Auto-merging src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp
CONFLICT (content): Merge conflict in src/Servers/IIS/AspNetCoreModuleV2/CommonLib/HostFxrResolver.cpp
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0001 Fix runtime architecture detection logic in ANCM.
Error: The process '/usr/bin/git' failed with exit code 128 Please backport manually! |
I'll backport manually; merging to main for now |
Thanks a bunch @DeagleGross for picking this up. |
/backport to release/10.0 |
Started backporting to release/10.0: https://github.com/dotnet/aspnetcore/actions/runs/17923852252 |
Fixes #63650
A recent change (#61894) inadvertently regressed a scenario that was working by accident.
That change switched us away from using
GetBinaryTypeW
to determine the architecture of a binary, because that method actually loads the binary into the process and we wanted to avoid that. However, what was missed there is that in some cases (e.g. x64 process running on arm64) the function fails entirely (since it can't even load the binary), which used to lead us to skip that binary when doing our probing and instead rely on fallback methods.After #61894, we started to correctly determine that both x64 and arm64 binaries are 64-bit, but since we weren't distinguishing between the two, we could end up attempting to load the wrong one.
This fixes that by actually determining the specific architecture and ensuring a match. There's some nearby code that needs to be rethought as well, but I'm trying to keep this change minimal so we can minimize risk late in .NET 10.