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.

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

[mono][interp] Resolve virtual method on delegates created by compiled code #101168

Merged
merged 1 commit into from Apr 17, 2024

Conversation

BrzVlad
Copy link
Member

@BrzVlad BrzVlad commented Apr 17, 2024

Creating a delegate would normally end up calling into the runtime via ves_icall_mono_delegate_ctor. However, jit/aot backand have a fastpath where the delegate is not fully initialized (relying on the delegate trampoline to resolve the actual method to be called when the delegate is first called). Interp delegate initialization therefore doesn't take place. If this is the case and the delegate method is virtual, we would need to resolve it based on the target object.

Fixes #101074

…d code

Creating a delegate would normally end up calling into the runtime via ves_icall_mono_delegate_ctor. However, jit/aot backand have a fastpath where the delegate is not fully initialized (relying on the delegate trampoline to resolve the actual method to be called when the delegate is first called). Interp delegate initialization therefore doesn't take place. If this is the case and the delegate method is virtual, we would need to resolve it based on the target object.
Copy link
Contributor

Tagging subscribers to this area: @BrzVlad, @kotlarmilos
See info in area-owners.md if you want to be subscribed.

@simonrozsival
Copy link
Member

We should backport this fix to a .NET 8 servicing release

@simonrozsival
Copy link
Member

/cc @vitek-karas

@vitek-karas
Copy link
Member

+1 on backporting. @BrzVlad how risky is this fix for servicing?

@BrzVlad
Copy link
Member Author

BrzVlad commented Apr 18, 2024

I think it is rather low risk (but not 100% risk free as I would have considered all my previous backports in general). The main problem is that we don't have good testing for this code path.

@simonrozsival
Copy link
Member

/backport to release/8.0-staging

Copy link
Contributor

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/8754040309

@vitek-karas
Copy link
Member

@BrzVlad Sorry for not understanding the code change, so I'm going to ask: The new behavior will kick in only for the cases which would fail otherwise, or would it also be used in some cases which already work today and this might change that behavior?

@BrzVlad
Copy link
Member Author

BrzVlad commented Apr 23, 2024

@vitek-karas So long story short, if the delegate is created by code that doesn't handle interp related initialization (when jit inlines some delegate initialization) we hit this path in interpreter when first invoking the delegate. What this change does is when that first invoke happens and the method is virtual we resolve the virtual method on the target object. So, in theory, there could have been cases where this code was calling the correct method by chance and after now resolving the virtual method something unexpected could happen, but I would say the chance is very low.

@simonrozsival
Copy link
Member

So, in theory, there could have been cases where this code was calling the correct method by chance and after now resolving the virtual method something unexpected could happen, but I would say the chance is very low.

@BrzVlad so does this mean that there could be a case like

class BaseClass
{
    public virtual int GetValue() => 1;
}

class DerivedClass : BaseClass
{
    public override int GetValue() => 2;
}

where the developer would call a delegate for inst.GetValue and they would be getting 1 because of the bug and their code would expect this behavior, and after this change, it will break because it will start returning 2?

@BrzVlad
Copy link
Member Author

BrzVlad commented Apr 23, 2024

I wouldn't really worry about cases like that. I was rather saying that maybe in your case, the target object is a BaseClass so BaseClass.GetValue is the correct method to call, but with the fix a virtual method lookup happens on the target object, and maybe something unexpected can happen here. I can't pinpoint something specific, it is unlikely for something to break, just saying.

matouskozak pushed a commit to matouskozak/runtime that referenced this pull request Apr 30, 2024
…d code (dotnet#101168)

Creating a delegate would normally end up calling into the runtime via ves_icall_mono_delegate_ctor. However, jit/aot backand have a fastpath where the delegate is not fully initialized (relying on the delegate trampoline to resolve the actual method to be called when the delegate is first called). Interp delegate initialization therefore doesn't take place. If this is the case and the delegate method is virtual, we would need to resolve it based on the target object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Mono] App crashes with BadImageFormatException: Method has no body
5 participants