Skip to content

Conversation

@stevenebutler
Copy link
Contributor

@stevenebutler stevenebutler commented Jul 11, 2023

Closes #19875

When MyDocuments folder is blank (e.g. when network drive containing folder is disconnected, or running under local system account) PowerShell crashes on startup. This PR adds some null/blank path checking to strings before they are added to
the module path.

PR Context

PR Checklist

@stevenebutler stevenebutler changed the title Guard against null when adding path components Guard against null or blank path components when adding to module path Jul 11, 2023
@StevenBucher98 StevenBucher98 added the PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update label Jul 17, 2023
Copy link
Collaborator

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

please add some validation for this new behavior

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 17, 2023
@stevenebutler
Copy link
Contributor Author

please add some validation for this new behavior

@JamesWTruher - the bug was caused by unexpected return data from this API call in ModuleIntrinsics.cs

string myDocumentsPath = Environment.GetFolderPath(Environment.SpecialFolder.MyDocuments);

To add tests for this in the build we'd either have to have the ability to run under an account where it returns "" or provide some way to simulate the API returning the empty string (e.g. through a layer of indirection controlled by an environment variable). Is this something you'd like to see, or if you have an alternative approach please let me know.

The easiest way I've found to test the impact of this change manually is to use psexec from SysInternals to launch a System account (elevated) command prompt (which returns "" for the API call above). See issue #19875 comments for some screenshots from that testing.

psexec -i -s cmd.exe

Then in the system account elevated command prompt start the pwsh executable and observe:

  • pwsh-7.4-preview 4 crashes with null pointer exception when starting
  • with this PR it starts successfully

I have also tested it manually in the scenario where a domain joined PC with network located MyDocuments is disconnected from the domain network and confirmed it no longer crashes in that scenario either.

I'm not familiar enough with the build environment to know how such a test could be orchestrated. The only alternative would be to use some interfaces and create some C# unit tests but I haven't seen any unit test projects in the source base so don't know if this would be an acceptable approach here.

@microsoft-github-policy-service microsoft-github-policy-service bot removed the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 18, 2023
@iSazonov
Copy link
Collaborator

iSazonov commented Jul 18, 2023

@stevenebutler Easiest way is to use a test hook. See

public static class InternalTestHooks

[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('NoPromptForPassword', $true)

@stevenebutler
Copy link
Contributor Author

@stevenebutler Easiest way is to use a test hook. See

public static class InternalTestHooks

[System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('NoPromptForPassword', $true)

The issue is the test hook would have to be configured before powershell starts up to trigger the bug because this crash happens long before you get an interactive shell or can run a script. It happens while PowerShell itself is initializing.

Maybe we can start up PowerShell then tweak the test hook and call into the method that crashes in the module intrinsics code and possibly trigger the same exception afterPowerShell has started. I'll look into that possibility.

@iSazonov
Copy link
Collaborator

The issue is the test hook would have to be configured before powershell starts up to trigger the bug because this crash happens long before you get an interactive shell or can run a script. It happens while PowerShell itself is initializing.

We could use specific/hidden env variable like __SuppressAnsiEscapeSequences or __PWSH_LOGIN_CHECKED for turning on the test hook.

@pull-request-quantifier-deprecated

This PR has 30 quantified lines of changes. In general, a change size of upto 200 lines is ideal for the best PR experience!


Quantification details

Label      : Extra Small
Size       : +21 -9
Percentile : 12%

Total files changed: 3

Change summary by file extension:
.cs : +12 -9
.ps1 : +9 -0

Change counts above are quantified counts, based on the PullRequestQuantifier customizations.

Why proper sizing of changes matters

Optimal pull request sizes drive a better predictable PR flow as they strike a
balance between between PR complexity and PR review overhead. PRs within the
optimal size (typical small, or medium sized PRs) mean:

  • Fast and predictable releases to production:
    • Optimal size changes are more likely to be reviewed faster with fewer
      iterations.
    • Similarity in low PR complexity drives similar review times.
  • Review quality is likely higher as complexity is lower:
    • Bugs are more likely to be detected.
    • Code inconsistencies are more likely to be detected.
  • Knowledge sharing is improved within the participants:
    • Small portions can be assimilated better.
  • Better engineering practices are exercised:
    • Solving big problems by dividing them in well contained, smaller problems.
    • Exercising separation of concerns within the code changes.

What can I do to optimize my changes

  • Use the PullRequestQuantifier to quantify your PR accurately
    • Create a context profile for your repo using the context generator
    • Exclude files that are not necessary to be reviewed or do not increase the review complexity. Example: Autogenerated code, docs, project IDE setting files, binaries, etc. Check out the Excluded section from your prquantifier.yaml context profile.
    • Understand your typical change complexity, drive towards the desired complexity by adjusting the label mapping in your prquantifier.yaml context profile.
    • Only use the labels that matter to you, see context specification to customize your prquantifier.yaml context profile.
  • Change your engineering behaviors
    • For PRs that fall outside of the desired spectrum, review the details and check if:
      • Your PR could be split in smaller, self-contained PRs instead
      • Your PR only solves one particular issue. (For example, don't refactor and code new features in the same PR).

How to interpret the change counts in git diff output

  • One line was added: +1 -0
  • One line was deleted: +0 -1
  • One line was modified: +1 -1 (git diff doesn't know about modified, it will
    interpret that line like one addition plus one deletion)
  • Change percentiles: Change characteristics (addition, deletion, modification)
    of this PR in relation to all other PRs within the repository.


Was this comment helpful? 👍  :ok_hand:  :thumbsdown: (Email)
Customize PullRequestQuantifier for this repository.

@stevenebutler
Copy link
Contributor Author

It's not necessary to use env vars as the code that was failing can be called directly after startup with the test hook set to fake an empty MyDocuments location.

I've added this test, which failed without the patch.

      try {
            [System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('SetMyDocumentsSpecialFolderToBlank', $true)
            [System.Management.Automation.ModuleIntrinsics]::GetModulePath($null, $null, $null)
        }
        finally {
            [System.Management.Automation.Internal.InternalTestHooks]::SetTestHook('SetMyDocumentsSpecialFolderToBlank', $false)
        }

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

blocking for security review

@microsoft-github-policy-service microsoft-github-policy-service bot added the Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept label Jul 24, 2023
@TravisEz13 TravisEz13 added the WG-Security security related areas such as JEA label Jul 24, 2023
Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

I think it's good.

Copy link
Member

@TravisEz13 TravisEz13 left a comment

Choose a reason for hiding this comment

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

Reviewed with WG-Security and we don't have any concerns

@daxian-dbw
Copy link
Member

@JamesWTruher Can you please take another look and update your review? Thanks!

Copy link
Collaborator

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

thanks!

@daxian-dbw daxian-dbw merged commit 3710671 into PowerShell:master Jul 31, 2023
@daxian-dbw daxian-dbw added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed Waiting on Author The PR was reviewed and requires changes or comments from the author before being accept labels Jul 31, 2023
@daxian-dbw daxian-dbw added CL-Engine Indicates that a PR should be marked as an engine change in the Change Log and removed CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels Aug 1, 2023
@stevenebutler stevenebutler deleted the 19875-prevent-startup-crash-when-personalModulePath-is-blank-or-null branch January 4, 2024 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-Engine Indicates that a PR should be marked as an engine change in the Change Log Extra Small PowerShell-Docs not needed The PR was reviewed and doesn't appear to require a PowerShell Docs update WG-Security security related areas such as JEA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PowerShell 7.4.0-preview.4 fails to start on Windows PC when disconnected from domain network

8 participants