Skip to content

Start-Process: Correct Unspecified WorkingDirectory Resolution - #11946

Merged
Dongbo Wang (daxian-dbw) merged 3 commits into
PowerShell:masterfrom
NoMoreFood:process_literalpath
Jun 9, 2020
Merged

Start-Process: Correct Unspecified WorkingDirectory Resolution#11946
Dongbo Wang (daxian-dbw) merged 3 commits into
PowerShell:masterfrom
NoMoreFood:process_literalpath

Conversation

@NoMoreFood

@NoMoreFood Bryan Berns (NoMoreFood) commented Feb 25, 2020

Copy link
Copy Markdown
Contributor

PR Summary

Modified Start-Process such that an unspecified working directory is correctly treated as literal when being resolved.

Fix #11940

PR Context

Problem was discovered when investigating an error that resulted from executing Start-Process in a directory that contained wildcard characters.

PR Checklist

@daxian-dbw

Copy link
Copy Markdown
Member

Although it might be a grey area, this is a breaking change as it changes how the arguments passed to WorkingDirectory, RedirectStandardError, RedirectStandardInput, and RedirectStandardOutput get resolved. Need the committee to weigh in.

@daxian-dbw Dongbo Wang (daxian-dbw) added Breaking-Change breaking change that may affect users Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Mar 13, 2020
@SteveL-MSFT

Steve Lee (SteveL-MSFT) commented Mar 18, 2020

Copy link
Copy Markdown
Member

@PowerShell/powershell-committee reviewed this, we propose that instead of always treating it as a literal, we would check if the path exists as a literal use it, otherwise fallback to the current behavior

@SteveL-MSFT Steve Lee (SteveL-MSFT) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Mar 18, 2020
@vexx32

Rain Sallow (/u/ta11ow) (vexx32) commented Mar 18, 2020

Copy link
Copy Markdown
Collaborator

How?

Working Directory by definition cannot be a useful wildcarded path, which can usually be assumed to exist. If you have an incomplete path, you can tab complete the rest. If you really need wildcards for any of these, you have Get-Item available.

Redirection paths typically won't exist for most use cases (again, no use for wildcards if the path doesn't exist), but as I understand it the cmdlet isn't capable of writing to multiple paths anyway, can it?

@NoMoreFood

Copy link
Copy Markdown
Contributor Author

Rain Sallow (/u/ta11ow) (@vexx32) Agreed. In fact, when I force the command to match multiple matches for output redirection, the command says: Start-Process: Cannot perform operation because the path resolved to more than one file. This command cannot operate on multiple files.

Steve Lee (@SteveL-MSFT) Certainly can't say that makes a lot of sense to me, but I can adjust the code if that's the verdict. Should the documentation be updated to say it "Sort Of" supports wildcards? Thoughts on redirection parameters as Rain Sallow (/u/ta11ow) (@vexx32) mentions?

@daxian-dbw

Copy link
Copy Markdown
Member

Redirection paths typically won't exist for most use cases

I agree that in most use cases the redirection files for standard output/error don't exist beforehand. But -RedirectStandardInput by definition requires an existing file.
Even for -RedirectStandardError and -RedirectStandardOutput, it's hard to say the current wildcard behavior is not being used at all. I can imagine a CI system where a new log file is created every day in the format ci-log-<date>.txt and output/error of a process is supposed to be written to that log file. For this hypothetical scenario, it's natural to use a wildcard like ci-log-*.txt for those 2 parameters (only one file, which is created on that day, will be matched). I think that's why the committee decide to suggest a more conservative (less breaking) behavior.

Working Directory by definition cannot be a useful wildcarded path, which can usually be assumed to exist.

One can create two directories named [a]b and ab. [a]b is a valid wildcard that matches ab. Again, I think the committee's suggestion is more conservative (less breaking).

@iSazonov

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@daxian-dbw) Based on your examples I can conclude that we should first make wildcard check and then literal.

@SteveL-MSFT

Copy link
Copy Markdown
Member

Ilya (@iSazonov) that's a valid point and would seem to be less breaking as any existing usage expecting wildcard matching would continue to work

@SteveL-MSFT Steve Lee (SteveL-MSFT) added Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Mar 31, 2020
- Modified Start-Process such that an unspecified working directory is correctly treated as literal when being resolved.
- Added test for running Start-Process within a directory with with wildcard characters.
@NoMoreFood

Copy link
Copy Markdown
Contributor Author

Ilya (@iSazonov) Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) Rain Sallow (/u/ta11ow) (@vexx32) It appears that PathUtils.ResolvePath() is not positioned well to do the sort of trial-and-error that's discussed. Instead of biting off that task right now, what I've done adjusted this PR's scope to address the originally reported behavior that was causing Start-Process to fail when WorkingDirectory is not specified but the current directory is set to a path that contains wildcard-type characters. (e.g. Set-Location 'C:\Path[]'; Start-Process cmd.exe was failing). I believe this change is non-breaking and should be non-controversial.

@NoMoreFood Bryan Berns (NoMoreFood) changed the title Start-Process: Treat Working Directory / Redirection Paths As Literal Start-Process: Correct Unspecified WorkingDirectory Resolution Apr 3, 2020
@JamesWTruher

Copy link
Copy Markdown
Collaborator

I think there are additional problems here and definitely some holes in our testing. As the following shows:

Describe "Should be able to set location to name with wild card" {
    BeforeAll {
        $testCases = @{ Name = 'foo[' },@{ Name = 'foo]' },@{ Name = 'foo[]' }, @{ Name = 'foo?' }, @{ Name = 'foo[?]' },
            @{ Name = 'foo[*]' },@{ Name = 'foo[*' },@{ Name = 'foo*]' }
    }

    It "Should be able to create and set the location to <Name>" -TestCases $testCases {
        param ( $Name )

        if ( $Name -match "\*" -and $IsWindows ) {
            Set-ItResult -Skipped -Because "'$Name' is not supported as directory name on Windows"
            return
        }
        
        $item = New-Item -Type Directory -Path ${TESTDRIVE}/$Name
        $item.Name | Should -Be $Name
        
        $i = Get-Item -LiteralPath ${TESTDRIVE}/$Name
        $i.Name | Should -Be $Name

        $location = Get-Location
        try {
            Set-Location -LiteralPath ${TESTDRIVE}/$Name
            $sName = [System.IO.Path]::GetFileName((Get-Location ).Path)
            $sName | Should -Be $Name
        }
        catch {
            throw "Set-Location $Name Failed"
        }
        finally { 
            Set-Location -LiteralPath $location.Path
        }
    }
}

when run on my Mac, I get the following result

Describing Should be able to set location to name with wild card
  [+] Should be able to create and set the location to foo[ 36ms
  [+] Should be able to create and set the location to foo] 13ms
  [+] Should be able to create and set the location to foo[] 26ms
  [+] Should be able to create and set the location to foo? 28ms
  [+] Should be able to create and set the location to foo[?] 41ms
  [-] Should be able to create and set the location to foo[*] 56ms
    Expected strings to be the same, but they were different.
    Expected length: 6
    Actual length:   5
    Strings differ at index 4.
    Expected: 'foo[*]'
    But was:  'foo[]'
    18:         $i.Name | Should -Be $Name
    at <ScriptBlock>, /tmp/zap/tt.tests.ps1: line 18
  [-] Should be able to create and set the location to foo[* 17ms
    Expected strings to be the same, but they were different.
    String lengths are both 5.
    Strings differ at index 4.
    Expected: 'foo[*'
    But was:  'foo[]'
    18:         $i.Name | Should -Be $Name
    at <ScriptBlock>, /tmp/zap/tt.tests.ps1: line 18
  [-] Should be able to create and set the location to foo*] 20ms
    Expected strings to be the same, but they were different.
    String lengths are both 5.
    Strings differ at index 3.
    Expected: 'foo*]'
    But was:  'foo[]'
    18:         $i.Name | Should -Be $Name
    at <ScriptBlock>, /tmp/zap/tt.tests.ps1: line 18

The files present in $TESTDRIVE are:

PS> ls $TESTDRIVE |ft -auto

    Directory: /private/tmp/ea03bb1e-4ae5-46b7-af36-6341e4015b20

UnixMode   User  Group  LastWriteTime Size Name
--------   ----  -----  ------------- ---- ----
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo?
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo[
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo[?]
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo[]
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo[*
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo[*]
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo]
drwxr-xr-x james wheel 4/8/2020 13:27   64 foo*]

@NoMoreFood

Copy link
Copy Markdown
Contributor Author

James Truher (@JamesWTruher) Yeah, I suspected that there are going to be these sort of wildcard processing issues I located in Start-Process in other cmdlets as well. Unless you've already dove in, I'll look into the behavior you noted as a separate PR (unless you think it's somehow related to this specific fix for Start-Process).

Note, for Windows your match should be more like $Name -match '[<>:"/\|?*]' -and $IsWindows since none of these characters are supported. When these illegal characters are eliminated, it passes on Windows but only the three test cases are executed. I'll try on MacOS later.

@iSazonov

Copy link
Copy Markdown
Collaborator

The more I think about wildcards in the scenario the less I like it. I'd prefer literal paths - it is unnatural behavior to expand wildcards for these paths. I can’t imagine that someone selects a working directory by wildcard, and especially a redirect file. It is unreliable and unpredictable.

@NoMoreFood

Copy link
Copy Markdown
Contributor Author

James Truher (@JamesWTruher) Reproduced your Mac results. Set-Location -LiteralPath definitely doesn't handle processing '*' characters very well whereas the native shells do just fine. I do believe this is a separate issue though from the one this PR addresses though. Will look at the underlying code tomorrow to verify...

@NoMoreFood

Bryan Berns (NoMoreFood) commented Apr 9, 2020

Copy link
Copy Markdown
Contributor Author

James Truher (@JamesWTruher) I confirmed the behavior you reported is a different issue and is related to the code called within Set-Location that's responsible for case normalization. I have a potential fix and will submit that separately. Please proceed with adjudicating this PR as-is.

Update: Noted behavior is corrected in #12294. This includes an enhanced version of your test script.

@SteveL-MSFT

Copy link
Copy Markdown
Member

@PowerShell/powershell-committee reviewed this, we agree that an implicit WorkingDirectory should be literal and never glob, so the targeted fix is fine for this PR (not a comment on the code itself but the intent).

@SteveL-MSFT Steve Lee (SteveL-MSFT) added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Apr 15, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label May 27, 2020
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

…w Changes

- Added named argument to ResolveFilePath() call for clarity.
@iSazonov Ilya (iSazonov) added CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log labels May 29, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

Dongbo Wang (@daxian-dbw) I think the PR is ready to merge.

@ghost ghost removed the Review - Needed The PR is being reviewed label May 29, 2020
@ghost ghost added the Review - Needed The PR is being reviewed label Jun 5, 2020
@ghost

Copy link
Copy Markdown

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Mainainer, Please provide feedback and/or mark it as Waiting on Author

@daxian-dbw Dongbo Wang (daxian-dbw) removed the Review - Needed The PR is being reviewed label Jun 8, 2020
@daxian-dbw
Dongbo Wang (daxian-dbw) merged commit edf7cc0 into PowerShell:master Jun 9, 2020
@iSazonov

Copy link
Copy Markdown
Collaborator

Bryan Berns (@NoMoreFood) Thanks for your contribution!

@ghost

Copy link
Copy Markdown

🎉v7.1.0-preview.4 has been released which incorporates this pull request.:tada:

Handy links:

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj/PowerShell that referenced this pull request Aug 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking-Change breaking change that may affect users CL-BreakingChange Indicates that a PR should be marked as a breaking change in the Change Log CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log Committee-Reviewed PS-Committee has reviewed this and made a decision

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Start-Process fails in folder containing [

6 participants