Skip to content

Conversation

@VSadov
Copy link
Member

@VSadov VSadov commented Jan 12, 2023

Fixes: #77697

Computing and setting ProcessorArchitecture was unintentionally omitted when GetAssemblyName(path) was switched to a managed implementation.

This will need to be backported to 7.0

@ghost
Copy link

ghost commented Jan 12, 2023

Tagging subscribers to this area: @dotnet/area-system-reflection-metadata
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes: #77697

ProcessorArchitecture was unintentionally omitted when GetAssemblyName(path) was switched to a managed implementation.

Author: VSadov
Assignees: -
Labels:

area-System.Reflection.Metadata

Milestone: -

@VSadov VSadov marked this pull request as ready for review January 12, 2023 23:19
@VSadov VSadov requested a review from jkotas January 12, 2023 23:20

private static ProcessorArchitecture CalculateProcArchIndex(PortableExecutableKinds pek, ImageFileMachine ifm, AssemblyFlags flags)
{
if (((uint)flags & 0xF0) == 0x70)
Copy link
Member

Choose a reason for hiding this comment

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

Any idea what this is trying to check for?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is one place that calls 0xF0 is ProcArchMask

// ProcArchMask = 0x00F0, // Bits describing the processor architecture

I suspect 0x70 means None, but that is just a guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Old NDP sources have this:

            // 0x70 specifies "reference assembly", a new concept in v4.0. For these, CLR wants to return None as arch so they
            // can be always loaded, regardless of process type. 

Copy link
Member Author

Choose a reason for hiding this comment

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

I've checked a few reference assemblies. The COFF header specifies machine as I386 and CorFlags are ILOnly, so this does look like needs to be handled in a special way.
Otherwise the suggested simplified way will return MSIL while expected to return None

@jkotas
Copy link
Member

jkotas commented Jan 13, 2023

I think that this logic is very convoluted. I would simplify into something that one can reason about. For example:

assemblyName.ProcessorArchitecture = peHeaders.CoffHeader.Machine switch
{
    ImageFileMachine.I386 => IsPlatformNeutral(peHeaders.CorHeader!.Flags) ? ProcessorArchitecture.MSIL : ProcessorArchitecture.X86;
    ImageFileMachine.AMD64 => ProcessorArchitecture.Amd64,
    ImageFileMachine.ARM => ProcessorArchitecture.Arm,    
    ImageFileMachine.IA64 => ProcessorArchitecture.IA64,
    _ => ProcessorArchitecture.None
}

static bool IsPlatformNeutral(CorFlags flags) =>
   ((flags & CorFlags.ILOnly) != 0) && ((flags & (CorFlags.Requires32Bit | CorFlags.Prefers32Bit) != CorFlags.Requires32Bit);

It will behave differently from the current convoluted logic for invalid images (ie images that would not execute anyway - like x86 image with PE32+ header, etc.). I think that it is ok.

@jkotas
Copy link
Member

jkotas commented Jan 13, 2023

We can potentially apply the same simplification in the CoreCLR copy as well.

@VSadov
Copy link
Member Author

VSadov commented Jan 13, 2023

We can potentially apply the same simplification in the CoreCLR copy as well.

Yes. I think we might be ok with duplication, since this is legacy and will not change often, if at all, and only needed in two places. But if implementations do not match, it could be confusing why the differences.

Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM!

@VSadov
Copy link
Member Author

VSadov commented Jan 18, 2023

Thanks!!

@VSadov
Copy link
Member Author

VSadov commented Jan 24, 2023

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3998851487

@ghost ghost locked as resolved and limited conversation to collaborators Feb 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AssemblyName.ProcessorArchitecture no longer returns the correct result

2 participants