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/6.0] Fix Assembly.LoadFrom resolver #70537

Merged
merged 1 commit into from
Jun 15, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Jun 10, 2022

Backport of #67890 to release/6.0

/cc @agocke @jkotas

Customer Impact

Using Assembly.LoadFrom with either single-file or dynamic assembly (any assembly which doesn't have a Location) can lead to FileLoadException caused by the Assembly.LoadFrom internal implementation.
Note that the LoadFrom can be called on a real assembly file and succeed, but later on trying to load a dependency for an assembly without Location can lead to the exception.

#69596
#67802

Testing

The fix in .NET 7 has been tested for both the single-file and dynamic assembly case manually.

Risk

Low, the code around the fix didn't change between 6 and 7 and 7 didn't see any regression related to this. Also the fix itself is a simple check to avoid calling Path APIs with empty string input.

Path.GetFullPath fails on empty path. Check for empty requestor path earlier.

Fixes #67802
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jun 10, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #67890 to release/6.0

/cc @agocke @jkotas

Customer Impact

Testing

Risk

IMPORTANT: If this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.

Author: github-actions[bot]
Assignees: -
Labels:

area-AssemblyLoader-coreclr

Milestone: 6.0.x

@jkotas
Copy link
Member

jkotas commented Jun 10, 2022

Did we hear more people hitting this issue?

@vitek-karas
Copy link
Member

@jkotas - yes : #69596

But we should probably fix this regardless because of single-file.
For example:

Assembly.LoadFrom("lib.dll"); // lib.dll exists so this succeeds.
Assembly.Load("NonExistent");

Normally this throws FileNotFoundException, in single-file it throws FileLoadException (with the Path is empty as inner exception).

But even worse (from the bug above):

Assembly.LoadFrom("lib.dll");
Type.GetType("MyType,NonExistent");

Normally the GetType returns null. In single-file it throws FileLoadException. And there's no workaround I could find outside of reimplementing my own LoadFrom.

@vitek-karas
Copy link
Member

I updated the issue description to fill in the impact/test/risk sections.

@agocke agocke added the Servicing-consider Issue for next servicing release review label Jun 13, 2022
@carlossanlop
Copy link
Member

carlossanlop commented Jun 14, 2022

@danmoseley @jeffschwMSFT do we want this change in July servicing? Asking because the Code Complete due date is today and this does not yet have the servicing-approved label.

cc @mmitche in case we need to wait for this change before starting the build. This can be discussed in tomorrow's Tactics.

@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 14, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.7 Jun 14, 2022
@agocke
Copy link
Member

agocke commented Jun 14, 2022

@carlossanlop This was approved this morning.

@mmitche
Copy link
Member

mmitche commented Jun 14, 2022

@carlossanlop Please merge when ready.

@danmoseley
Copy link
Member

@carlossanlop is out I believe.
I'll merge as the test failure is known (needs a back port of #65280 and its predecessor )

@danmoseley danmoseley merged commit 50def09 into release/6.0 Jun 15, 2022
@danmoseley danmoseley deleted the backport/pr-67890-to-release/6.0 branch June 15, 2022 02:46
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants