Rework PSModulePath path building #24568

jborean93 · 2024-11-12T02:38:35Z

PR Summary

Fixes the parsing of the powershell.config.json PSModulePath entry when it contains the path separator char. Instead of crashing this is able to handle entries with those characters and treat them as multiple entries.

This also rewrites the logic when building the process PSModulePath to
simplify the way entries were added and deal with canonicalisation of
the path entries.

Fixes: #20706

PR Context

Using powershell.config.json with a PSModulePath entry that contains the Path.Separator char (; for Windows and : for *nix) crashes the process on startup. This is because the current logic adds the value provided as is to the path but then attempts to find the index for where it is located. This lookup splits the new path value by the Path.Separator so the entry being added is never found (the raw value is never a split component of the new path).

This new PR consolidates the various path addition logic and tries to make it more consistent in how it handles new values. It also attempts to remove as much string allocations by working on spans where possible to hopefully improve the memory allocation at startup.

Ultimately the behaviour of this logic when building the PSModulePath is

Add the current user path
- Uses the current user .config.json and falls back to the default user's modules dir
Add the shared machine wide path
- On Windows this is C:\Program Files\PowerShell\Modules
- This not controlled by any config
Add the machine/system path
- Uses the $PSHome/powershell.config.json entry and falls back to $PSHome/Modules
Adds the existing process PSModulePath if present
- On Windows this is typically the machine wide PSModulePath (System32/Program Files\WindowsPowerShell)
- Will inherit from the parent process that spawned PowerShell
- On Linux this is not set by default but can be using whatever shell that is spawning PowerShell

There are a few behavioural changes introduced by this PR which I think aren't breaking changes but are behaviour differences for the better.

Paths are canonicalised and only added if not already present
- Whitespace is trimmed
- Trailing / or \ (on Windows) is removed
- On Windows / is converted to \
- All duplicates and empty entries are removed
- In the past some paths were checked before adding while others are just appended as is leading to mixed behaviour
- This new logic is consistent with all stages of building the PSModulePath
On non-Windows paths are compared in a case sensitive way
- This is because these OS’ are typically run on a filesystem where the case does matter
When PSModulePath is not set as a process env var (Linux mostly) and there is an explicit PSModulePath in the $PSHome/powershell.config.json the shared module path is now included
- This was omitted before but I cannot understand why
- Making this change keeps things consistent
- Should be a rare scenario in any case which is probably why it was never picked up

PR Checklist

jborean93 · 2024-11-12T04:29:54Z

Tests work locally on my macOS host so I need to figure out what the differences are in CI to cause it to fail.

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

jborean93 · 2024-11-13T07:25:35Z

I've pushed a commit that removed the existing logic and reimplemented it in what I hope is a more sane manner. I'll have to sit down and write some more comprehensive tests for this API to test out edge cases like cases, inserting the system/shared path earlier and so on. I'll also need to look into things like trailing backslash and whitespace handling as I think before this was considered equal when compared.

The only change that I can tell this makes is that the shared path C:\Program Files\PowerShell\Modules is added all the time whereas before it was skipped if the PSModulePath env var was unset and there was an explicit machine env var path. I think the change is better consistency wise but will have to think about it some more.

iSazonov · 2024-11-13T08:53:02Z

As for tests, I'd consider xUnit tests for GetModulePath(). It saves a lot your time and makes design intentions explicit.

jborean93 · 2024-11-21T09:14:55Z

I've updated the PR title and summary to sum up the changes I've made here. The tests are passing except for the CodeFactor checks which is complaining about the lack of docs on the new unit tests added. No other tests have the docs so I'm just ignoring this failure.

This is now ready for review.

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

iSazonov · 2024-11-21T12:25:46Z

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

                {
-                    currentProcessModulePath += Path.PathSeparator;
+                    entry = entry.TrimEnd(dirSeparatorChars);


It seems there is very optimized overload for two arguments so we can eliminate dirSeparatorChars.

Fair point I’ll use the params overload which I think is the same one used but avoids the span allocation.

Please see my comment about TrimEndingDirectorySeparator()

Ah there reason why I did this is because the params overload is for a string, we are operating on a ReadOnlySpan and MemoryExtensions.TrimEnd doesn't have such an overload.

I've kept the current code because TrimEndingDirectorySeparator() doesn't handle trimming more than 1 trailing separator.

I think it makes no sense for us to try to fix user errors. In fact, there may be a lot of user errors that we don't fix. I suggest following the same logic as you suggested for duplicated paths - just ignore it.
The main thing for us here is to correctly add the necessary paths and no more.

I suggest to use more simple code with TrimEndingDirectorySeparator() here. It is well tested (I see one issue in your code for Windows).

I suggest to use more simple code with TrimEndingDirectorySeparator() here. It is well tested (I see one issue in your code for Windows)

What's the issue for Windows?

I'll wait until the WG weighs in to see whether they prefer the current code that trims all trailing dir path separators or whether to swap to TrimEndingDirectorySeparator which only trims the last.

// On Windows we always want to trim '/' or '\'. Only exception // is to keep one trailing char if the path is the drive root // 'C:\+'. entry = entry.Length > 1 && entry[1] == Path.VolumeSeparatorChar && trimmedEntry.Length == 2 ? entry.Slice(0, 3) : trimmedEntry;

The code is not correct.

You will wait indefinitely.
I suggest making the code as simple as possible and accepting it now.
Since we are talking about an extreme case, you can create an issue for it and make changes to the code after additional discussion if it really turns out to be useful for many users.

You will wait indefinitely.

I'm assuming I'm waiting indefinitely to get this reviewed by the WG anyway :) I appreciate you reviewing these PRs though and I'm not trying to be obstructionist it's just if they have to review this anyway I prefer to wait for the final verdict before changing things.

I have picked up the bug if it's C: then the Slice will go out of bounds and will fix up. Not sure if that's the incorrect part you are referring to though.

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

iSazonov · 2024-11-21T17:03:16Z

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

                }
+#if UNIX
+                // Don't trim if it's just '/' as that's a valid entry on *nix.
+                else if (!(entry.Length == 1 && entry[0] == Path.DirectorySeparatorChar))


Can we use [IO.Path.IsPathRooted](https://source.dot.net/#System.Private.CoreLib/src/libraries/System.Private.CoreLib/src/System/IO/Path.Unix.cs,132)?

And what about C:\ on Windows?

Also there is TrimEndingDirectorySeparator() - it checks for size and root.

One downside of TrimEndingDirectorySeparator is that it seems like it only trims 1 char versus TrimEnd which trims them all if present. I don't think someone having C:\foo\\ would be common but it depends if we want to handle it or not (I think we should).

I've updated the code to handle Windows absolute paths and their trailing chars and expanded the comments that explains what it does. I didn't use TrimEndingDirectorySeparator() because it will only trim 1 char at the end.

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

Fixes the parsing of the powershell.config.json PSModulePath entry when it contains the path separator char. Instead of crashing this is able to handle entries with those characters and treat them as multiple entries. This also rewrites the logic when building the process PSModulePath to simplify the way entries were added and deal with canonicalisation of the path entries.

jborean93 mentioned this pull request Nov 12, 2024

Error starting PowerShell 7.4.0+ #20706

Open

5 tasks

jborean93 force-pushed the config-modulepath branch from 11ffe1d to 231f0bd Compare November 12, 2024 04:10

iSazonov reviewed Nov 12, 2024

View reviewed changes

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated Show resolved Hide resolved

jborean93 changed the title ~~Fix config.json PSModulePath path with sep char~~ WIP - Fix config.json PSModulePath path with sep char Nov 13, 2024

jborean93 requested review from daxian-dbw, TravisEz13, adityapatwardhan and anmenaga as code owners November 21, 2024 05:00

jborean93 force-pushed the config-modulepath branch 3 times, most recently from 2ee70e7 to cbd1fd7 Compare November 21, 2024 09:03

jborean93 changed the title ~~WIP - Fix config.json PSModulePath path with sep char~~ Rework PSModulePath path building Nov 21, 2024

iSazonov reviewed Nov 21, 2024

View reviewed changes

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated Show resolved Hide resolved

iSazonov reviewed Nov 22, 2024

View reviewed changes

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Outdated Show resolved Hide resolved

xtqqczze mentioned this pull request Nov 24, 2024

Set LangVersion compiler option to 13.0 #24617

Merged

jborean93 force-pushed the config-modulepath branch from 5554bb1 to e46c27b Compare November 26, 2024 05:54

jborean93 added 4 commits November 27, 2024 03:25

Better handle path roots and improve path canonicalisation on Windows

7c3a15b

More special handling for Windows drive root entries

102d59a

Use simpler string creation code now that LangVersion is updated

9019fe4

jborean93 force-pushed the config-modulepath branch from e46c27b to 9019fe4 Compare November 26, 2024 17:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework PSModulePath path building #24568

Rework PSModulePath path building #24568

jborean93 commented Nov 12, 2024 •

edited

Loading

jborean93 commented Nov 12, 2024

jborean93 commented Nov 13, 2024 •

edited

Loading

iSazonov commented Nov 13, 2024

jborean93 commented Nov 21, 2024

iSazonov Nov 21, 2024 •

edited

Loading

jborean93 Nov 21, 2024

iSazonov Nov 21, 2024

jborean93 Nov 21, 2024 •

edited

Loading

iSazonov Nov 22, 2024

jborean93 Nov 24, 2024

iSazonov Nov 26, 2024

jborean93 Nov 26, 2024

iSazonov Nov 21, 2024 •

edited

Loading

jborean93 Nov 21, 2024 •

edited

Loading

jborean93 Nov 21, 2024

Rework PSModulePath path building #24568

Are you sure you want to change the base?

Rework PSModulePath path building #24568

Conversation

jborean93 commented Nov 12, 2024 • edited Loading

PR Summary

PR Context

PR Checklist

jborean93 commented Nov 12, 2024

jborean93 commented Nov 13, 2024 • edited Loading

iSazonov commented Nov 13, 2024

jborean93 commented Nov 21, 2024

iSazonov Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

jborean93 Nov 21, 2024

Choose a reason for hiding this comment

iSazonov Nov 21, 2024

Choose a reason for hiding this comment

jborean93 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

iSazonov Nov 22, 2024

Choose a reason for hiding this comment

jborean93 Nov 24, 2024

Choose a reason for hiding this comment

iSazonov Nov 26, 2024

Choose a reason for hiding this comment

jborean93 Nov 26, 2024

Choose a reason for hiding this comment

iSazonov Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

jborean93 Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

jborean93 Nov 21, 2024

Choose a reason for hiding this comment

jborean93 commented Nov 12, 2024 •

edited

Loading

jborean93 commented Nov 13, 2024 •

edited

Loading

iSazonov Nov 21, 2024 •

edited

Loading

jborean93 Nov 21, 2024 •

edited

Loading

iSazonov Nov 21, 2024 •

edited

Loading

jborean93 Nov 21, 2024 •

edited

Loading