Implement DynamicMethod.RTDynamicMethod.CreateDelegate. #79427
Conversation
The implementation simply calls back to DynamicMethod.CreateDelegate. fix dotnet#78365
ghost
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-reflection Issue DetailsThe implementation simply calls back to DynamicMethod.CreateDelegate. fix #78365
|
|
|
||
| namespace System.Reflection.Emit.Tests; | ||
|
|
||
| public class DynamicMethodTests |
There was a problem hiding this comment.
I was surprised not to find an existing test file for this; is there one in another place that I just missed?
There was a problem hiding this comment.
DynamicMethod / RTDynamicMethod split is a left-over from Code Access Security that was removed in .NET Core. We did not bring any Core Access Security tests over.
There was a problem hiding this comment.
DynamicMethod tests are under https://github.com/dotnet/runtime/tree/main/src/libraries/System.Reflection.Emit.Lightweight/tests
There was a problem hiding this comment.
Not sure why this didn't come up in my search, but thanks! Once there is some alignment about whether this approach is even ok, I'll relocate the test to that project.
| Func<int> getTargetLength = getLength.Method.CreateDelegate<Func<int>>("ccc"); | ||
| Assert.Equal(3, getTargetLength()); | ||
|
|
||
| Assert.Equal(getLength, getTargetLength.Method.CreateDelegate<Func<string, int>>()); |
There was a problem hiding this comment.
Note: this change is not sufficient to get Delegate.CreateDelegate working because that method requires a RuntimeMethodInfo. I think that could be fixed by also checking for DynamicMethod / RTDynamicMethod explicitly (or perhaps better by just deferring to MethodInfo.CreateDelegate), but that felt out of scope. Let me know if that should be added in this PR or filed as a separate enhancement.
| @@ -430,6 +430,9 @@ public override string ToString() | |||
|
|
|||
| public override CallingConventions CallingConvention => _callingConvention; | |||
|
|
|||
| public override Delegate CreateDelegate(Type delegateType) => _owner.CreateDelegate(delegateType); | |||
There was a problem hiding this comment.
This gives up on controlling the access to the DynamicMethod described in https://github.com/dotnet/runtime/pull/79427/files#diff-d9e708636c5ff1b043b18cbe86e7b7b32267e6a0be131c589d64c11061557fadR382-R383.
If we want to give up on controlling the access to the DynamicMethod, we should just delete RTDynamicMethod and fold with DynamicMethod.
There was a problem hiding this comment.
@jkotas I'm not sure I understand why implementing CreateDelegate changes the degree of access. If you have access to a Delegate instance that was created from a DynamicMethod, you can already invoke the generated code. You can also effectively "cast" between delegate types by creating a wrapper delegate that just invokes the original delegate.
If we get rid of RTDynamicMethod, then a delegate owner could cast to DynamicMethod and access the ILGenerator. I'm not sure that matters, since emitting more IL doesn't seem to change the method behavior once it has been compiled (or perhaps I just didn't structure my method correctly).
If we want to give up on controlling the access to the DynamicMethod, we should just delete RTDynamicMethod and fold with DynamicMethod.
Probably the more important question. Do we want to give up this control? I know CAS is gone, but I don't have context into what exactly this control was accomplishing.
There was a problem hiding this comment.
what exactly this control was accomplishing.
It was preventing MethodInfo acquired from exception or stack trace to be used for invocation. CAS controlled permission for running code. It was not ok to leak permission to run fully trusted piece of code to partially trusted code. For example:
try
{
... call dynamic method that was created by fully trusted code ...
... assume that that the dynamic method threw an exception ...
}
catch (Exception e)
{
// Let's assume that this is partially trusted code. It was important for CAS that the partially trusted code
// cannot gain permission to run the dynamic method by inspecting the exception.
MethodInfo mi = (MethodInfo)e.TargetSite;
// `mi` cannot be used to call the method in any way here.
// `mi.Invoke` fails
// `mi.CreateDelegate` fails
}
Do we want to give up this control?
I think it is fine to give up this control. We do not care about CAS.
There was a problem hiding this comment.
Very interesting! Thanks for the explanation.
Makes sense that this can be done away with though.
The implementation simply calls back to DynamicMethod.CreateDelegate.
fix #78365