★ wanayoo — archive 1999 https://github.com/PowerShell/PowerShell/pull/8218Nouvelle recherche | Portail wanayoo
Skip to content

/PowerShell

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

Fix incorrect name check when autoloading required modules #8218

Merged
merged 41 commits into from Nov 13, 2018
+646 −17

Conversation

Reviewers

@daxian-dbw daxian-dbw

@BrucePay BrucePay

@TravisEz13 TravisEz13

@Francisco-Gamino Francisco-Gamino

@adityapatwardhan adityapatwardhan

Assignees

@adityapatwardhan adityapatwardhan

Labels
CL-Engine
Projects
None yet
Milestone
No milestone
5 participants
@rjmholt @bergmeister @adityapatwardhan @daxian-dbw @iSazonov
@rjmholt
Copy link
Member

commented Nov 9, 2018 •
edited

PR Summary

Fixes #8204.

The change I made in #7125 made the check too strict on the module we try to autoload it from requires and rejected it based on name.

This PR removes that name check but also sets up a fix for the more general problem that a ModuleSpecification can always specify a path rather than a name. I added some reusable logic to make this easier to service.

The main contribution is regression testing for RequiredModules, #requires -Modules and other cmdlets that can use ModuleSpecifications.

However, the general pervasive problem is that ModuleSpecifications contain raw user input and we don't do very much about handling paths that might be given as module names, namely:

  • Normalising directory separators
  • Resolving relative paths (which should be resolved differently depending on context)
  • Dealing with case-sensitivity
  • Handling trailing slashes
  • Matching on module directories accounting for the versioned directory structure
  • Working with special directory names like . and ..

The methods I've added deal with these and I've applied them in one place to help correct the immediate problem and make it more efficient.

But the fact remains that we have no way in the type system to identify when this resolution has already occurred; there are over 120 uses of the ModuleSpecification type in the code base, and we don't have a way to differentiate when they contain raw user input and when they can be trusted to have a canonical path. The way the module cmdlets are written currently, we redo a fair amount of this validation and normalisation logic, and it's probably somewhere we could find serious perf improvements.

PR Checklist

@rjmholt rjmholt requested review from BrucePay and daxian-dbw as code owners Nov 9, 2018

@iSazonov iSazonov requested review from adityapatwardhan and TravisEz13 Nov 9, 2018

@iSazonov iSazonov assigned adityapatwardhan Nov 9, 2018

@bergmeister

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 9, 2018

Thanks for handling the issue. I'd consider a regression test for this important feature to be essential.
@rjmholt

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 9, 2018

Agreed that it's essential -- I'm pretty surprised we didn't have one already.

I'm halfway through writing some already.

@rjmholt rjmholt force-pushed the rjmholt:fix-required-module-load-failure branch to ae5c56b Nov 9, 2018

@bergmeister

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Nov 9, 2018

Ok, feel free to add the test in a 2nd PR as I can understand that this is a time-pressing issue that can initially be verified with some manual testing (please include Windows as test platform though)
@rjmholt

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 9, 2018

We also seem to do required module autoloading in #requires pragmas:

PowerShell/src/System.Management.Automation/engine/CommandDiscovery.cs

Lines 307 to 313 in 4118fd2

ModuleCmdletBase.LoadRequiredModule(
context: context,
currentModule: null,
requiredModuleSpecification: requiredModule,
moduleManifestPath: null,
manifestProcessingFlags: ModuleCmdletBase.ManifestProcessingFlags.LoadElements | ModuleCmdletBase.ManifestProcessingFlags.WriteErrors,
error: out error);

If those are supposed to allow paths, I'll need to do the same path normalisation logic there.
@adityapatwardhan

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 9, 2018

@rjmholt 1 new test is failing on Linux and macOS. Please have a look.

@rjmholt rjmholt requested a review from Francisco-Gamino as a code owner Nov 9, 2018

@rjmholt

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Nov 12, 2018

The failing AppVeyor test is passing on my machine. Looking into it.
@daxian-dbw

daxian-dbw reviewed Nov 13, 2018
View changes

