Add ImportAsOperand to fix DynamicMethodBodyReader

[CreateAndInject]

Not every issue is produced from #452 #466, some issues exist a long time, close #551

DynamicTest.zip

Test code:

class Program
{
    public static void Main()
    {
        var module = ModuleDefMD.Load(typeof(Program).Assembly.Location);
        module.Context = ModuleDef.CreateModuleContext();
        var mi = typeof(My<>).GetMethod("Test");
        var md = (MethodDef)module.ResolveToken(mi.MetadataToken);
        var dm = DynamicMethodHelper.ConvertFrom(mi);
        dm.GetType().GetMethod("GetMethodDescriptor", BindingFlags.NonPublic | BindingFlags.Instance).Invoke(dm, null);
        var dmr = new DynamicMethodBodyReader(module, dm, new Importer(module, ImporterOptions.TryToUseDefs, GenericParamContext.Create(md)), DynamicMethodBodyReaderOptions.UnknownDeclaringType);
        dmr.Read();
        var instrs = dmr.Instructions;
        var exceptions = dmr.ExceptionHandlers;
        var locals = dmr.Locals;
        Console.WriteLine("OK");
        Console.Read();
    }
}

class My<T> : Exception where T : Exception
{
    public static T StaticField;
    public T InstanceField;

    public static void Test()
    {
        try
        {
            Console.WriteLine(typeof(T));
            Console.WriteLine(typeof(My<T>));
            Console.WriteLine(StaticField);
            Console.WriteLine(My<Exception>.StaticField);
            Console.WriteLine(new My<Exception>().InstanceField);
            Console.WriteLine(new My<T>().InstanceField);
            Activator.CreateInstance<T>();
            Activator.CreateInstance<My<T>>();
            Static();
            GenericStatic(StaticField);
            new My<Exception>().Instance();
            new My<Exception>().GenericInstance("");
            new My<T>().Instance();
            new My<T>().GenericInstance("");
            new List<T>().Add(default);
            new List<int>().Add(0);
        }
        catch (My<T> ex)
        {
        }
        catch (My<Exception> ex)
        {
        }
        catch (T ex)
        {
        }
    }

    public static void Static() { }

    public static void GenericStatic<U>(U a) { }

    public void Instance() { }

    public void GenericInstance<U>(U a) { }
}

[CreateAndInject]

@wtfsck What happen?

[wtfsck]

I'll have to check this later, hopefully next week.

[wwh1004]

The new implementation can't distinguish between ldtoken typedef and ldtoken typespec.

void Method<T>()
{
_ = typeof(List<>);
_ = typeof(List<T>);
}

[CreateAndInject]

The new implementation can't distinguish between ldtoken typedef and ldtoken typespec.

void Method<T>()
{
_ = typeof(List<>);
_ = typeof(List<T>);
}

Seems we can't distinguish them in dnlib, since we get the same object, see:
TokenResolver.cs in DynamicTest.zip

        public MemberInfo AsMember(int token) {
            return m_module.ResolveMember(token, m_typeContext, m_methodContext);
        }

ResolveMember get the same object for 0x02000005(typedef) and 0x1b000003(typespec)
We can't distinguish them unless we know the original byte[] to check it's 0x02 or 0x1b.

[CreateAndInject]

The new implementation can't distinguish between ldtoken typedef and ldtoken typespec.

void Method<T>()
{
_ = typeof(List<>);
_ = typeof(List<T>);
}

Yes, this code can be improved: Check if there's generic parameter in the context, if no, we can't use TypeSpec, but if yes, I have no idea.

[CreateAndInject]

Seems this only happen in ReadInlineTok(ldtoken), but never happen on ReadInlineType/CatchType, and never happen on method/field also, how about call Import in ReadInlineTok, otherwise call ImportAsOperand?

Of course, this is not a perfect solution, if user want to get 100% correct result, they have to build one more DynamicMethod with instantiation types, then check if there's ldtoken TypeDef/TypeRef => ldtoken TypeSpec, if it happen, change the operand to TypeSpec(Use GenericVar in GenericArguments).

How do you think?

[ElektroKill]

