Skip to content

Fix a module-loading regression that caused an infinite loop #6843

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.

Sign up for GitHub

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

Merged
daxian-dbw merged 3 commits into from
May 10, 2018
Merged

Fix a module-loading regression that caused an infinite loop #6843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ed60dc5
Fix a module-loading regression that caused an infinite loop
daxian-dbw May 8, 2018
0cdb35e
[Feature] Add test for the fix
daxian-dbw May 8, 2018
ef08299
[Feature] Move test to 'Microsoft.PowerShell.Core\Import-Module.Tests…
daxian-dbw May 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 12 additions & 27 deletions src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2820,7 +2820,7 @@ internal PSModuleInfo LoadModuleManifest(
{
bool found = false;
// Never load nested modules to the global scope.
bool oldGLobal = this.BaseGlobal;
bool oldGlobal = this.BaseGlobal;
this.BaseGlobal = false;

PSModuleInfo nestedModule = LoadModuleNamedInManifest(
Expand All @@ -2838,7 +2838,7 @@ internal PSModuleInfo LoadModuleManifest(
out found,
shortModuleName: null);

this.BaseGlobal = oldGLobal;
this.BaseGlobal = oldGlobal;

// If found, add it to the parent's list of NestedModules
if (found)
Expand Down Expand Up @@ -2976,30 +2976,16 @@ internal PSModuleInfo LoadModuleManifest(
BaseCmdletPatterns = oldCmdletPatterns;
}

// If there is an existing session state and the new module info
// session state is empty, then use the existing session state.
// This will be the case when ModuleToProcess is a binary module
// and there were nested modules.
if (newManifestInfo.SessionState == null && ss != null)
Copy link
Member Author

@daxian-dbw daxian-dbw May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

When it's not module analysis (get-module -list), ss will never be null. I verified that binary root module also uses ss as the session state. This is why this check is no longer needed.

{
newManifestInfo.SessionState = ss;
ss.Internal.Module = newManifestInfo;
}
// If the new module info session state is not empty but there is no
// existing session state, then use the new module info's sessions state;
// This will be the case if the module to process is a script module
// but there where no nested modules.
else if (newManifestInfo.SessionState != null && ss == null)
Copy link
Member Author

@daxian-dbw daxian-dbw May 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ss == null only if we are doing module analysis, but when we are doing module analysis, newManifestInfo.SessionState is always null, because we don't really load anything. This is why this check is no longer needed.

{
ss = newManifestInfo.SessionState;
}

// We don't need to care if they are both null which will be the
// case when there is only a binary module in ModuleToProcess and
// no nested modules. At this point the moduleInfo and the value in
// ss should be identical.
Dbg.Assert(newManifestInfo.SessionState == ss,
"ss and newManifestInfo.SessionState should be the same after handling ModuleToProcess");
// For most cases, 'newManifestInfo.SessionState' should be identical to 'ss':
// 1. when 'importingModule == true', 'newManifestInfo' uses the same session state as 'ss' because we passed in 'ss' when loading the RootModule via 'LoadModuleNamedInManifest'.
// 2. when 'importingModule == false', both session states will be null since we are in module analysis mode (Get-Module -ListAvailable).
//
// However, there is one exception when the RootModule is also put in NestedModules in the module manifest (ill-organized module structure).
// For example, module folder 'test' contains two files: 'test.psd1' and 'test.psm1', and 'test.psd1' has the following content:
// "@{ ModuleVersion = '0.0.1'; RootModule = 'test'; NestedModules = @('test') }"
//
// In that case, the nested module will first be loaded with a different session state, and then when trying to load the RootModule via 'LoadModuleNamedInManifest',
// the same loaded nested module will be reused for the RootModule by 'LoadModuleNamedInManifest'.

// Change the module name to match the manifest name, not the original name
newManifestInfo.SetName(manifestInfo.Name);
Expand Down Expand Up @@ -3248,7 +3234,6 @@ internal PSModuleInfo LoadModuleManifest(
{
if ((exportedCmdlets != null) && (ss != null))
{
Dbg.Assert(ss.Internal.ExportedCmdlets != null, "ss.Internal.ExportedCmdlets should not be null");
manifestInfo.ExportedCmdlets.Clear();

// Mark stuff for export
Expand Down
9 changes: 9 additions & 0 deletions src/System.Management.Automation/engine/Modules/PSModuleInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -577,6 +577,15 @@ public ReadOnlyDictionary<string, TypeDefinitionAst> GetExportedTypeDefinitions(
var res = new Dictionary<string, TypeDefinitionAst>(StringComparer.OrdinalIgnoreCase);
foreach (var nestedModule in this.NestedModules)
{
if (nestedModule == this)
{
// Circular nested modules could happen with ill-organized module structure.
// For example, module folder 'test' has two files: 'test.psd1' and 'test.psm1', and 'test.psd1' has the following content:
// "@{ ModuleVersion = '0.0.1'; RootModule = 'test'; NestedModules = @('test') }"
// Then, 'Import-Module test.psd1 -PassThru' will return a ModuleInfo object with circular nested modules.
continue;
Copy link
Collaborator

@iSazonov iSazonov May 9, 2018

Choose a reason for hiding this comment

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

Based on Issue which we fix I believe it make sense to inform users about the situation or even terminated the cmdlet.

Copy link
Member Author

@daxian-dbw daxian-dbw May 9, 2018

Choose a reason for hiding this comment

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

Turning something that works to an error is a hard breaking change. As for the warning, I'm not sure if we should do it, since technically import-module is doing what it's asked to do ... (have itself as the nested module).

Copy link
Collaborator

@iSazonov iSazonov May 9, 2018

Choose a reason for hiding this comment

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

Can ScriptAnalyzer catch this?

Copy link
Member Author

@daxian-dbw daxian-dbw May 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Good point! I think the script analyzer should be able to catch this kind of issue. That's the best place to throw a warning. I just checked in VS Code and the analyzer doesn't catch that now. I can open an issue at the script analyzer repo.

Copy link
Member Author

@daxian-dbw daxian-dbw May 10, 2018

Choose a reason for hiding this comment

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

Issue opened: PowerShell/PSScriptAnalyzer#997

}

foreach (var typePairs in nestedModule.GetExportedTypeDefinitions())
{
// The last one name wins! It's the same for command names in nested modules.
Expand Down
22 changes: 22 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,3 +241,25 @@ Describe "Workflow .Xaml module is not supported in PSCore" -tags "Feature" {
{ Import-Module $xamlNestedModule -ErrorAction Stop } | Should -Throw -ErrorId "Modules_WorkflowModuleNotSupported,Microsoft.PowerShell.Commands.ImportModuleCommand"
}
}

Describe "Circular nested module test" -tags "CI" {
BeforeAll {
$moduleFolder = Join-Path $TestDrive CircularNestedModuleTest
$psdPath = Join-Path $moduleFolder CircularNestedModuleTest.psd1
$psmPath = Join-Path $moduleFolder CircularNestedModuleTest.psm1

New-Item -Path $moduleFolder -ItemType Directory -Force > $null
Set-Content -Path $psdPath -Value "@{ ModuleVersion = '0.0.1'; RootModule = 'CircularNestedModuleTest'; NestedModules = @('CircularNestedModuleTest') }" -Encoding Ascii
Set-Content -Path $psmPath -Value "function bar {}" -Encoding Ascii
}

AfterAll {
Remove-Module -Name CircularNestedModuleTest -Force -ErrorAction SilentlyContinue
Remove-Item -Path $moduleFolder -Force -Recurse
}

It "Loading the module should succeed and return a module with circular nested module" {
$m = Import-Module $psdPath -PassThru
$m.NestedModules[0] | Should -Be $m
}
}