Show resolved Hide resolved src/System.Management.Automation/engine/Modules/GetModuleCommand.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleSpecification.cs
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated
src/System.Management.Automation/engine/Modules/RemoveModuleCommand.cs
// Paths in the module name may fail here because
// they the wrong directory separator or are relative.
// Fix with the code below:
// FullyQualifiedName = FullyQualifiedName.Select(ms => ms.WithNormalizedName(Context, SessionState.Path.CurrentLocation.Path)).ToArray();

This comment has been minimized.

Sign in to view
Copy link
@daxian-dbw

daxian-dbw Nov 13, 2018

Member

What's the concern about putting this fix in place? Regression? Perf degradation?

This comment has been minimized.

Sign in to view
Copy link
@rjmholt

rjmholt Nov 13, 2018

Author Member

Just keeping the fix constrained in this PR. It shouldn't cause regressions or perf degradations, but to ensure there are no regressions (since this PR is a regression fix itself) I just left it as a comment.

This comment has been minimized.

Sign in to view
Copy link
@daxian-dbw

daxian-dbw Nov 13, 2018

Member

Will you open issue to track all those todo comments? They can be easily forgotten :)

This comment has been minimized.

Sign in to view
Copy link
@daxian-dbw

daxian-dbw Nov 13, 2018

Member

Did you open an issue to track those todo's?

This comment has been minimized.

Sign in to view
Copy link
@rjmholt

rjmholt Nov 13, 2018

Author Member

Next on my list
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleCmdletBase.cs Outdated

rjmholt added some commits Nov 13, 2018

@rjmholt
Make required name null check clearer
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
5950e75
@rjmholt
Protect against path resolution failure
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
cbc650e
@rjmholt
Fix Path.AltDirectorySeparatorChar misuse
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
87a9704
@rjmholt
[Feature] Fix non-const const assignment
Loading status checks…
2378bb8
@rjmholt
[Feature] Undo change to ModuleCmdletBase.IsRooted
Loading status checks…
9faf21e
@daxian-dbw

daxian-dbw reviewed Nov 13, 2018
View changes

Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated
Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated
@daxian-dbw

daxian-dbw reviewed Nov 13, 2018
View changes

Show resolved Hide resolved src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated
src/System.Management.Automation/engine/Modules/RemoveModuleCommand.cs
// Paths in the module name may fail here because
// they the wrong directory separator or are relative.
// Fix with the code below:
// FullyQualifiedName = FullyQualifiedName.Select(ms => ms.WithNormalizedName(Context, SessionState.Path.CurrentLocation.Path)).ToArray();

This comment has been minimized.

Sign in to view
Copy link
@daxian-dbw

daxian-dbw Nov 13, 2018

Member

Did you open an issue to track those todo's?
@rjmholt
[Feature] Fix comment typo
Loading status checks…
389b245
@daxian-dbw

daxian-dbw approved these changes Nov 13, 2018
View changes

Copy link
Member

left a comment •
edited

Only pending action item left is to open an issue tracking all "TODO" comments.
@adityapatwardhan

This comment has been minimized.

Sign in to view
Copy link
Member

commented Nov 13, 2018

Since the CodeFactor issues are for existing methods, re-ordering will cause larger diffs. We can ignore CodeFactor issues for this PR.

@adityapatwardhan adityapatwardhan merged commit aa76c68 into PowerShell:master Nov 13, 2018
5 of 6 checks passed

5 of 6 checks passed

CodeFactor 3 issues found.
Details
PowerShell-CI-linux #PR-8218-20181113.09 succeeded
Details
PowerShell-CI-macos #PR-8218-20181113.09 succeeded
Details
PowerShell-CI-windows #PR-8218-20181113.09 succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details

This was referenced Nov 13, 2018

Open
Open

@bergmeister bergmeister referenced this pull request Nov 14, 2018

Closed

RequiredModules manifest property stopped working recently (noticed in daily build) #8204

@iSazonov iSazonov added the CL-Engine label Jan 17, 2019

@rjmholt rjmholt referenced this pull request Feb 21, 2019

Open

Refactor ImportModule commandlet #8936

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.