Skip to content

Conversation

samhocevar
Copy link

@samhocevar samhocevar commented Aug 18, 2023

The DefaultShellCommandOption registry key has a size limit of 15 characters. This seems unreasonably small especially since DefaultShellArguments has a much larger limit and its contents are not used for a non-interactive shell, so a lot of what is in there has to be duplicated.

For instance, if I wanted to configure OpenSSH to use MSYS2 without creating a helper script, this is what my registry keys would look like:

DefaultShell                 C:\msys64\msys2_shell.cmd
DefaultShellArguments        -defterm -mingw64 -no-start -shell zsh
DefaultShellCommandOption    -defterm -mingw64 -no-start -shell zsh -c
DefaultShellEscapeArguments  0

This PR increases the limit to the same value as DefaultShellArguments, which is currently PATH_MAX / 2 - 1.

@tgauth
Copy link
Collaborator

tgauth commented Aug 21, 2023

commenting to try to kick-off CI...have had some changes to the trigger recently

@tgauth
Copy link
Collaborator

tgauth commented Aug 21, 2023

CI looks good - thanks @samhocevar!

@tgauth
Copy link
Collaborator

tgauth commented Aug 21, 2023

Actually - I'm having trouble repro'ing the scenario in the description with these changes so there may be additional changes needed to get this to work.

The non-interactive session still fails to return the output of the command to the console.

@tgauth tgauth self-requested a review August 21, 2023 19:43
@samhocevar
Copy link
Author

Sorry, DefaultShellEscapeArguments should actually be 0 in that case. I have edited my original report.

@samhocevar
Copy link
Author

Maybe the following information can help pinpoint the problem? Without modifying OpenSSH, my setup uses the following configuration:

DefaultShell                 C:\msys64\msys2_shell_helper.cmd
DefaultShellArguments        -shell zsh -l
DefaultShellCommandOption    -shell zsh -c
DefaultShellEscapeArguments  0

My C:\msys64\msys2_shell_helper.cmd wrapper contains just the following line:

@call %~dp0\msys2_shell.cmd -defterm -mingw64 -no-start %*

And non-interactive login works like this:

22/08 23:51 sam@potpal🪟~% ssh localhost uname -a
sam@localhost's password:
MINGW64_NT-10.0-22621 potpal 3.4.7.x86_64 2023-08-09 17:32 UTC x86_64 Msys
22/08 23:51 sam@potpal🪟~%

@tgauth
Copy link
Collaborator

tgauth commented Aug 23, 2023

Thanks @samhocevar - are you running ssh localhost uname -a from a zsh session?

With just the DefaultShell configured, I'm able to execute a non-interactive command.
With both the DefaultShell and DefaultShellCommandOption configured, I'm getting a - : invalid option

@samhocevar
Copy link
Author

samhocevar commented Aug 24, 2023

Thanks for investigating thoroughly!

In my case, it does not seem to matter what the originating shell is, I get the same behaviour from PowerShell (7.1.0-preview.3), MSYS2 (hard to tell the exact version, but mostly current), or WSL (Debian Trixie):
image
image
image

Here is my exact registry configuration for SSHd:

% reg query 'HKLM\Software\OpenSSH'

HKEY_LOCAL_MACHINE\Software\OpenSSH
    DefaultShell    REG_SZ    C:\msys64\msys2_shell_helper.cmd
    DefaultShellArguments    REG_SZ    -shell zsh -l
    DefaultShellEscapeArguments    REG_DWORD    0x0
    DefaultShellCommandOption    REG_SZ    -shell zsh -c

My unmodified Windows SSHd (C:/Windows/System32/OpenSSH/sshd.exe) version is OpenSSH_for_Windows_8.6p1, LibreSSL 3.4.3.

My Zsh version (C:/msys64/usr/bin/zsh.exe) is zsh 5.9 (x86_64-pc-msys).

I am attaching the full contents of C:/msys64/msys2_shell.cmd because that is the file most likely to differ, with the rather inconsistent upgrade paths one may have with MSYS2:

@tgauth
Copy link
Collaborator

tgauth commented Sep 26, 2023

Thanks for the additional clarifications @samhocevar - am now able to repro the fix locally.

One more question...

This PR increases the limit to the same value as DefaultShellArguments, which is currently PATH_MAX / 2 - 1.

Is DefaultShellArguments set to PATH_MAX / 2 - 1 or just PATH_MAX?

@tgauth tgauth merged commit 1ab66dd into PowerShell:latestw_all Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants