Allow open generic in dynamic method (experimental, not a legal operation, with unpredictable problems) #556
Conversation
…tion, with unpredictable problems) Fix 0xd4d#551
|
How strange code,
|
This is expected behavior. When type is consistent with originalDeclaringType, it will be treated as GenericInstSig. |
|
The information regarding the declaring type is used to determine whether we should interpret the type as an unbound generic instantiation signature or retain original handling. In the case of invalid dynamic methods, we only face a problem with the "declaring type" (in this case
The behavior is only changed if you use the new Importer constructor, other constructors behave the same way. |
@ElektroKill That's why I said what your design(eg. Can only find location from ModuleDef, can't determine from MemberDef) is always clumsy yesterday,
I don't know why don't we do everything inside of dnlib as possible. |
Implementing strange behavior by default which is only needed for an extremely rare case as default is not a good idea as we might introduce a regression, especially since dnlib does not have any unit tests to verify that everything that worked previously works properly now. The reason older dnlib worked is because its handling of generic import was wrong even for valid dynamic methods. Correcting the behavior to match information from reflection resulted in this edge case occurring for invalid dynamic methods. |
|
I don't agree it's strange behavior, why there's |
There was a problem hiding this comment.
Thanks! I'll merge this if this works for @CreateAndInject and if @ElektroKill doesn't find any problems with it.
@wwh1004 You don't like one PR change everyone's usage habits, but this is the exact thing what you do, we have to use the Importer's constructor with |
This PR allows partly fixing the importing logic as an
|
var mi = typeof(My<,>).GetMethod("Test");
var md = (MethodDef)module.ResolveToken(mi.MetadataToken);
var dm = DynamicMethodHelper.ConvertFrom(mi);There's an issue, in my sample, |
If you are unpacking a method using dynamic approach you can always just resolve the method using reflection. If you access dynamic method from a detour/hook then you can probably use something like Stack trace to find the method. |
|
Let me say something like this: static void Do()
{
var dm1 = DynamicMethodBuilder.Build(1057);
dm1.Invoke(null, ..args..);
var dm2 = DynamicMethodBuilder.Build(9527);
dm2.Invoke(null, ..args..);
var dm3 = DynamicMethodBuilder.Build(15);
dm3.Invoke(null, ..args..);
}
class DynamicMethodBuilder
{
public static DynamicMethod Build(int methodID) => throw new NotImplementedException();
} |
|
More clear: static void Do()
{
var walk = DynamicMethodBuilder.Build("walk");
walk.Invoke(null, ..args..);
var run = DynamicMethodBuilder.Build("run");
run.Invoke(null, ..args..);
var fly = DynamicMethodBuilder.Build("fly");
fly.Invoke(null, ..args..);
var jump = DynamicMethodBuilder.Build("jump");
jump.Invoke(null, ..args..);
}
class DynamicMethodBuilder
{
public static DynamicMethod Build(string action)
{
return action switch
{
"walk" => EmitInstrs(),
"run" => ConvertFromMethodBase(),
"fly" => BuildByByteArray(),
"jump" => BuildByAnyOtherWay(),
_ => throw new NotSupportedException(),
};
}
static DynamicMethod EmitInstrs() => throw new NotImplementedException();
static DynamicMethod ConvertFromMethodBase() => throw new NotImplementedException();
static DynamicMethod BuildByByteArray() => throw new NotImplementedException();
static DynamicMethod BuildByAnyOtherWay() => throw new NotImplementedException();
} |
|
If you don't know the the MethodDef corresponding to this dynamic method, how do you restore it? If you know that, the declaring type of the MethodDef is originalDeclaringType. This PR is insensible for normal importing. No one need to change calling parameters or adapt to new behavior. The new constructor for Importer is an optional feature to support the dynamic method with open generic. If you don't use this constructor, this PR can be considered non-existent. |
Why a dynamic method must have a corresponding MethodDef/MethodBase? original: static void Main(string[] args)
{
args.ToList();
Console.WriteLine("hello");
Console.ReadKey();
}protected: static void Main(string[] args)
{
Delegates[215](args);
Delegates[1701](Delegates[51]());
Delegates[20]();
}There're 4 dynamic method instances, we restore 3 calls and 1 ldstr from 4 dynamic method instances, but not every dyanmic method has a corresponding MethodDef/MethodBase. |
|
The declaring type of Main is the originalDeclaringType. Where you get the generic arguments as type instantiation, where is the originalDeclaringType. |
|
Add this file to references, and code following, how to prepare using ClassLibrary1;
using dnlib.DotNet;
using dnlib.DotNet.Emit;
using System;
using System.Reflection;
class Program
{
public static void Main()
{
Test(1);
var dm = DynamicMethodBuilder.Build(null);
dm.GetType().GetMethod("GetMethodDescriptor", BindingFlags.NonPublic | BindingFlags.Instance).Invoke(dm, null);
var module = ModuleDefMD.Load(typeof(Program).Assembly.Location);
module.Context = ModuleDef.CreateModuleContext();
var originalDeclaringType = typeof(Program); // how
var dmr = new DynamicMethodBodyReader(module, dm, new Importer(module, ImporterOptions.TryToUseDefs, default, null, originalDeclaringType), DynamicMethodBodyReaderOptions.UnknownDeclaringType);
dmr.Read();
var instrs = dmr.Instructions;
foreach (var instr in instrs)
Console.WriteLine(instr);
Console.WriteLine("OK");
Console.Read();
}
static void Test(object obj)
{
var dm = DynamicMethodBuilder.Build(obj);
dm.Invoke(null, null);
}
} |
|
@wtfsck As they can't resolve the issue I mentioned, can we eliminate this PR now? |
|
This is an unrealistic scenario. In this case, the method returned by I think you are stretching this problem out and creating unreasonable and unrealistic scenarios to try to show that a fix you wanted is suddenly not necessary because this implementation does not satisfy a scenario that will not happen outside of such a forced example. |
|
Furthermore, the dynamic method created and invoked by |
@ElektroKill Absurd, are you slave of wwh1004? I build DynamicMethod by a complete
Is his DynamicMethod in the picture runnable? Both unrunnable, why his code is good, my code is unreasonable? Code of ClassLibrary1, I don't do anything else than what I said before. public static DynamicMethod Build(object obj)
{
var type = typeof(My<>);
if (obj != null)
type = type.MakeGenericType(obj.GetType());
var mi = type.GetMethod("Test");
return DynamicMethodHelper.ConvertFrom(mi);
} |
Oh wow, I'm not the worshipper or slave of anyone [1] First of all, I should mention that it is necessary to read all of this mess of words before making any conclusions as to the contents. I do not in any way shape or form agree with building uninstantiated dynamic methods. Doing that is invalid in reflection. The dynamic method created in the demo is there to forcefully make the invalid method for the purpose of demonstrating that the PR works. It is in no way, shape, or form correct to make such invalid dynamic methods. Such invalid dynamic methods should not be created at all except for demonstrations like the one in the original PR description. The only somewhat acceptable reason for this is to unpack when you do not want to properly instantiate the types. If we assume that as the only case, such an invalid method is created, then we also know the declaring type as mentioned before. The way the DynamicMethod is created in the original PR description is not appropriate but it's there to simulate a dynamic method created during such unpacking attempt. The reason I said that your example is not a realistic scenario is that if we assume that any code has the method Example scenario I can see for unpacking: class MyType<T> {
public static void Method() {
SomeDynamicMethodBuilder.Build(...).Invoke(null, null);
}
}In order to generate the correct dynamic method here, the code in SomeDynamicMethodBuilder.Build would need to be aware of the values of the generic arguments in order to perform instantiation when building the dynamic method (this is because dynamic methods themselves need to be non-generic). This means that it either has to collect the types of these arguments via a This underlines the fact that just having a This also exposes the fact that in the cases of protectors that use such obfuscation techniques, the generic arguments used for the method must be first explicitly captured before the DynamicMethod builder is executed. This means that the declaring type of the method will also be accessible since the type argument is only available to methods inside it. [1] Please don't make any weird assumptions about me being biased towards anyone or something like this. I'm trying my best to discuss this weird problem and solution to it in as much detail as possible so that the most optimal solution can be implemented and that all information regarding the occurrence of this problem in the real fully functional software space can be considered. I have no grudges towards anyone here. Please don't make this personal when it is not. This is just an issue and PR discussion. Let's focus on discussing the issue at hand instead and try to find the most optimal solution. In case of any questions or clarifications please ask or discuss. |
|
I already made a scene which my code can do better than this PR. |
Fix #551
Test code for dynamic method with open generic:
DynamicTest.zip
Test code for normal importing:
See #466
Description:
This PR introduces a new constructor for Importer to enable open generic in dynamic method supporting.
When pass a non-null value to the new parameter 'originalDeclaringType', this PR is enabled. It changes some importing behavior to be compatible with illegal open generic dynamic methods.
This PR does not introduce other API changes (including additions, deletions, behavior changes), and will not have any impact on normal importing.
To put it simply, this PR will apply MustTreatTypeAsGenericInstType into whole method body importing, not only method sig and so on.
Known issues:
Due to quirks of the .NET reflection system, it is impossible to distinguish whether the operand in ldtoken is a typedef or a typespec when open generics exist. To keep normal importing behavior can get correct result of ldtoken typedef, when open generics exist and it is impossible to distinguish between typedef and typespec, they are uniformly imported as typedef.
Test code in DynamicTest.zip if you don't want to download it.
@ElektroKill @CreateAndInject