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

[release/9.0-staging] Fix getting resource when ResourceResolve returns assembly with resource that is an assembly ref #112893

Open
wants to merge 1 commit into
base: release/9.0-staging
Choose a base branch
from

Conversation

elinor-fung
Copy link
Member

Backport of #112810
Fixes #111537

Customer Impact

  • Customer reported
  • Found internally

[Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.]

Reported in #111537 - Stack overflow on resolving a resource via ResourceResolve for an assembly with an assembly ref manifest resource.

When getting a resource where ResourceResolve handler returns an assembly with a manifest resource that is an assembly ref, we incorrectly resolved the reference on the original assembly instead of the assembly returned by the handler and then also looked for the resource on the original assembly again instead of using the referenced assembly.

Regression

  • Yes
  • No

Regression in .NET 9 from eae1542.

Testing

Added automated test for manifest resource assembly ref. The manifest resource file (as opposed to assembly ref) case is already covered in libraries tests.

Risk

Low.

…rce that is an assembly ref (dotnet#112810)

When getting a resource where `ResourceResolve` handler returns an assembly with a manifest resource that is an assembly ref, we incorrectly resolved the reference on the original assembly instead of the assembly returned by the handler and then also looked for the resource on the original assembly again instead of using the referenced assembly.

This change includes a test for this case using IL. The manifest resource file (as opposed to assembly ref) case is already covered in libraries tests.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot reviewed 1 out of 6 changed files in this pull request and generated no comments.

Files not reviewed (5)
  • src/coreclr/vm/peassembly.cpp: Language not supported
  • src/tests/Loader/ResourceResolve/ManifestResourceAssemblyRef.il: Language not supported
  • src/tests/Loader/ResourceResolve/ManifestResourceAssemblyRef.ilproj: Language not supported
  • src/tests/Loader/ResourceResolve/ResourceAssembly.csproj: Language not supported
  • src/tests/Loader/ResourceResolve/ResourceResolve.csproj: Language not supported
Comments suppressed due to low confidence (2)

src/tests/Loader/ResourceResolve/ResourceResolve.cs:41

  • [nitpick] The variable name 'expectedBytes' is ambiguous. It should be renamed to 'expectedResourceBytes' to better reflect its purpose.
Span<byte> expectedBytes = new byte[expected.Length];

src/tests/Loader/ResourceResolve/ResourceResolve.cs:43

  • [nitpick] The variable name 'streamBytes' is ambiguous. It should be renamed to 'actualResourceBytes' to better reflect its purpose.
Span<byte> streamBytes = new byte[stream.Length];
@elinor-fung elinor-fung added this to the 9.0.x milestone Feb 25, 2025
@elinor-fung elinor-fung added the Servicing-consider Issue for next servicing release review label Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-AssemblyLoader-coreclr Servicing-consider Issue for next servicing release review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant