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

Adding null check to avoid abort when invalid IL is encountered #57522

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

bholmes
Copy link
Contributor

@bholmes bholmes commented Aug 16, 2021

When a callvirt instruction is encountered on a static method, there is
a hard abort/crash. This commit avoids the problem.

When a callvirt instruction is encountered on a static method, there is
a hard abort/crash. This commit avoids the problem.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 16, 2021
@bholmes
Copy link
Contributor Author

bholmes commented Aug 16, 2021

Here is a small program that shows the abort.

using System;
using System.Reflection;
using System.Reflection.Emit;

namespace ConsoleApp2
{
    class Program
    {
        static void Main(string[] args)
        {
            MethodInfo testMethod = typeof(Program).GetMethod("TestMethod");

            DynamicMethod dynamicMethod = new DynamicMethod("TestMethod", typeof(int), new Type[0], restrictedSkipVisibility: true);
            ILGenerator iLGenerator = dynamicMethod.GetILGenerator();
            iLGenerator.Emit(OpCodes.Callvirt, testMethod);
            iLGenerator.Emit(OpCodes.Ret);
            var ttt = (Func<int>)dynamicMethod.CreateDelegate(typeof(Func<int>));

            Console.WriteLine($"{ttt()}");
        }

        static public int TestMethod ()
        {
            return 42;
        }
    }
}

From this sample, the real problem is probably that CreateDelegate is not throwing because of the invalid IL. My commit does not address this aspect.

@bholmes
Copy link
Contributor Author

bholmes commented Aug 16, 2021

Can we sync this with the Mono repo as well?

@lambdageek
Copy link
Member

/azp run sync-runtime-to-mono

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek
Copy link
Member

@bholmes looks like the bot couldn't do the backport to mono/mono for some reason - could you open a PR manually

@bholmes
Copy link
Contributor Author

bholmes commented Aug 18, 2021

@bholmes looks like the bot couldn't do the backport to mono/mono for some reason - could you open a PR manually

mono/mono#21194

@akoeplinger
Copy link
Member

The wasm test failures are caused by #57501.

@akoeplinger akoeplinger merged commit 1ea615c into dotnet:main Aug 19, 2021
@srxqds
Copy link
Contributor

srxqds commented Aug 19, 2021

backport to .net6.0?

@akoeplinger
Copy link
Member

/backport to release/6.0-rc1

@github-actions
Copy link
Contributor

Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1147170411

@ghost ghost locked as resolved and limited conversation to collaborators Sep 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Codegen-JIT-mono community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants