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 support for multiple internal metadata view interfaces defined from multiple assemblies #93

Merged
merged 4 commits into from
Jul 2, 2018
Merged

Fix support for multiple internal metadata view interfaces defined from multiple assemblies #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
68cb6a0
Add test for implementing internal metadata view interfaces from two …
AArnott May 5, 2018
8c3054a
Generate each metadata view proxy into its own dynamic assembly
AArnott May 7, 2018
5e8bc49
Reduce # of dynamic assemblies to just the minimal set required
AArnott May 8, 2018
b970746
Update test name and add comment
AArnott May 8, 2018
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
61 changes: 48 additions & 13 deletions src/Microsoft.VisualStudio.Composition/Configuration/MetadataViewGenerator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Microsoft.VisualStudio.Composition
{
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Globalization;
using System.Linq;
using System.Reflection;
Expand Down Expand Up @@ -79,36 +80,48 @@ internal static class MetadataViewGenerator
private const string MetadataViewFactoryName = "Create";

private static readonly Dictionary<Type, MetadataViewFactory> MetadataViewFactories = new Dictionary<Type, MetadataViewFactory>();
private static readonly AssemblyName ProxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "MetadataViewProxies_{0}", Guid.NewGuid()));

private static readonly Type[] CtorArgumentTypes = new Type[] { typeof(IReadOnlyDictionary<string, object>), typeof(IReadOnlyDictionary<string, object>) };
private static readonly MethodInfo MdvDictionaryTryGet = CtorArgumentTypes[0].GetTypeInfo().GetMethod("TryGetValue");
private static readonly MethodInfo MdvDictionaryIndexer = CtorArgumentTypes[0].GetTypeInfo().GetMethod("get_Item");
private static readonly MethodInfo ObjectGetType = typeof(object).GetTypeInfo().GetMethod("GetType", Type.EmptyTypes);
private static readonly ConstructorInfo ObjectCtor = typeof(object).GetTypeInfo().GetConstructor(Type.EmptyTypes);

private static ModuleBuilder transparentProxyModuleBuilder;
private static SkipClrVisibilityChecks skipClrVisibilityChecks;
private static readonly Dictionary<ImmutableHashSet<AssemblyName>, ModuleBuilder> TransparentProxyModuleBuilderByVisibilityCheck = new Dictionary<ImmutableHashSet<AssemblyName>, ModuleBuilder>(new ByContentEqualityComparer());

public delegate object MetadataViewFactory(IReadOnlyDictionary<string, object> metadata, IReadOnlyDictionary<string, object> defaultMetadata);

private static AssemblyBuilder CreateProxyAssemblyBuilder(ConstructorInfo constructorInfo)
{
return AssemblyBuilder.DefineDynamicAssembly(ProxyAssemblyName, AssemblyBuilderAccess.Run);
var proxyAssemblyName = new AssemblyName(string.Format(CultureInfo.InvariantCulture, "MetadataViewProxies_{0}", Guid.NewGuid()));
return AssemblyBuilder.DefineDynamicAssembly(proxyAssemblyName, AssemblyBuilderAccess.Run);
}

private static ModuleBuilder GetProxyModuleBuilder()
/// <summary>
/// Gets the <see cref="ModuleBuilder"/> to use for generating a proxy for the given type.
/// </summary>
/// <param name="viewType">The type of the interface to generate a proxy for.</param>
/// <returns>The <see cref="ModuleBuilder"/> to use.</returns>
private static ModuleBuilder GetProxyModuleBuilder(TypeInfo viewType)
{
Requires.NotNull(viewType, nameof(viewType));
Assumes.True(Monitor.IsEntered(MetadataViewFactories));
if (transparentProxyModuleBuilder == null)

// Dynamic assemblies are relatively expensive. We want to create as few as possible.
// For each unique set of skip visibility check assemblies, we need a new dynamic assembly
// because the CLR will not honor any additions to that set once the first generated type is closed.
// So maintain a dictionary to point at dynamic modules based on the set of skip visiblity check assemblies they were generated with.
var skipVisibilityCheckAssemblies = SkipClrVisibilityChecks.GetSkipVisibilityChecksRequirements(viewType);
if (!TransparentProxyModuleBuilderByVisibilityCheck.TryGetValue(skipVisibilityCheckAssemblies, out ModuleBuilder moduleBuilder))
Copy link

Choose a reason for hiding this comment

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

If the type is public, do you still need to do this? (i.e when skipVisibilityCheckAssemblies is ImmutableHashSet<AssemblyName>.Empty

Copy link
Member Author

Choose a reason for hiding this comment

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

When the set is empty, we still need a dynamic assembly to generate into. Our lookup dictionary needs an entry for the empty set as well so we don't end up special casing empty sets -- we execute the same code regardless of the size of the set.

{
// make a new assemblybuilder and modulebuilder
var assemblyBuilder = CreateProxyAssemblyBuilder(typeof(SecurityTransparentAttribute).GetTypeInfo().GetConstructor(Type.EmptyTypes));
transparentProxyModuleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule");
skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, transparentProxyModuleBuilder);
moduleBuilder = assemblyBuilder.DefineDynamicModule("MetadataViewProxiesModule");
var skipClrVisibilityChecks = new SkipClrVisibilityChecks(assemblyBuilder, moduleBuilder);
skipClrVisibilityChecks.SkipVisibilityChecksFor(skipVisibilityCheckAssemblies);
TransparentProxyModuleBuilderByVisibilityCheck.Add(skipVisibilityCheckAssemblies, moduleBuilder);
}

return transparentProxyModuleBuilder;
return moduleBuilder;
}

public static MetadataViewFactory GetMetadataViewFactory(Type viewType)
Expand Down Expand Up @@ -141,9 +154,7 @@ private static TypeInfo GenerateInterfaceViewProxyType(Type viewType)
TypeBuilder proxyTypeBuilder;
Type[] interfaces = { viewType };

var proxyModuleBuilder = GetProxyModuleBuilder();
skipClrVisibilityChecks.SkipVisibilityChecksFor(viewType.GetTypeInfo());

var proxyModuleBuilder = GetProxyModuleBuilder(viewType.GetTypeInfo());
proxyTypeBuilder = proxyModuleBuilder.DefineType(
string.Format(CultureInfo.InvariantCulture, "_proxy_{0}_{1}", viewType.FullName, Guid.NewGuid()),
TypeAttributes.Public,
Expand Down Expand Up @@ -269,5 +280,29 @@ private static IEnumerable<PropertyInfo> GetAllProperties(this Type type)
{
return type.GetTypeInfo().GetInterfaces().Concat(new Type[] { type }).SelectMany(itf => itf.GetTypeInfo().GetProperties());
}

private class ByContentEqualityComparer : IEqualityComparer<ImmutableHashSet<AssemblyName>>
{
public bool Equals(ImmutableHashSet<AssemblyName> x, ImmutableHashSet<AssemblyName> y)
{
if (x.Count != y.Count)
{
return false;
}

return !x.Except(y).Any();
}

public int GetHashCode(ImmutableHashSet<AssemblyName> obj)
{
int hashCode = 0;
foreach (var item in obj)
{
hashCode += item.GetHashCode();
}

return hashCode;
}
}
}
}
60 changes: 46 additions & 14 deletions src/Microsoft.VisualStudio.Composition/Reflection/SkipClrVisibilityChecks.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ namespace Microsoft.VisualStudio.Composition.Reflection
{
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Reflection;
using System.Reflection.Emit;
Expand Down Expand Up @@ -64,33 +65,64 @@ internal SkipClrVisibilityChecks(AssemblyBuilder assemblyBuilder, ModuleBuilder
}

/// <summary>
/// Ensures the CLR will skip visibility checks when accessing
/// the assembly that contains the specified member.
/// Gets the set of assemblies that a generated assembly must be granted the ability to skip visiblity checks for
/// in order to access the specified type.
/// </summary>
/// <param name="memberInfo">The member that may not be publicly accessible.</param>
internal void SkipVisibilityChecksFor(MemberInfo memberInfo)
/// <param name="typeInfo">The type which may be internal.</param>
/// <returns>The set of names of assemblies to skip visibility checks for.</returns>
internal static ImmutableHashSet<AssemblyName> GetSkipVisibilityChecksRequirements(TypeInfo typeInfo)
{
this.SkipVisibilityChecksFor(memberInfo.Module.Assembly);
Requires.NotNull(typeInfo, nameof(typeInfo));

if (ReflectionHelpers.IsPublic(typeInfo.AsType()))
{
return ImmutableHashSet<AssemblyName>.Empty;
}
else
{
var result = ImmutableHashSet<AssemblyName>.Empty
.Add(typeInfo.Assembly.GetName());

// If this type has a base type defined in another assembly that is also internal
Copy link

Choose a reason for hiding this comment

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

Did you mean to leave the commented code here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, since it really feels like it should be here, and I want it to remind me to ask the CLR folks why I don't need visibility into transitive assemblies when I only ask to skip visibility checks for the first assembly.

Copy link
Member

Choose a reason for hiding this comment

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

Because what's happening there is that access checks are only done on the metadata as written. For instance, A derives from B derives from C. As written, A only knows about B, so the access check from A is only to see if it can access B. For access to C, the only access check is that B is allowed to access C. These rules are straightforward as long as no techniques are used to skirt around access checks, but when you do, you can see surprises like this.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, thanks. If A defines a member that my proxy implements, evidently that doesn't require C's visibility into A. I guess if A defines a member with a Type that is internal to A, at that point C couldn't implement it without an explicit skip visibility check for A. Yes?

// (with InternalsVisibleTo to the assembly hosting the original type)
// then we'll need to add that.
// TODO: Learn we somehow we don't need to do this, even given our test defines an internal interface
// that derives from another internal interface defined in another assembly.
////if (typeInfo.BaseType != null)
////{
//// result = result.Union(GetSkipVisibilityChecksRequirements(typeInfo.BaseType.GetTypeInfo()));
////}

////foreach (var iface in typeInfo.GetInterfaces())
////{
//// result = result.Union(GetSkipVisibilityChecksRequirements(iface.GetTypeInfo()));
////}

return result;
}
}

/// <summary>
/// Add an attribute to the dynamic assembly so that the CLR will skip visibility checks
/// for the specified assembly.
/// Add attributes to a dynamic assembly so that the CLR will skip visibility checks
/// for the assemblies with the specified names.
/// </summary>
/// <param name="assembly">The assembly to skip visibility checks for.</param>
private void SkipVisibilityChecksFor(Assembly assembly)
/// <param name="assemblyNames">The names of the assemblies to skip visibility checks for.</param>
internal void SkipVisibilityChecksFor(IEnumerable<AssemblyName> assemblyNames)
{
Requires.NotNull(assembly, nameof(assembly));
var assemblyName = assembly.GetName();
this.SkipVisibilityChecksFor(assemblyName);
Requires.NotNull(assemblyNames, nameof(assemblyNames));

foreach (var assemblyName in assemblyNames)
{
this.SkipVisibilityChecksFor(assemblyName);
}
}

/// <summary>
/// Add an attribute to the dynamic assembly so that the CLR will skip visibility checks
/// Add an attribute to a dynamic assembly so that the CLR will skip visibility checks
/// for the assembly with the specified name.
/// </summary>
/// <param name="assemblyName">The name of the assembly to skip visibility checks for.</param>
private void SkipVisibilityChecksFor(AssemblyName assemblyName)
internal void SkipVisibilityChecksFor(AssemblyName assemblyName)
{
Requires.NotNull(assemblyName, nameof(assemblyName));

Expand Down
19 changes: 19 additions & 0 deletions src/tests/Microsoft.VisualStudio.Composition.Tests/ExportMetadataTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,17 @@ public void ImportManyWithInternalMetadataInterface(IContainer container)
Assert.IsType<PartWithExportMetadata>(importingPart.ImportingProperty.Single().Value);
}

/// <summary>
/// Verifies that we can skip visibility checks for more than one assembly.
/// </summary>
[Trait("Access", "NonPublic")]
[MefFact(CompositionEngines.V3EmulatingV1, typeof(ImportManyPartWithInternalMetadataInterface), typeof(ImportManyPartWithInternalMetadataInterface2), typeof(PartWithExportMetadata))]
public void InternalMetadataViewInterfacesAcrossMultipleAssemblies(IContainer container)
{
container.GetExportedValue<ImportManyPartWithInternalMetadataInterface>();
container.GetExportedValue<ImportManyPartWithInternalMetadataInterface2>();
}

[MefFact(CompositionEngines.Unspecified, typeof(ImportingPartWithMetadataInterface), typeof(PartWithExportMetadata))]
[Trait("Efficiency", "InstanceReuse")]
public void MetadataViewInterfaceInstanceSharedAcrossImports(IContainer container)
Expand Down Expand Up @@ -645,6 +656,14 @@ public class ImportManyPartWithInternalMetadataInterface
internal IEnumerable<Lazy<PartWithExportMetadata, IMetadataInternal>> ImportingProperty { get; set; }
}

[MefV1.Export, MefV1.PartCreationPolicy(MefV1.CreationPolicy.NonShared)]
[Export]
public class ImportManyPartWithInternalMetadataInterface2
{
[ImportMany, MefV1.ImportMany]
internal IEnumerable<Lazy<PartWithExportMetadata, AssemblyDiscoveryTests.IInternalMetadataView>> ImportingProperty { get; set; }
}

[MefV1.Export, MefV1.PartCreationPolicy(MefV1.CreationPolicy.NonShared)]
[Export]
public class ImportManyPartWithMetadataClass
Expand Down