-
Notifications
You must be signed in to change notification settings - Fork 7.7k
PS should try to load the assembly from file path first before loading from assemblyName #4196
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
Changes from all commits
cb1f0af
192de66
52ac1bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1323,110 +1323,72 @@ internal void RemoveAssembly(string name) | |
[SuppressMessage("Microsoft.Reliability", "CA2001:AvoidCallingProblematicMethods", MessageId = "System.Reflection.Assembly.LoadFrom")] | ||
internal static Assembly LoadAssembly(string name, string filename, out Exception error) | ||
{ | ||
// First we try to load the assembly based on the given name | ||
|
||
Assembly loadedAssembly = null; | ||
error = null; | ||
|
||
string fixedName = null; | ||
if (!String.IsNullOrEmpty(name)) | ||
{ | ||
// Remove the '.dll' if it's there... | ||
fixedName = name.EndsWith(".dll", StringComparison.OrdinalIgnoreCase) | ||
? Path.GetFileNameWithoutExtension(name) | ||
: name; | ||
|
||
var assemblyString = Utils.IsPowerShellAssembly(fixedName) | ||
? Utils.GetPowerShellAssemblyStrongName(fixedName) | ||
: fixedName; | ||
|
||
try | ||
{ | ||
loadedAssembly = Assembly.Load(new AssemblyName(assemblyString)); | ||
} | ||
catch (FileNotFoundException fileNotFound) | ||
{ | ||
error = fileNotFound; | ||
} | ||
catch (FileLoadException fileLoadException) | ||
{ | ||
error = fileLoadException; | ||
// this is a legitimate error on CoreCLR for a newly emited with Add-Type assemblies | ||
// they cannot be loaded by name, but we are only interested in importing them by path | ||
} | ||
catch (BadImageFormatException badImage) | ||
{ | ||
error = badImage; | ||
return null; | ||
} | ||
catch (SecurityException securityException) | ||
{ | ||
error = securityException; | ||
return null; | ||
} | ||
} | ||
|
||
if (loadedAssembly != null) | ||
return loadedAssembly; | ||
|
||
if (!String.IsNullOrEmpty(filename)) | ||
{ | ||
error = null; | ||
|
||
try | ||
{ | ||
loadedAssembly = Assembly.LoadFrom(filename); | ||
return loadedAssembly; | ||
} | ||
catch (FileNotFoundException fileNotFound) | ||
{ | ||
error = fileNotFound; | ||
} | ||
catch (FileLoadException fileLoadException) | ||
{ | ||
error = fileLoadException; | ||
return null; | ||
} | ||
catch (BadImageFormatException badImage) | ||
{ | ||
error = badImage; | ||
return null; | ||
} | ||
catch (SecurityException securityException) | ||
{ | ||
error = securityException; | ||
return null; | ||
} | ||
} | ||
|
||
#if !CORECLR// Assembly.LoadWithPartialName is not in CoreCLR. In CoreCLR, 'LoadWithPartialName' can be replaced by Assembly.Load with the help of AssemblyLoadContext. | ||
// Finally try with partial name... | ||
if (!String.IsNullOrEmpty(fixedName)) | ||
{ | ||
try | ||
{ | ||
// This is a deprecated API, use of this API needs to be | ||
// reviewed periodically. | ||
#pragma warning disable 0618 | ||
loadedAssembly = Assembly.LoadWithPartialName(fixedName); | ||
|
||
if (loadedAssembly != null) | ||
{ | ||
// In the past, LoadWithPartialName would just return null in most cases when the assembly could not be found or loaded. | ||
// In addition to this, the error was always cleared. So now, clear the error variable only if the assembly was loaded. | ||
error = null; | ||
} | ||
return loadedAssembly; | ||
} | ||
|
||
// Expected exceptions are ArgumentNullException and BadImageFormatException. See https://msdn.microsoft.com/en-us/library/12xc5368(v=vs.110).aspx | ||
catch (BadImageFormatException badImage) | ||
{ | ||
error = badImage; | ||
} | ||
} | ||
#endif | ||
return null; | ||
// First we try to load the assembly based on the filename | ||
|
||
Assembly loadedAssembly = null; | ||
error = null; | ||
|
||
if (!String.IsNullOrEmpty(filename)) | ||
{ | ||
try | ||
{ | ||
loadedAssembly = Assembly.LoadFrom(filename); | ||
} | ||
catch (FileNotFoundException fileNotFound) | ||
{ | ||
error = fileNotFound; | ||
} | ||
catch (FileLoadException fileLoadException) | ||
{ | ||
error = fileLoadException; | ||
} | ||
catch (BadImageFormatException badImage) | ||
{ | ||
error = badImage; | ||
} | ||
catch (SecurityException securityException) | ||
{ | ||
error = securityException; | ||
} | ||
} | ||
else if (!String.IsNullOrEmpty(name)) | ||
{ | ||
string fixedName = null; | ||
// Remove the '.dll' if it's there... | ||
fixedName = name.EndsWith(".dll", StringComparison.OrdinalIgnoreCase) | ||
? Path.GetFileNameWithoutExtension(name) | ||
: name; | ||
|
||
var assemblyString = Utils.IsPowerShellAssembly(fixedName) | ||
? Utils.GetPowerShellAssemblyStrongName(fixedName) | ||
: fixedName; | ||
|
||
try | ||
{ | ||
loadedAssembly = Assembly.Load(new AssemblyName(assemblyString)); | ||
} | ||
catch (FileNotFoundException fileNotFound) | ||
{ | ||
error = fileNotFound; | ||
} | ||
catch (FileLoadException fileLoadException) | ||
{ | ||
error = fileLoadException; | ||
// this is a legitimate error on CoreCLR for a newly emited with Ad 8000 d-Type assemblies | ||
// they cannot be loaded by name, but we are only interested in importing them by path | ||
} | ||
catch (BadImageFormatException badImage) | ||
{ | ||
error = badImage; | ||
} | ||
catch (SecurityException securityException) | ||
{ | ||
error = securityException; | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Delete blank line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. resolved |
||
// We either return the loaded Assembly, or return null. | ||
return loadedAssembly; | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,3 +88,33 @@ Describe "Import-Module for Binary Modules in GAC" -Tags 'CI' { | |
} | ||
} | ||
} | ||
|
||
Describe "Import-Module for Binary Modules" -Tags 'CI' { | ||
|
||
It "PS should try to load the assembly from file path first" { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you sure this test validates the fix? It passes even without your fix. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. previously when system.automation.management is used loading error will be occured. |
||
$src = @" | ||
using System.Management.Automation; // Windows PowerShell namespace. | ||
|
||
namespace ModuleCmdlets | ||
{ | ||
[Cmdlet(VerbsDiagnostic.Test,"BinaryModuleCmdlet1")] | ||
public class TestBinaryModuleCmdlet1Command : Cmdlet | ||
{ | ||
protected override void BeginProcessing() | ||
{ | ||
WriteObject("BinaryModuleCmdlet1 exported by the ModuleCmdlets module."); | ||
} | ||
} | ||
} | ||
"@ | ||
|
||
Add-Type -TypeDefinition $src -OutputAssembly $TESTDRIVE\System.dll | ||
$results = powershell -noprofile -c "`$module = Import-Module $TESTDRIVE\System.dll -Passthru; `$module.ImplementingAssembly.Location; Test-BinaryModuleCmdlet1" | ||
|
||
#Ignore slash format difference under windows/Unix | ||
$path = (Get-ChildItem $TESTDRIVE\System.dll).FullName | ||
$results[0] | Should Be $path | ||
$results[1] | Should BeExactly "BinaryModuleCmdlet1 exported by the ModuleCmdlets module." | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we were given a filepath and it doesn't exist, it doesn't seem like we should try to load something that happens to have the same name but different path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved