[generator] fix generic parameter code generation for interface. #100
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Could this please include a unit test change to e.g. |
atsushieno
force-pushed
the
fix-generic-iface-param-use
branch
from
021d434 to
11bafdb
Compare
This fixes bug #43513.
The background for the fix is, well, complicated.
It was generating wrong code:
// This method is explicitly implemented as a member of an instantiated Com.Google.Android.Exoplayer.Drm.IExoMediaDrm
void global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrm.SetOnEventListener (global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrmOnEventListener p0)
{
SetOnEventListener (global::Java.Interop.JavaObjectExtensions.JavaCast<global::Com.Google.Android.Exoplayer.Drm.IExoMediaDrmOnEventListener<Java.Lang.Object>>(p0));
}
where IExoMediaDrmOnEventListener is a normally generated interface ACW
which does not have any generic parameter.
The problem is, the Java API has generic argument T.
Usually it does not matter in normal classes. But reference to an interface
type with generic arguments is treated special: `IRequireGenericMarshal`
It was introduced at old monodroid commit 443598f (which is not in this
Java.Interop repo) for fixing bug #5979 to handle IList and ICollection
for collection marshaling.
So... basically ACWs were not expected to behave as IRequireGenericMarshal.
And now that behavior bites us at bug #43513.
Now, how to fix this new bug? This change introduces another boolean flag
to determine if reference to this type should really output generic
arguments. If it is False, then do not output any further.
This is the best logical explanation I can make right now.
What I still wonder are 1) whether it is only about InterfaceGen, and
2) if we could rather simplify things and eliminate this
IRequireGenericMarshal thing. But so far I have no answer to those.
atsushieno
force-pushed
the
fix-generic-iface-param-use
branch
from
11bafdb to
fee43c9
Compare
|
Sure, added tests. |
jonpryor
added a commit
to jonpryor/java.interop
that referenced
this pull request
Feb 9, 2021
Context: https://medium.com/@alex.birsan/dependency-confusion-4a5d60fec610 Context: https://azure.microsoft.com/en-us/resources/3-ways-to-mitigate-risk-using-private-package-feeds/ Context: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/12676/ncident-help-for-Substitution-attack-risk-from-multiple-package-feeds Changes: dotnet/android-tools@26d65d9...63510cf * dotnet/android-tools@63510cf: [ci] Update packageSources in NuGet.config (dotnet#105) * dotnet/android-tools@83ed0a4: Bump ta xamarin/LibZipSharp/1.0.22@9f563dd1 (dotnet#104) * dotnet/android-tools@8ea78a4: Add Microsoft.Android.Build.BaseTasks project (dotnet#101) * dotnet/android-tools@b2d9fdf: [NDK] Locate and select only compatible NDK versions (dotnet#103) * dotnet/android-tools@5ff1702: [tests] Use dotnet test to run AndroidSdk-Tests (dotnet#102) * dotnet/android-tools@ad80a42: [ci] Use the new "main" default branch (dotnet#100) There is a Package Substitution Attack inherent in NuGet, whereby if multiple package sources provide packages with the same name, it is *indeterminate* which package source will provide the package. For example, consider the [`XliffTasks` package][0], currently provided from the [`dotnet-eng`][1] feed, and *not* present in the NuGet.org feed. If a "hostile attacker" submits an `XliffTasks` package to NuGet.org, then we don't know, and cannot control, whether the build will use the "hostile" `XliffTasks` package from NuGet.org or the "desired" package from `dotnet-eng`. There are two ways to prevent this attack: 1. Use `//packageSources/clear` and have *only one* `//packageSources/add` entry in `NuGet.config` 2. Use `//packageSources/clear` and *fully trust* every `//packageSources/add` entry in `NuGet.config`. `NuGet.org` *cannot* be a trusted source, nor can any feed location which allows "anyone" to add new packages, nor can a feed which itself contains [upstream sources][2]. As the `XliffTasks` package is *not* in `NuGet.org`, option (1) isn't an option. Go with option (2), using the existing `dotnet-eng` source and the new *trusted* [`dotnet-public`][3] package source. [0]: https://github.com/dotnet/xliff-tasks [1]: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=dotnet-eng [2]: https://docs.microsoft.com/en-us/azure/devops/artifacts/concepts/upstream-sources?view=azure-devops [3]: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=dotnet-public
jonpryor
added a commit
to jonpryor/java.interop
that referenced
this pull request
Feb 9, 2021
Context: https://medium.com/@alex.birsan/dependency-confusion-4a5d60fec610 Context: https://azure.microsoft.com/en-us/resources/3-ways-to-mitigate-risk-using-private-package-feeds/ Context: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/12676/ncident-help-for-Substitution-attack-risk-from-multiple-package-feeds Changes: dotnet/android-tools@26d65d9...479931c * dotnet/android-tools@479931c [build] Move global.json file to root directory (dotnet#106) * dotnet/android-tools@63510cf: [ci] Update packageSources in NuGet.config (dotnet#105) * dotnet/android-tools@83ed0a4: Bump ta xamarin/LibZipSharp/1.0.22@9f563dd1 (dotnet#104) * dotnet/android-tools@8ea78a4: Add Microsoft.Android.Build.BaseTasks project (dotnet#101) * dotnet/android-tools@b2d9fdf: [NDK] Locate and select only compatible NDK versions (dotnet#103) * dotnet/android-tools@5ff1702: [tests] Use dotnet test to run AndroidSdk-Tests (dotnet#102) * dotnet/android-tools@ad80a42: [ci] Use the new "main" default branch (dotnet#100) There is a Package Substitution Attack inherent in NuGet, whereby if multiple package sources provide packages with the same name, it is *indeterminate* which package source will provide the package. For example, consider the [`XliffTasks` package][0], currently provided from the [`dotnet-eng`][1] feed, and *not* present in the NuGet.org feed. If a "hostile attacker" submits an `XliffTasks` package to NuGet.org, then we don't know, and cannot control, whether the build will use the "hostile" `XliffTasks` package from NuGet.org or the "desired" package from `dotnet-eng`. There are two ways to prevent this attack: 1. Use `//packageSources/clear` and have *only one* `//packageSources/add` entry in `NuGet.config` 2. Use `//packageSources/clear` and *fully trust* every `//packageSources/add` entry in `NuGet.config`. `NuGet.org` *cannot* be a trusted source, nor can any feed location which allows "anyone" to add new packages, nor can a feed which itself contains [upstream sources][2]. As the `XliffTasks` package is *not* in `NuGet.org`, option (1) isn't an option. Go with option (2), using the existing `dotnet-eng` source and the new *trusted* [`dotnet-public`][3] package source. [0]: https://github.com/dotnet/xliff-tasks [1]: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=dotnet-eng [2]: https://docs.microsoft.com/en-us/azure/devops/artifacts/concepts/upstream-sources?view=azure-devops [3]: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=dotnet-public
jonpryor
added a commit
that referenced
this pull request
Feb 9, 2021
…796) Context: https://medium.com/@alex.birsan/dependency-confusion-4a5d60fec610 Context: https://azure.microsoft.com/en-us/resources/3-ways-to-mitigate-risk-using-private-package-feeds/ Context: https://devdiv.visualstudio.com/DevDiv/_wiki/wikis/DevDiv.wiki/12676/ncident-help-for-Substitution-attack-risk-from-multiple-package-feeds Changes: dotnet/android-tools@26d65d9...479931c * dotnet/android-tools@479931c [build] Move global.json file to root directory (#106) * dotnet/android-tools@63510cf: [ci] Update packageSources in NuGet.config (#105) * dotnet/android-tools@83ed0a4: Bump ta xamarin/LibZipSharp/1.0.22@9f563dd1 (#104) * dotnet/android-tools@8ea78a4: Add Microsoft.Android.Build.BaseTasks project (#101) * dotnet/android-tools@b2d9fdf: [NDK] Locate and select only compatible NDK versions (#103) * dotnet/android-tools@5ff1702: [tests] Use dotnet test to run AndroidSdk-Tests (#102) * dotnet/android-tools@ad80a42: [ci] Use the new "main" default branch (#100) There is a Package Substitution Attack inherent in NuGet, whereby if multiple package sources provide packages with the same name, it is *indeterminate* which package source will provide the package. For example, consider the [`XliffTasks` package][0], currently provided from the [`dotnet-eng`][1] feed, and *not* present in the NuGet.org feed. If a "hostile attacker" submits an `XliffTasks` package to NuGet.org, then we don't know, and cannot control, whether the build will use the "hostile" `XliffTasks` package from NuGet.org or the "desired" package from `dotnet-eng`. There are two ways to prevent this attack: 1. Use `//packageSources/clear` and have *only one* `//packageSources/add` entry in `NuGet.config` 2. Use `//packageSources/clear` and *fully trust* every `//packageSources/add` entry in `NuGet.config`. `NuGet.org` *cannot* be a trusted source, nor can any feed location which allows "anyone" to add new packages, nor can a feed which itself contains [upstream sources][2]. As the `XliffTasks` package is *not* in `NuGet.org`, option (1) isn't an option. Go with option (2), using the existing `dotnet-eng` source and the new *trusted* [`dotnet-public`][3] package source. [0]: https://github.com/dotnet/xliff-tasks [1]: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=dotnet-eng [2]: https://docs.microsoft.com/en-us/azure/devops/artifacts/concepts/upstream-sources?view=azure-devops [3]: https://dev.azure.com/dnceng/public/_packaging?_a=feed&feed=dotnet-public
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes bug #43513.
The background for the fix is, well, complicated.
It was generating wrong code:
where IExoMediaDrmOnEventListener is a normally generated interface ACW
which does not have any generic parameter.
The problem is, the Java API has generic argument T.
Usually it does not matter in normal classes. But reference to an interface
type with generic arguments is treated special:
IRequireGenericMarshalIt was introduced at old monodroid commit 443598f (which is not in this
Java.Interop repo) for fixing bug #5979 to handle IList and ICollection
for collection marshaling.
So... basically ACWs were not expected to behave as IRequireGenericMarshal.
And now that behavior bites us at bug #43513.
Now, how to fix this new bug? This change introduces another boolean flag
to determine if reference to this type should really output generic
arguments. If it is False, then do not output any further.
This is the best logical explanation I can make right now.
What I still wonder are 1) whether it is only about InterfaceGen, and
2) if we could rather simplify things and eliminate this
IRequireGenericMarshal thing. But so far I have no answer to those.