I don’t think adding a isLdtoken field when importing is a good idea. Instead the importer should guarantee that the reflection object passed into it always produces a corresponding dnlib object. It seems that in this case the ImportAsOperand method is just producing a wrong dnlib object which doesn’t match what reflection provides. This is obvious as there is a clear difference between an unbound generic type (List<>) and an instantiated generic type List where T is a method generic parameter. One should produce a uninstantiated TypeDef or TypeRef object and optionally have it wrapped in a ClassOrValueTypeSig and the second one should produce a GenericInstSig which is optionally wrapped in a TypeSped. The ImportAsOperand method doesn’t seem to handle this clear and important difference which is incorrect. Instead of trying to avoid encountering this difference by adding a “hack fix”, the root cause of the import error should be identified and resolved.

[ElektroKill]

Furthermore, I’m not sure whether adding so much additional code in the form of new ImportAsOperand methods is a good idea here. The code issue in reference mentions nothing in regard to special handling of field and types and yet here we see such changes. Why? The issue in question only mentions the importing of method operands as MemberRef/MethodSpec. More explanation as to the reasons is necessary here in order to evaluate this PR.

im currently away from home but when i get back I will look more into this apparent issue with generics, dynamic methods and the importer.

[CreateAndInject]

Instead the importer should guarantee that the reflection object passed into it always produces a corresponding dnlib object.

That's what Import do, it's why I add ImportAsOperand rather than change the behavior of Import.

The ImportAsOperand method doesn’t seem to handle this clear and important difference which is incorrect.

1. ldtoken: keep the same as original
2. other: better
3. nothing worse
   ImportAsOperand just support one more choice for the user, it doesn't change the behavior of Import, can you support a demo which DynamicMethodBodyReader + Import work fine, but DynamicMethodBodyReader + ImportAsOperand generated incorrect code?

The following code is a classic DynamicMethod protector, all methods are encrypted like this, if DynamicMethodBodyReader doesn't support uninstantiation types, could you tell me how to make an unpacker to restore the code? Anyway, it's welcome if you have a better way.

class My<T>
{
    public bool Test<U>(T a, U b)
    {
        object[] args = { this, a, b };
        DynamicMethod dynamicMethod = DynamicMethodBuilder.Create(751);
        return (bool)dynamicMethod.Invoke(null, args);
    }
}

// Generated by protector
class DynamicMethodBuilder
{
    public static DynamicMethod Create(int methodID) => throw new NotImplementedException();
}

[ElektroKill]

Okay, so I've looked into this a bit and this PR seems to be fixing a problem when importing a DynamicMethod in reflection which would result in an execution exception as soon as it is invoked. If we modify the example provided in the issue to include an additional statement:

Console.WriteLine(typeof(My<T>));
Console.WriteLine(typeof(My<>));

