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

Incorrect interface dispatch on generics #10837

Closed
jaredpar opened this issue Sep 26, 2018 · 2 comments

Comments

@jaredpar
Copy link
Contributor

@jaredpar jaredpar commented Sep 26, 2018

Steps to Reproduce

Add the following code to Program.cs

using System;
interface Interface<T>
{
    void Method();
}
class Base<T> : Interface<T>
{
    void Interface<T>.Method() { Console.WriteLine("Base.Method()"); }
}
class Derived<U, V> : Base<int>, Interface<U>
{
    void Interface<U>.Method() { Console.WriteLine("Derived`2.Method()"); }
}
class Derived : Derived<int, string>, Interface<string>, Interface<int>
{
    void Interface<string>.Method() { Console.WriteLine("Derived.Method()"); }
}
class Program
{
    public static void Main()
    {
        Interface<int> j = new Derived();
        j.Method();
    }
}

Then

  1. mcs Program.cs
  2. mono Program.exe

Expected Output: "Derived`2.Method()"
Actual Output: "Base.Method()"

Here is a sharp lab demo of the expected behavior: sharp lab demo

Note: this is a simplification of the C# emit test Microsoft.CodeAnalysis.CSharp.UnitTests.CodeGen.CodeGenExplicitImplementationTests.TestExplicitImplementationInBaseGenericType2

On which platforms did you notice this

Ubuntu 18.04 using Mono 5.19.0.209

@marek-safar

This comment has been minimized.

Copy link
Member

@marek-safar marek-safar commented Sep 27, 2018

@kumpera @vargaz that needs your attention

@kumpera kumpera self-assigned this Sep 28, 2018
kumpera added a commit to kumpera/mono that referenced this issue Oct 2, 2018
…stead of doing it from the grounds up. Fixes mono#10837

Say you have;
```
interface Bar<T> { T Bla(); }
class Foo<A,B>: IBar<A>, IBar<B> {
	int IBar<int>.Bla)() { ... }
}
```

The GTD of Foo will have the following interface offsets table:
```
4: IBar<A>
5: IBar<B>
```

With the existing code, if you have Foo<int,int> the interface offsets table will look like this:
```
4: IBar<int>
```

The problem with that is it will produce a hole in the vtable and subclasses of Foo<int,int> will have not on slot 5.

That would be just wasteful, unless you have this:

```
class Sub: Foo<int,int>, IBar<int> {
	int IBar<int>.Bla)() { ...  }
}
```

IOW, a subclass that overrides the interface implementation in Foo. The problem is that it's not always reliable to which iface offset
calls to `IBar<int>` will go through and thus it's possible to have calls made against instances of `Sub` to execute the code from its base
class.

The solution is to inflate the interface offsets table from the gtd as that will preserve the number of entries and will cause the
rest of the vtable layout code to fill the duplicated slots correctly.
@kumpera

This comment has been minimized.

Copy link
Member

@kumpera kumpera commented Oct 2, 2018

Tentative fix: #10931

kumpera added a commit to kumpera/mono that referenced this issue Oct 2, 2018
kumpera added a commit to kumpera/mono that referenced this issue Oct 5, 2018
…stead of doing it from the grounds up. Fixes mono#10837

Say you have;
```
interface Bar<T> { T Bla(); }
class Foo<A,B>: IBar<A>, IBar<B> {
	int IBar<int>.Bla)() { ... }
}
```

The GTD of Foo will have the following interface offsets table:
```
4: IBar<A>
5: IBar<B>
```

With the existing code, if you have Foo<int,int> the interface offsets table will look like this:
```
4: IBar<int>
```

The problem with that is it will produce a hole in the vtable and subclasses of Foo<int,int> will have not on slot 5.

That would be just wasteful, unless you have this:

```
class Sub: Foo<int,int>, IBar<int> {
	int IBar<int>.Bla)() { ...  }
}
```

IOW, a subclass that overrides the interface implementation in Foo. The problem is that it's not always reliable to which iface offset
calls to `IBar<int>` will go through and thus it's possible to have calls made against instances of `Sub` to execute the code from its base
class.

The solution is to inflate the interface offsets table from the gtd as that will preserve the number of entries and will cause the
rest of the vtable layout code to fill the duplicated slots correctly.
kumpera added a commit to kumpera/mono that referenced this issue Oct 5, 2018
kumpera added a commit to kumpera/mono that referenced this issue Oct 16, 2018
…stead of doing it from the grounds up. Fixes mono#10837

Say you have;
```
interface Bar<T> { T Bla(); }
class Foo<A,B>: IBar<A>, IBar<B> {
	int IBar<int>.Bla)() { ... }
}
```

The GTD of Foo will have the following interface offsets table:
```
4: IBar<A>
5: IBar<B>
```

With the existing code, if you have Foo<int,int> the interface offsets table will look like this:
```
4: IBar<int>
```

The problem with that is it will produce a hole in the vtable and subclasses of Foo<int,int> will have not on slot 5.

That would be just wasteful, unless you have this:

```
class Sub: Foo<int,int>, IBar<int> {
	int IBar<int>.Bla)() { ...  }
}
```

IOW, a subclass that overrides the interface implementation in Foo. The problem is that it's not always reliable to which iface offset
calls to `IBar<int>` will go through and thus it's possible to have calls made against instances of `Sub` to execute the code from its base
class.

The solution is to inflate the interface offsets table from the gtd as that will preserve the number of entries and will cause the
rest of the vtable layout code to fill the duplicated slots correctly.
kumpera added a commit to kumpera/mono that referenced this issue Oct 16, 2018
kumpera added a commit to kumpera/mono that referenced this issue Oct 16, 2018
…stead of doing it from the grounds up. Fixes mono#10837

Say you have;
```
interface Bar<T> { T Bla(); }
class Foo<A,B>: IBar<A>, IBar<B> {
	int IBar<int>.Bla)() { ... }
}
```

The GTD of Foo will have the following interface offsets table:
```
4: IBar<A>
5: IBar<B>
```

With the existing code, if you have Foo<int,int> the interface offsets table will look like this:
```
4: IBar<int>
```

The problem with that is it will produce a hole in the vtable and subclasses of Foo<int,int> will have not on slot 5.

That would be just wasteful, unless you have this:

```
class Sub: Foo<int,int>, IBar<int> {
	int IBar<int>.Bla)() { ...  }
}
```

IOW, a subclass that overrides the interface implementation in Foo. The problem is that it's not always reliable to which iface offset
calls to `IBar<int>` will go through and thus it's possible to have calls made against instances of `Sub` to execute the code from its base
class.

The solution is to inflate the interface offsets table from the gtd as that will preserve the number of entries and will cause the
rest of the vtable layout code to fill the duplicated slots correctly.
kumpera added a commit to kumpera/mono that referenced this issue Oct 16, 2018
kumpera added a commit to kumpera/mono that referenced this issue Nov 5, 2018
…stead of doing it from the grounds up. Fixes mono#10837

Say you have;
```
interface Bar<T> { T Bla(); }
class Foo<A,B>: IBar<A>, IBar<B> {
	int IBar<int>.Bla)() { ... }
}
```

The GTD of Foo will have the following interface offsets table:
```
4: IBar<A>
5: IBar<B>
```

With the existing code, if you have Foo<int,int> the interface offsets table will look like this:
```
4: IBar<int>
```

The problem with that is it will produce a hole in the vtable and subclasses of Foo<int,int> will have not on slot 5.

That would be just wasteful, unless you have this:

```
class Sub: Foo<int,int>, IBar<int> {
	int IBar<int>.Bla)() { ...  }
}
```

IOW, a subclass that overrides the interface implementation in Foo. The problem is that it's not always reliable to which iface offset
calls to `IBar<int>` will go through and thus it's possible to have calls made against instances of `Sub` to execute the code from its base
class.

The solution is to inflate the interface offsets table from the gtd as that will preserve the number of entries and will cause the
rest of the vtable layout code to fill the duplicated slots correctly.
kumpera added a commit to kumpera/mono that referenced this issue Nov 5, 2018
lambdageek added a commit that referenced this issue Nov 15, 2018
…stead of doing it from the grounds up. (#10931)

* [metadata] Compute interface offsets of ginst by inflating the GTD instead of doing it from the grounds up. Fixes #10837

Say you have;
```
interface Bar<T> { T Bla(); }
class Foo<A,B>: IBar<A>, IBar<B> {
	int IBar<int>.Bla)() { ... }
}
```

The GTD of Foo will have the following interface offsets table:
```
4: IBar<A>
5: IBar<B>
```

With the existing code, if you have Foo<int,int> the interface offsets table will look like this:
```
4: IBar<int>
```

The problem with that is it will produce a hole in the vtable and subclasses of Foo<int,int> will have not on slot 5.

That would be just wasteful, unless you have this:

```
class Sub: Foo<int,int>, IBar<int> {
	int IBar<int>.Bla)() { ...  }
}
```

IOW, a subclass that overrides the interface implementation in Foo. The problem is that it's not always reliable to which iface offset
calls to `IBar<int>` will go through and thus it's possible to have calls made against instances of `Sub` to execute the code from its base
class.

The solution is to inflate the interface offsets table from the gtd as that will preserve the number of entries and will cause the
rest of the vtable layout code to fill the duplicated slots correctly.

* Add regression test for #10837.

* [sre] Replace assert with proper error handling.

* [metadata] Don't mono_class_init ifaces, just have offsets setup.

* [metadata] Make setup_interface_offsets not depend on mono_class_init and fix lazy iid computation.

All that interface offset setup needs are iid as the output in interfaces_packed is sorted by it. Remove
the need to use mono_class_init by properly extracting and making iid setup lazy.

Beyond that, make mono_get_unique_iid not return 0 as a valid iid.
Otherwise we don't have a proper sentinel value when lazily computing iid.

* [metadata] Keep interfaces in the same relative order when computing offsets

qsort() is not guaranteed to be a stable sort (one that keeps elements with the
same key in the same order).  For example, the msvc qsort isn't stable.

Add an insertion_order field to ClassAndOffset to keep interfaces with the same
interface_id in the same relative order.

This is all relevant because in setup_interface_offsets we could see the same
interface multiple times (for example, with generics) at different offsets.
jonpryor added a commit to xamarin/xamarin-android that referenced this issue Apr 24, 2019
Bumps to mono/api-snapshot@ae01378
Bumps to mono/reference-assemblies@e5173a5
Bumps to mono/bockbuild@d30329d
Bumps to mono/boringssl@3d87996
Bumps to mono/corefx@72f7d76
Bumps to mono/corert@1b7d4a1
Bumps to mono/helix-binaries@7e893ea
Bumps to mono/illinker-test-assets@f21ff68
Bumps to mono/linker@13d864e
Bumps to mono/llvm@1aaaaa5 [mono]
Bumps to mono/llvm@2c2cffe [xamarin-android]
Bumps to mono/NUnitLite@0029561
Bumps to mono/roslyn-binaries@0bbc9b4
Bumps to mono/xunit-binaries@8f6e62e

	$ git diff --shortstat 886c4901..e66c7667      # mono
        3597 files changed, 350850 insertions(+), 91128 deletions(-)
	$ git diff --shortstat 349752c464c5fc93b32e7d45825f2890c85c8b7d..2c2cffedf01e0fe266b9aaad2c2563e05b750ff4
	 240 files changed, 18562 insertions(+), 6581 deletions(-)

Context: dotnet/coreclr#22046

Fixes: CVE 2018-8292 on macOS
Fixes: http://work.devdiv.io/737323
Fixes: dotnet/corefx#33965
Fixes: dotnet/standard#642
Fixes: mono/mono#6997
Fixes: mono/mono#7326
Fixes: mono/mono#7517
Fixes: mono/mono#7750
Fixes: mono/mono#7859
Fixes: mono/mono#8360
Fixes: mono/mono#8460
Fixes: mono/mono#8766
Fixes: mono/mono#8922
Fixes: mono/mono#9418
Fixes: mono/mono#9507
Fixes: mono/mono#9951
Fixes: mono/mono#10024
Fixes: mono/mono#10030
Fixes: mono/mono#10038
Fixes: mono/mono#10448
Fixes: mono/mono#10735
Fixes: mono/mono#10735
Fixes: mono/mono#10737
Fixes: mono/mono#10743
Fixes: mono/mono#10834
Fixes: mono/mono#10837
Fixes: mono/mono#10838
Fixes: mono/mono#10863
Fixes: mono/mono#10945
Fixes: mono/mono#11020
Fixes: mono/mono#11021
Fixes: mono/mono#11021
Fixes: mono/mono#11049
Fixes: mono/mono#11091
Fixes: mono/mono#11095
Fixes: mono/mono#11123
Fixes: mono/mono#11138
Fixes: mono/mono#11146
Fixes: mono/mono#11202
Fixes: mono/mono#11214
Fixes: mono/mono#11317
Fixes: mono/mono#11326
Fixes: mono/mono#11378
Fixes: mono/mono#11385
Fixes: mono/mono#11478
Fixes: mono/mono#11479
Fixes: mono/mono#11488
Fixes: mono/mono#11489
Fixes: mono/mono#11527
Fixes: mono/mono#11529
Fixes: mono/mono#11596
Fixes: mono/mono#11603
Fixes: mono/mono#11613
Fixes: mono/mono#11623
Fixes: mono/mono#11663
Fixes: mono/mono#11681
Fixes: mono/mono#11684
Fixes: mono/mono#11693
Fixes: mono/mono#11697
Fixes: mono/mono#11779
Fixes: mono/mono#11809
Fixes: mono/mono#11858
Fixes: mono/mono#11895
Fixes: mono/mono#11898
Fixes: mono/mono#11898
Fixes: mono/mono#11965
Fixes: mono/mono#12182
Fixes: mono/mono#12193
Fixes: mono/mono#12218
Fixes: mono/mono#12235
Fixes: mono/mono#12263
Fixes: mono/mono#12307
Fixes: mono/mono#12331
Fixes: mono/mono#12362
Fixes: mono/mono#12374
Fixes: mono/mono#12402
Fixes: mono/mono#12421
Fixes: mono/mono#12461
Fixes: mono/mono#12479
Fixes: mono/mono#12479
Fixes: mono/mono#12552
Fixes: mono/mono#12603
Fixes: mono/mono#12747
Fixes: mono/mono#12831
Fixes: mono/mono#12843
Fixes: mono/mono#12881
Fixes: mono/mono#13030
Fixes: mono/mono#13284
Fixes: mono/mono#13297
Fixes: mono/mono#13455
Fixes: mono/mono#13460
Fixes: mono/mono#13478
Fixes: mono/mono#13479
Fixes: mono/mono#13522
Fixes: mono/mono#13607
Fixes: mono/mono#13610
Fixes: mono/mono#13610
Fixes: mono/mono#13639
Fixes: mono/mono#13672
Fixes: mono/mono#13834
Fixes: mono/mono#13878
Fixes: mono/mono#6352
Fixes: mono/monodevelop#6898
Fixes: xamarin/maccore#1069
Fixes: xamarin/maccore#1407
Fixes: xamarin/maccore#604
Fixes: xamarin/xamarin-macios#4984
Fixes: xamarin/xamarin-macios#5289
Fixes: xamarin/xamarin-macios#5363
Fixes: xamarin/xamarin-macios#5381
Fixes: https://issuetracker.unity3d.com/issues/editor-crashes-with-g-logv-when-entering-play-mode-with-active-flowcanvas-script
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.