Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Prevent ImplicitTransform of index getter method (#41418) #41647

Conversation

josalem
Copy link

@josalem josalem commented Oct 8, 2019

Description

Cherry-pick of #41418 to release/3.1

Fix for the exception thrown in dotnet/coreclr#26890 that is a fallout of the issue being fixed in DiagnosticSourceEventSource in conjunction with dotnet/aspnetcore#11730.

small repro (click to expand)

Attach dotnet trace to this app and turn on the Microsoft-Diagnostics-DiagnosticSource provider.

    class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine($"PID: {System.Diagnostics.Process.GetCurrentProcess().Id}");
            var diagnosticListener = new DiagnosticListener("MySource");
            while (true)
            {
                Console.Write(">");
                var input = Console.ReadLine();
                if (input == "exit")
                    break;

                diagnosticListener.Write("MyEvent", new List<int> { 1, 2, 3 });
            }
        }
    }

Root Cause of Exception:

The getter for the implicit Item property for the index operator takes an argument, and DiagnosticSourceEventSource makes the assumption that property getters don't take arguments. It tries to bind the resulting delegate of type [retval] get_Item(int32 index) to the type Func<TObject, TProperty> so the binding fails and we get the exception you see in dotnet/coreclr#26890.

This change prevents DiagnosticSourceEventSource from attempting to serialize the implicit Item property by blocking index getters.

Customer Impact

ASP.NET Core has used DiagnosticSourceEventSource for their eventing. Since they changed their event types to inherit from IReadOnlyCollection in 3.0, they will all hit the exception in dotnet/coreclr#26890 when turned on causing a bad diagnostic experience if you do not explicitly specify a transform.

Regression

I don't believe this is a regression as the code existed in the 2.0 time frame as well, but it was rarely encountered. The difference now being that after dotnet/aspnetcore#11730, the behavior is being hit routinely by ASP.NET Core when diagnosing an MVC app.

Risk

Minimal. This code prevents an exception from being thrown and prevents bad behavior without changing expected behavior.

Tests

A test were added to validate that this change prevents the implicit index getter from being serialized.

CC - @tommcdon @noahfalk

* Prevent TransformSpec from attempting to implicitly create a delegate for index properties

* Add tests
@josalem josalem added this to the 3.1 milestone Oct 8, 2019
@josalem josalem requested a review from MeiChin-Tsai October 8, 2019 18:31
@josalem josalem self-assigned this Oct 8, 2019
@danmoseley
Copy link
Member

I'm fine with merging this into 3.1 since it's evidently causing problems for ASP.NET

@josalem
Copy link
Author

josalem commented Oct 9, 2019

@danmosemsft have you heard anyone else running in to this issue besides the linked coreclr/aspnet issue?

@danmoseley
Copy link
Member

No. My assumption was that you are bringing this because it's being requested by ASP.NET. @rynowak ?

@josalem for clarity could you reformat the top post per the template: dotnet/coreclr#26910

@josalem
Copy link
Author

josalem commented Oct 9, 2019

You are correct, I was just seeing if you had heard of this independently from another source than the issue I linked. I'll update the PR text.

@rynowak
Copy link
Member

rynowak commented Oct 9, 2019

I wasn't aware of this at all. Based on the impact it sounds like we need to fix it, right?

@MeiChin-Tsai
Copy link

Approved for 3.1. Please read the Teams' channel info regarding freeze period.

@josalem josalem merged commit 080eeeb into dotnet:release/3.1 Oct 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants