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

daxian-dbw · 2018-05-08T19:51:15Z

PR Summary

This regression was introduced by #6523, in PSModuleInfo.cs. A circular nested module check was removed because the comment there suggested it happens only with a deprecated workflow module. This causes a StackOverflow exception when running into circular nested modules.

Circular nested modules could happen for a module that is not well structured. For example, the 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') }

The same value test is put in both RootModule and NestedModule, which will end up with a module whose nested module points to itself.

There are two changes in this PR:

Add back the circular nested module check in PSModuleInfo.cs.
Remove a wrong Dbg.Assert in ModuleCmdletBase.cs and two checks before it.
- For the assertion Dbg.Assert(newManifestInfo.SessionState == ss, when facing the example above, the nested module will first be loaded with a different session state, and then when trying to load the root module, the same loaded nested module will be reused for it. So 'newManifestInfo.SessionState' is not ss. The assersion will fail in that case.
- For the two checks before the assersion, they are not needed anymore based on the comments there.

PR Checklist

daxian-dbw · 2018-05-08T19:54:17Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

-                // 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)


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.

daxian-dbw · 2018-05-08T19:55:38Z

src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs

-                // 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)


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.

rjmholt

These changes look good to me.

One question is: is there a possibility of a second-order nested module cycle?

Say A is a top level module, with nested module B, and B has in its nested modules A, then we'll still get caught in an infinite recursion? Unless that's not possible.

If it is possible, it seems like the only way to fix it is to carry a HashSet of module infos that we've seen with us down the recursive calls and ignore anything that's already in it. That would also prevent us reanalysing shared nested modules.

iSazonov · 2018-05-09T11:51:00Z

src/System.Management.Automation/engine/Modules/PSModuleInfo.cs

+                    // 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;


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

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).

Can ScriptAnalyzer catch this?

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.

Issue opened: PowerShell/PSScriptAnalyzer#997

iSazonov · 2018-05-09T11:57:43Z

test/powershell/engine/Module/Module.Basic.Tests.ps1

+# Copyright (c) Microsoft Corporation. All rights reserved.
+# Licensed under the MIT License.
+
+Describe "Module basic tests" -tags "CI" {


Maybe put the single test in test\powershell\Modules\Microsoft.PowerShell.Core\Import-Module.Tests.ps1?

OK, will do.

daxian-dbw · 2018-05-09T17:02:41Z

@rjmholt I tried out the scenario you mentioned above and powershell handles it (yay!):

PS:7> Import-Module mtest
Import-Module : Cannot load the module 'F:\tmp\dump\mtest\blah' because the module nesting limit has been exceeded. Modules can only be nested to 10 levels. Evaluate and change the order in which you are loading modules to prevent exceeding the nesting limit, and then try running your script again.
At line:1 char:1
+ Import-Module mtest
+ ~~~~~~~~~~~~~~~~~~~
+ CategoryInfo          : InvalidOperation: (F:\tmp\dump\mtest\blah:String) [Import-Module], InvalidOperationException
+ FullyQualifiedErrorId : Modules_ModuleTooDeeplyNested,Microsoft.PowerShell.Commands.ImportModuleCommand

….ps1'

Fix a module-loading regression that caused an infinite loop

ed60dc5

daxian-dbw requested review from BrucePay, adityapatwardhan, iSazonov and rjmholt May 8, 2018 19:51

daxian-dbw changed the title ~~Fix a module-loading regression that caused an infinite loop~~ [WIP] Fix a module-loading regression that caused an infinite loop May 8, 2018

daxian-dbw commented May 8, 2018

View reviewed changes

[Feature] Add test for the fix

0cdb35e

daxian-dbw changed the title ~~[WIP] Fix a module-loading regression that caused an infinite loop~~ Fix a module-loading regression that caused an infinite loop May 8, 2018

rjmholt approved these changes May 8, 2018

View reviewed changes

iSazonov reviewed May 9, 2018

View reviewed changes

[Feature] Move test to 'Microsoft.PowerShell.Core\Import-Module.Tests…

ef08299

….ps1'

iSazonov approved these changes May 9, 2018

View reviewed changes

daxian-dbw mentioned this pull request May 10, 2018

Check whether a module references itself in 'NestedModules' field in the .psd1 file PowerShell/PSScriptAnalyzer#997

Open

daxian-dbw merged commit db45785 into PowerShell:master May 10, 2018

daxian-dbw deleted the module branch May 10, 2018 23:09

alx9r mentioned this pull request May 20, 2020

This apparently acyclic combination of Import-Module calls results in stack overflow #12039

Closed

rjmholt mentioned this pull request Feb 23, 2021

Process is terminating due to StackOverflowException with custom provider on PSModulePath #5565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

daxian-dbw commented May 8, 2018 •

edited

Loading

Uh oh!

daxian-dbw May 8, 2018 •

edited

Loading

Uh oh!

daxian-dbw May 8, 2018 •

edited

Loading

Uh oh!

rjmholt left a comment •

edited

Loading

Uh oh!

iSazonov May 9, 2018

Uh oh!

daxian-dbw May 9, 2018

Uh oh!

iSazonov May 9, 2018

Uh oh!

daxian-dbw May 9, 2018 •

edited

Loading

Uh oh!

daxian-dbw May 10, 2018

Uh oh!

iSazonov May 9, 2018

Uh oh!

daxian-dbw May 9, 2018

Uh oh!

daxian-dbw commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

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

Uh oh!

Conversation

daxian-dbw commented May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

daxian-dbw May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw May 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iSazonov May 9, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw May 9, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov May 9, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw May 10, 2018

Choose a reason for hiding this comment

Uh oh!

iSazonov May 9, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw May 9, 2018

Choose a reason for hiding this comment

Uh oh!

daxian-dbw commented May 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

daxian-dbw commented May 8, 2018 •

edited

Loading

daxian-dbw May 8, 2018 •

edited

Loading

daxian-dbw May 8, 2018 •

edited

Loading

rjmholt left a comment •

edited

Loading

daxian-dbw May 9, 2018 •

edited

Loading