Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

Closed
wants to merge 5 commits into from
Closed

Fix WinMD handling in System.Reflection.MetadataLoadContext #58344

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d0806d6
Fix WinMD handling in System.Reflection.MetadataLoadContext
andrew-boyarshin Aug 30, 2021
114f43d
Apply review suggestion
andrew-boyarshin Aug 30, 2021
66463ea
Skip the new test on Browser platform
andrew-boyarshin Aug 30, 2021
ccb94df
Address review feedback
andrew-boyarshin Oct 9, 2021
7698e90
Bring back the test skip for Browser
andrew-boyarshin Oct 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ...dataLoadContext/src/System/Reflection/TypeLoading/Assemblies/Ecma/EcmaAssembly.Modules.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics.CodeAnalysis;
using System.IO;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Reflection.PortableExecutable;

namespace System.Reflection.TypeLoading.Ecma
Expand Down Expand Up @@ -81,7 +82,7 @@ protected sealed override IEnumerable<AssemblyFileInfo> GetAssemblyFileInfosFrom
AssemblyFile af = h.GetAssemblyFile(reader);
if (includeResourceModules || af.ContainsMetadata)
{
yield return new AssemblyFileInfo(af.Name.GetString(reader), af.ContainsMetadata, h.GetToken().GetTokenRowNumber());
yield return new AssemblyFileInfo(af.Name.GetString(reader), af.ContainsMetadata, reader.GetRowNumber(h));
}
}
}
Expand Down
9 changes: 4 additions & 5 deletions ...ction.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Ecma/MetadataTable.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Collections.Generic;
using System.Diagnostics;
using System.Reflection.Metadata;
using System.Reflection.Metadata.Ecma335;
using System.Threading;

namespace System.Reflection.TypeLoading.Ecma
Expand All @@ -13,10 +14,8 @@ namespace System.Reflection.TypeLoading.Ecma
///
/// The key type is hard-coded to EntityHandle.
/// The "T" type is the value type (e.g. RoTypeDefinition objects)
/// The "C" type is an optional context value passed through the factory methods (so we don't to allocate a closure each time.)
/// </summary>
internal sealed class MetadataTable<T, C>
steveharter marked this conversation as resolved.
Show resolved Hide resolved
where T : class
internal sealed class MetadataTable<T> where T : class
{
private readonly T?[] _table;

Expand All @@ -26,12 +25,12 @@ public MetadataTable(int count)
_table = new T?[count];
}

public T GetOrAdd(EntityHandle handle, C context, Func<EntityHandle, C, T> factory)
public T GetOrAdd(EntityHandle handle, EcmaModule context, Func<EntityHandle, EcmaModule, T> factory)
{
Debug.Assert(!handle.IsNil);
Debug.Assert(factory != null);

int index = handle.GetToken().GetTokenRowNumber() - 1;
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary (was there an issue converting from token to row number) or just simplification?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's at least half of the fix. System.Reflection handles WinMDs fine, because it knows about virtual AssemblyRefs. System.Reflection.MetadataLoadContext didn't. I dropped the wrong/incomplete GetTokenRowNumber impl from System.Reflection.MetadataLoadContext, and used the correct System.Reflection one.

Copy link
Member

Choose a reason for hiding this comment

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

System.Reflection handles WinMDs fine, because it knows about virtual AssemblyRefs

I assume you mean System.Reflection.Metadata, not System.Reflection via the reader option MetadataReaderOptions.ApplyWindowsRuntimeProjections and the resulting MetadataKind.WindowsMetadata enum value.

int index = context.Reader.GetRowNumber(handle) - 1;
T?[] table = _table;
T? result = Volatile.Read(ref table[index]);
if (result != null)
Expand Down
2 changes: 0 additions & 2 deletions ...ystem.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ public static ReadOnlyCollection<T> ToReadOnlyCollection<T>(this IEnumerable<T>
return new ReadOnlyCollection<T>(list.ToArray());
}

public static int GetTokenRowNumber(this int token) => token & 0x00ffffff;

public static RoMethod? FilterInheritedAccessor(this RoMethod accessor)
{
if (accessor.ReflectedType == accessor.DeclaringType)
Expand Down
27 changes: 17 additions & 10 deletions ...taLoadContext/src/System/Reflection/TypeLoading/Modules/Ecma/EcmaModule.MetadataTables.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ namespace System.Reflection.TypeLoading.Ecma
/// </summary>
internal sealed partial class EcmaModule
{
internal MetadataTable<EcmaDefinitionType, EcmaModule> TypeDefTable
internal MetadataTable<EcmaDefinitionType> TypeDefTable
{
get
{
Expand All @@ -21,7 +21,7 @@ internal MetadataTable<EcmaDefinitionType, EcmaModule> TypeDefTable
_lazyTypeDefTable;
}
}
private volatile MetadataTable<EcmaDefinitionType, EcmaModule>? _lazyTypeDefTable;
private volatile MetadataTable<EcmaDefinitionType>? _lazyTypeDefTable;

private void EnsureTypeDefTableFullyFilled()
{
Expand All @@ -36,7 +36,7 @@ private void EnsureTypeDefTableFullyFilled()
}
private bool _typeDefTableFullyFilled; // Only gets set true if EnsureTypeDefTableFullyFilled() fills the table. False negative just means some unnecessary work is done.

internal MetadataTable<RoDefinitionType, EcmaModule> TypeRefTable
internal MetadataTable<RoDefinitionType> TypeRefTable
{
get
{
Expand All @@ -45,9 +45,9 @@ internal MetadataTable<RoDefinitionType, EcmaModule> TypeRefTable
_lazyTypeRefTable;
}
}
private volatile MetadataTable<RoDefinitionType, EcmaModule>? _lazyTypeRefTable;
private volatile MetadataTable<RoDefinitionType>? _lazyTypeRefTable;

internal MetadataTable<EcmaGenericParameterType, EcmaModule> GenericParamTable
internal MetadataTable<EcmaGenericParameterType> GenericParamTable
{
get
{
Expand All @@ -56,9 +56,9 @@ internal MetadataTable<EcmaGenericParameterType, EcmaModule> GenericParamTable
_lazyGenericParamTable;
}
}
private volatile MetadataTable<EcmaGenericParameterType, EcmaModule>? _lazyGenericParamTable;
private volatile MetadataTable<EcmaGenericParameterType>? _lazyGenericParamTable;

internal MetadataTable<RoAssembly, EcmaModule> AssemblyRefTable
internal MetadataTable<RoAssembly> AssemblyRefTable
{
get
{
Expand All @@ -67,11 +67,18 @@ internal MetadataTable<RoAssembly, EcmaModule> AssemblyRefTable
_lazyAssemblyRefTable;
}
}
private volatile MetadataTable<RoAssembly, EcmaModule>? _lazyAssemblyRefTable;
private volatile MetadataTable<RoAssembly>? _lazyAssemblyRefTable;

private MetadataTable<T, EcmaModule> CreateTable<T>(TableIndex tableIndex) where T : class
private MetadataTable<T> CreateTable<T>(TableIndex tableIndex) where T : class
{
return new MetadataTable<T, EcmaModule>(Reader.GetTableRowCount(tableIndex));
int rowCount = tableIndex switch
Copy link
Member

Choose a reason for hiding this comment

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

style\nit: for a simple two-case switch I think an if statement is more appropriate.

{
// Windows Metadata assemblies contain additional "virtual" AssemblyRefs we need to account for.
// This is the simplest way to get the total AssemblyRefs count:
TableIndex.AssemblyRef => Reader.AssemblyReferences.Count,
_ => Reader.GetTableRowCount(tableIndex)
};
return new MetadataTable<T>(rowCount);
}
}
}
2 changes: 2 additions & 0 deletions ...ibraries/System.Reflection.MetadataLoadContext/tests/src/SampleMetadata/SampleMetadata.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,4 +548,6 @@ public class ClassWithDefaultMember1<T> where T : ClassWithDefaultMember1<T>
{
public int Yes;
}

public delegate void SampleCompletedHandler();
}
58 changes: 58 additions & 0 deletions src/libraries/System.Reflection.MetadataLoadContext/tests/src/TestUtils/TestData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3074,5 +3074,63 @@ internal static class TestData
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAA");

Copy link
Member

Choose a reason for hiding this comment

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

Was there is an attempt in tests to use an actual .winmd file, as in the original issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a reduced copy that still exhibits the issue. The only snag was having to use ILASM from .NET Framework instead of .NET one, which silently generated .winmd with wrong metadata version, which is a crucial thing for reproducing the issue. The original .winmd files (Windows SDK UnionMetadata one, and the ones from Windows installation folder) are too big to put in a test.

// // Metadata version: WindowsRuntime 1.4
// .assembly extern mscorlib
// {
// .publickeytoken = (B7 7A 5C 56 19 34 E0 89 ) // .z\V.4..
// .ver 255:255:255:255
// }
// .assembly windowsruntime Foundation
// {
// .hash algorithm 0x00008004
// .ver 255:255:255:255
// }
// .module Foundation.winmd
// .imagebase 0x00400000
// .file alignment 0x00000200
// .stackreserve 0x00100000
// .subsystem 0x0003 // WINDOWS_CUI
// .corflags 0x00000001 // ILONLY
//
// .class public auto ansi windowsruntime sealed Foundation.CompletedHandler
// extends [mscorlib]System.MulticastDelegate
// {
// .method private hidebysig specialname rtspecialname
// instance void .ctor(object 'object', native int 'method') runtime managed
// {
// } // end of method CompletedHandler::.ctor
//
// .method public hidebysig newslot specialname virtual
// instance void Invoke() runtime managed
// {
// } // end of method CompletedHandler::Invoke
// } // end of class Foundation.CompletedHandler
public static readonly string s_MetadataDelegateAssemblyName = "Foundation, Version=255.255.255.255, Culture=neutral, PublicKeyToken=null, ContentType=WindowsRuntime";
public static readonly byte[] s_MetadataDelegateImage = Convert.FromBase64String(
"TVqQAAMAAAAEAAAA//8AALgAAAAAAAAAQAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAA4fug4AtAnNIbgBTM0hVGhpcyBwcm9ncmFt" +
"IGNhbm5vdCBiZSBydW4gaW4gRE9TIG1vZGUuDQ0KJAAAAAAAAABQRQAATAECAHiMLGEAAAAAAAAAAOAAAiELAQsAAAQAAAACAAAAAAAAbiIAAAAgAAAAQAAA" +
"AABAAAAgAAAAAgAABAAAAAAAAAAEAAAAAAAAAABgAAAAAgAAAAAAAAMAQIUAABAAABAAAAAAEAAAEAAAAAAAABAAAAAAAAAAAAAAABgiAABTAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAEAAAAwAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAIAAACAAAAAAAAAAAAAAA" +
"CCAAAEgAAAAAAAAAAAAAAC50ZXh0AAAAdAIAAAAgAAAABAAAAAIAAAAAAAAAAAAAAAAAACAAAGAucmVsb2MAAAwAAAAAQAAAAAIAAAAGAAAAAAAAAAAAAAAA" +
"AABAAABCAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAABQIgAAAAAAAEgAAAACAAUAUCAAAMgBAAABAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAEJTSkIBAAEAAAAAABQAAABXaW5kb3dzUnVudGltZSAxLjQAAAAA" +
"BQB0AAAAtAAAACN+AAAoAQAAdAAAACNTdHJpbmdzAAAAAJwBAAAIAAAAI1VTAKQBAAAQAAAAI0dVSUQAAAC0AQAAFAAAACNCbG9iAAAAAAAAAAIAAAFHAQAA" +
"CQAAAAD6JTMAFgAAAQAAAAEAAAACAAAAAgAAAAIAAAABAAAAAQAAAAAAIwABAAAAAAAGABEACgAAAAAAAQAAAAAAAQABAAFBAAA9AE4ABQABAAEAAAAAAAMA" +
"gRhZAAoAAQAAAAAAAwDGCW0AEAADAAAAAQBfAAAAAgBmAASAAAD/AP8A/wD/AAACAAAAAE4AAAD/AP8A/wD/AAAAAAABADQAAAAAAAAAADxNb2R1bGU+AFN5" +
"c3RlbQBNdWx0aWNhc3REZWxlZ2F0ZQBGb3VuZGF0aW9uLndpbm1kAG1zY29ybGliAENvbXBsZXRlZEhhbmRsZXIARm91bmRhdGlvbgAuY3RvcgBvYmplY3QA" +
"bWV0aG9kAEludm9rZQAAAyAAAAAAAP9GtVt8T4ROg3HdL1tpj4gACLd6XFYZNOCJBSACARwYAyAAAUAiAAAAAAAAAAAAAF4iAAAAIAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAABQIgAAAAAAAAAAAAAAAAAAAABfQ29yRGxsTWFpbgBtc2NvcmVlLmRsbAAAAAAA/yUAIEAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAACAAAAwAAABwMgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA" +
"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA="
);
}
}
31 changes: 31 additions & 0 deletions src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using SampleMetadata;
using System.Collections;
using System.Collections.Generic;
using System.IO;
using System.Linq;
using System.Runtime.InteropServices;
using Xunit;
Expand All @@ -28,6 +29,7 @@ private static IEnumerable<Type> InvariantTestData
{
yield return typeof(object).Project();
yield return typeof(Span<>).Project();
yield return typeof(SampleCompletedHandler).Project();
#if false
foreach (Type t in typeof(TopLevelType).Project().Assembly.GetTypes())
{
Expand Down Expand Up @@ -383,6 +385,8 @@ public static void TestIsPrimitive()
Assert.False(typeof(BindingFlags).Project().IsPrimitive);
Assert.False(typeof(int[]).Project().IsPrimitive);

Assert.False(typeof(SampleCompletedHandler).Project().IsPrimitive);

return;
}

Expand All @@ -403,6 +407,7 @@ public static void TestIsValueType()
Assert.False(typeof(ValueType).Project().IsValueType);
Assert.False(typeof(Enum).Project().IsValueType);
Assert.True(typeof(MyColor).Project().IsValueType);
Assert.False(typeof(SampleCompletedHandler).Project().IsValueType);

return;
}
Expand Down Expand Up @@ -611,5 +616,31 @@ public static void TypesWithStrangeCharacters()
Assert.Equal(t, tRetrieved);
}
}

[Fact]
[SkipOnPlatform(TestPlatforms.Browser, "BCL assemblies enumeration and loading is not supported on Browser.")]
public static void WindowsRuntimeMetadataDelegateType()
{
// Get the array of runtime assemblies.
// This will allow us to at least inspect types depending only on BCL.
string[] runtimeAssemblies = Directory.GetFiles(RuntimeEnvironment.GetRuntimeDirectory(), "*.dll");
buyaa-n marked this conversation as resolved.
Show resolved Hide resolved

// Create MetadataLoadContext that can resolve assemblies using the created array.
PathAssemblyResolver resolver = new(runtimeAssemblies);
using MetadataLoadContext mlc = new(resolver);

using MemoryStream peStream = new(TestData.s_MetadataDelegateImage);
Assembly a = mlc.LoadFromStream(peStream);
Assert.NotNull(a);
Assert.Equal(string.Empty, a.Location);
Assert.Equal("WindowsRuntime 1.4", a.ImageRuntimeVersion);

string fullName = a.GetName().FullName;
Assert.Equal(TestData.s_MetadataDelegateAssemblyName, fullName);

Type[] types = a.GetTypes();
Assert.Single(types);
Assert.False(types[0].IsValueType);
}
}
}