Throw BadImageFormatException when missing PE metadata

[FiniteReality]

Fixes #6200

Context

InvalidOperationException was being thrown by AssemblyInformation.CorePopulateMetadata, causing the ResolveAssemblyReference task to fail rather than disregarding the file.

Changes Made

I copied the code from AssemblyNameExtension to ensure there is metadata present before getting a metadata reader:

msbuild/src/Shared/AssemblyNameExtension.cs

Lines 214 to 231 in 6819f7a

	bool hasMetadata = false;
	try
	{
	// This can throw if the stream is too small, which means
	// the assembly doesn't have metadata.
	hasMetadata = peFile.HasMetadata;
	}
	finally
	{
	// If the file does not contain PE metadata, throw BadImageFormatException to preserve
	// behavior from AssemblyName.GetAssemblyName(). RAR will deal with this correctly.
	if (!hasMetadata)
	{
	throw new BadImageFormatException(string.Format(CultureInfo.CurrentCulture,
	AssemblyResources.GetString("ResolveAssemblyReference.AssemblyDoesNotContainPEMetadata"),
	path));
	}
	}

Testing

I looked for unit tests for AssemblyInformation, but could not find any, so I didn't add a unit test for this case. However, I successfully built the project that was reproducing the error in #6200, indicating that the change did indeed fix the issue.

[benvillalobos]

If we're checking this here, we should add the same check in MetadataReader.cs. This is what it looks like currently, which could also throw.

                    if (_peReader != null)
                    {
                        if (_peReader.HasMetadata)
                        {
                            _reader = _peReader.GetMetadataReader();
                        }
                    }

[FiniteReality]

we should add the same check in MetadataReader.cs.

It seems MetadataReader already has a try/catch surrounding the HasMetadata check, which closes the reader, meaning MetadataReader.Create() returns null as _reader is set to null.

[ladipro]

This looks good although I wonder if pulling the code into a helper and calling it from both AssemblyNameExtension and AssemblyInformation wouldn't be slightly more elegant.

[FiniteReality]

This looks good although I wonder if pulling the code into a helper and calling it from both AssemblyNameExtension and AssemblyInformation wouldn't be slightly more elegant.

So maybe an extension method PEReader.ThrowIfMissingMetadata()?

[cdmihai]

I think the "prescribed / designed" way of referencing non-CLR projects is telling the sdk to not treat the ProjectReference output as a managed assembly. But I'm not 100% sure, I couldn't find any docs on the expected way to do this (@KathleenDollard @dsplaisted). Might be safer to go down that route than to allow native assemblies to flow into RAR which might have unknown, cascading consequences for the downstream code that reads RAR outputs.

[ladipro]

How does RAR behave if you pass a non-PE file?

[FiniteReality]

How does RAR behave if you pass a non-PE file?

Generally, a BadImageFormatException is thrown, which RAR catches and aborts with "File does not contain managed metadata". For example:

         Primary reference "HelloWorldLibraryC".
             Could not find dependent files. Assembly file '/home/dev/Development/Finite.Cpp.Sdk/artifacts/bin/samples/HelloWorldLibraryC/Debug/HelloWorldLibraryC.so' could not be opened -- PE image doesn't contain managed metadata.
             Could not find dependent files. Image is too small.
             Resolved file path is "/home/dev/Development/Finite.Cpp.Sdk/artifacts/bin/samples/HelloWorldLibraryC/Debug/HelloWorldLibraryC.so".
             Reference found at search path location "".
             The ImageRuntimeVersion for this reference is "".

[KirillOsenkov]

Not sure if relevant, but Microsoft.Build.Tasks.Core.dll has an IsManagedAssembly() method:
https://source.dot.net/#Microsoft.Build.Tasks.Core/ManifestUtil/PathUtil.cs,ef4ea3875d69614b

Also here's a way to find out if an assembly is managed without throwing:
https://github.com/KirillOsenkov/MetadataTools/blob/main/src/PEFile/IsManagedAssembly.cs

[KirillOsenkov]

@cdmihai the project reference stuff is certainly valid, but I'm questioning whether that should be done as a separate PR? This feels nicely self-contained and relatively isolated and it does fix the issue at hand?

[Forgind]

Thanks @FiniteReality!