and debug the dynamic method body reader it is noticeable that in the dynamic method created the type used for both of these ldtokens is the EXACLTY THE SAME. There is no way to differentiate between these two cases! This is direct evidence that using DynamicMethodBodyReader in cases like these where the types in the dynamic method are uninstantiated/unbound will always result in incorrect code as information is lost. This dynamic method which is passed to the reader will fail upon execution as pointed out by @wwh1004. In cases where we can see a clear failure to read correct code no matter what we do (the object's properties are the same) I think it's not worth messing with the importer and reader to attempt to create valid code for different cases in similarly broken dynamic methods. Even if we do mess with the importer for this extreme edge case, fixing only part of the issue is, in my opinion, frowned upon.

As for the actual hack fix, the implementation appears to be okay to me at first glance since ldtoken is the only instruction that accepts unbound generic types (This is something we should verify not only in spec but at runtime, if someone knows the answer feel free to chime in! It could be that ldtoken is the only one since it's the only inlinetok instruction but it needs checking) meaning that relying on IsGenericType as an indication of whether we should read generic arguments seems okay? Although I remember there were some issues with importing generics in the past, perhaps @wwh1004 can chime into this after working on improving the generic import before.

If we do end up going through on this PR I think the naming of the newly added members should be improved to be more clear.

• Using ldtoken as a parameter name in DynamicMethodBodyReader is strange, I think we should we should use something like isTokenOperand or something similar to better reflect that this applies to InlineTok operand type instructions
• The name ImportAsOperand is a strange choice as this method is not used for all operand types (inlinetok is excluded). I think the name should better reflect the fact that the behavior of the generic import is very different. I'm not very good with coming up with names but ImportAsOperand seems like it could be confusing for users not super familiar with reflection.

Hope my little investigation helps in handling this strange and abnormal edge case!

[CreateAndInject]

fixing only part of the issue is, in my opinion, frowned upon.

Don't agree, consider a man has cancer, there's a medicine, but it's not 100% useful, it can heal him in 99% cases, do you think whether we should use it when we don't have a better way? IMPORTANT, even if 1% cases happen, it doesn't make anything worse.

Currently, DynamicMethodBodyReader almost completely generate wrong IL for generic types/methods. Why do I build an invalid DynamicMethod? It's not for executing purpose, it's my trick to restore IL, otherwise, how can we restore a generate type/method when it's protected to DynamicMethod? A valid and runnable DynamicMethoid always don't contain uninstantiation types, but the original method contains!

This implemention only has issue when there's typeof(My<T>), it work fine for typeof(My<>) and typeof(My<int>) and any other instructions.

100% issue on generic type/method vs 1%, how will you choice?

[ElektroKill]

Currently, DynamicMethodBodyReader almost completely generate wrong IL for generic types/methods.

This is not true, in cases where the types are actually correct, the generated code is 100% valid and correct. The reader can restore the code back to the original without issues. Issues appear when you pass in an invalid DynamicMethod.

Instead of creating invalid methods for your unpacking here is a trick you can apply:

Define a dynamic assembly and module, some dummy types like DummyType1, DummyType2, for every generic parameter a type has to make sure the types match the constraints. Then when using reflection to obtain the generic type or method, instantiate them with these dummy types. Then perform reading which will give dnlib types instantiated with dummy types. After reading just modify each typesignature which contains dummytype reference to correct generic parameter reference. This way you always create a valid dynamic method in reflection and do not risk any corruption.

Furthermore, even if this is merged, using your current way of unpacking, which creates dynamic methods that are invalid, allows the possibility of corruption of IL in the case I mentioned above:

Console.WriteLine(typeof(My<T>));
Console.WriteLine(typeof(My<>));

A valid and runnable DynamicMethoid always don't contain uninstantiation types, but the original method contains!

This is not true, dynamic method can contain uninstantiated generic types when using the ldtoken instruction in for example a typeof expression.

[CreateAndInject]

This is not true, in cases where the types are actually correct, the generated code is 100% valid and correct.

Notice: what I mentioned is generic types/methods, the original type is generic parameter T.

Define a dynamic assembly and module, some dummy types like DummyType1, DummyType2, for every generic parameter a type has to make sure the types match the constraints.

Base type or interface constraint in host assembly may not be visible to unpacker assembly.

This is not true, dynamic method can contain uninstantiated generic types when using the ldtoken instruction in for example a typeof expression.

That's why there's 1 issue remaining. ImportAsOperand has ability to generate proper instructions when an instruction of a dynamic method can't contain uninstantiated generic types.

I think we should we should use something like isTokenOperand or something similar to better reflect that this applies to InlineTok operand type instructions

There's ITokenOperand in dnlib, isTokenOperand indicate any types derived from ITokenOperand, people may mistakenly assume any instructions with ITokenOperand as operand, eg. call/ldfld/box.

[ElektroKill]

Base type or interface constraint in host assembly may not be visible to unpacker assembly.

This is why I suggest generating dynamic modules and assembly. If you do this, you can configure this module to skip accessibility checks to the file you are trying to unpack, meaning you can make your dummy types inherit any type in order to match generic parameter constraints. You can look into attributes like IgnoresAccessChecksToAttribute which can be used to disable access checks at runtime. It's not documented but if you look at runtime source code you will find references to it.

There's ITokenOperand in dnlib, isTokenOperand indicate any types derived from ITokenOperand, people may mistakenly assume any instructions with ITokenOperand as operand, eg. call/ldfld/box.

It was just a suggestion, we could also use names like isInlineTok or what was used in PR #556

[CreateAndInject]

Seems people don't like it, just close