Installer does not find schtasks if NoDefaultCurrentDirectoryInExePath=1

[KalleOlaviNiemitalo]

• I was not able to find an open or closed issue matching what I'm seeing

Setup

• Which version of Git for Windows are you using? Is it 32-bit or 64-bit?

$ git --version --build-options

git version 2.23.0.windows.1
cpu: x86_64
built from commit: 4db2e5cc9e1522131a039cbad3970f147a39f0ce
sizeof-long: 4
sizeof-size_t: 8

• Which version of Windows are you running? Vista, 7, 8, 10? Is it 32-bit or 64-bit?

Windows 7 Professional SP1 64-bit

$ cmd.exe /c ver

Microsoft Windows [Version 6.1.7601]

• What options did you set as part of the installation? Or did you choose the
  defaults?

$ cat /etc/install-options.txt

Editor Option: Nano
Custom Editor Path:
Path Option: Cmd
Plink Path: C:\Program Files\PuTTY\plink.exe
SSH Option: Plink
Tortoise Option: false
CURL Option: WinSSL
CRLF Option: LFOnly
Bash Terminal Option: MinTTY
Performance Tweaks FSCache: Enabled
Use Credential Manager: Disabled
Enable Symlinks: Disabled
Enable Builtin Interactive Add: Disabled

• Any other interesting things about your environment that might be related
  to the issue you're seeing?

The NoDefaultCurrentDirectoryInExePath value at HKEY_CURRENT_USER\Environment has type REG_SZ and data 1.

The Path value at HKEY_LOCAL_MACHINE\SYSTEM\CurrentControlSet\Control\Session Manager\Environment starts with %SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;. The type of that value is REG_EXPAND_SZ, which seems correct to me.

The Path value at HKEY_CURRENT_USER\Environment lists unrelated directories and has no percent signs in it. The type of that value is REG_SZ.

Details

• Which terminal/shell are you running Git from? e.g Bash/CMD/PowerShell/other

Not applicable. The error occurs during installation.

• What commands did you run to trigger this issue? If you can provide a
  Minimal, Complete, and Verifiable example
  this will help us understand the issue.

Double-click Git-2.23.0-64-bit.exe in Windows Explorer and select the options described above, plus "Check daily for Git for Windows updates".

• What did you expect to occur after running these commands?

Installation succeeds without errors.

• What actually happened instead?

An error message pops up:

---------------------------
Git 2.23.0 Setup
---------------------------
Line 2307: Unable to schedule the Git for Windows updater (output: , errors: 'schtasks' is not recognized as an internal or external command,

operable program or batch file.

).
---------------------------
OK   
---------------------------

I then click OK and installation continues.

• If the problem was occurring with a specific repository, can you provide the
  URL to that repository to help us with testing?

Not applicable.

Process Monitor

Process Monitor shows that Git-2.23.0-64-bit.tmp starts C:\Windows\system32\cmd.exe with the command line "C:\Windows\system32\cmd.exe" /C schtasks /Create /F /TN "Git for Windows Updater" /XML "C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\auto-updater.xml" >"C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\remove-autoupdate.log" 2>"C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\remove-autoupdate.err" and with a Path environment variable whose value starts with literally %SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;, rather than C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;. The cmd.exe process then tries to open C:\Windows\system32\%SystemRoot%\system32\ and fails.

When the Git-2.23.0-64-bit.tmp process starts, it gets a Path environment variable whose value correctly starts with C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;. It also starts several cmd.exe processes with this correct Path value, e.g. when it runs "C:\Windows\system32\cmd.exe" /C .\mingw64\bin\git.exe config --system "ssh.variant" "putty" >"C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\config-set.out" 2>"C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\config-set.err" (source?). However, the Path variable is already wrong when it runs "C:\Windows\system32\cmd.exe" /C .\mingw64\bin\git.exe config --system "core.editor" "nano.exe" >"C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\config-set.out" 2>"C:\Users\Kalle\AppData\Local\Temp\is-6PE04.tmp\config-set.err" (source?).

The working directory of the cmd.exe process is C:\Windows\system32\, but because the environment also contains NoDefaultCurrentDirectoryInExePath=1, cmd.exe does not search that directory when it is not listed in PATH.

[KalleOlaviNiemitalo]

In installer/environment.inc.iss, function SetEnvStrings has a comment talking about "environment variable names that are expanded at evaluation time": https://github.com/git-for-windows/build-extra/blob/a15a4b28caa9178bbaa3963c8940ac4e417e1ac3/installer/environment.inc.iss#L56-L61

As far as I can tell, when cmd.exe searches for an executable file, it does not expand any environment variable names that are listed in the value of the PATH environment variable. I believe those should instead be expanded when the value is copied from the Registry to the environment variable block of a process. On that basis, it seems wrong to me that the Expandable parameter of function SetEnvStrings affects only the type of the Registry values and does not affect the string passed to SetEnvironmentVariable. Perhaps it should call ExpandEnvironmentStringsW, but I don't know Inno Setup scripting.

[KalleOlaviNiemitalo]

I suppose an easy workaround might be to make the installer call SetEnvironmentVariable(TEXT("NoDefaultCurrentDirectoryInExePath"), nullptr); then the cmd.exe processes would not inherit that environment variable, and cmd.exe would find schtasks.exe from its current directory. This could be doable with less knowledge about string marshaling in Win32 calls from Inno Setup. It would not fix the bug (passing the wrong Path value to child processes) but would hide the symptom.

[jearld]

BeXEpATH1923

…

On Mon, Sep 2, 2019, 1:01 PM KalleOlaviNiemitalo ***@***.***> wrote: I suppose an easy workaround might be to make the installer call SetEnvironmentVariable(TEXT("NoDefaultCurrentDirectoryInExePath"), nullptr); then the cmd.exe processes would not inherit that environment variable, and cmd.exe would find schtasks.exe from its current directory. This could be doable with less knowledge about string marshaling in Win32 calls from Inno Setup. It would not fix the bug (passing the wrong Path value to child processes) but would hide the symptom. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#2319?email_source=notifications&email_token=AC65ZPWRT3WL2TOW4CWQQODQHVIJBA5CNFSM4IS55OMKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5WKZSQ#issuecomment-527215818>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AC65ZPTK3RHZ2AFTOVLRVC3QHVIJBANCNFSM4IS55OMA> .

[dscho]

Process Monitor shows that Git-2.23.0-64-bit.tmp starts C:\Windows\system32\cmd.exe with the command line "C:\Windows\system32\cmd.exe" /C schtasks [...]

This command-line should be correct, no?

with a Path environment variable whose value starts with literally %SystemRoot%\system32;%SystemRoot%;%SystemRoot%\System32\Wbem;

I once encountered such a PATH, but IIRC could not reproduce as soon as I had opened a cmd.exe...

In installer/environment.inc.iss, function SetEnvStrings has a comment talking about "environment variable names that are expanded at evaluation time": https://github.com/git-for-windows/build-extra/blob/a15a4b28caa9178bbaa3963c8940ac4e417e1ac3/installer/environment.inc.iss#L56-L61

But this only affects values obtained from the registry, correct? Is your default PATH incorrect, or even empty, then?

[KalleOlaviNiemitalo]

This command-line should be correct, no?

AFAIK, all the command lines are correct, just the Path environment variable is wrong.

Is your default PATH incorrect, or even empty, then?

If I start Command Prompt from the Start menu, then the cmd.exe process gets an environment in which Path correctly starts with C:\Windows\system32;C:\Windows;C:\Windows\System32\Wbem;.

And, if I double-click Git-2.23.0-64-bit.exe, then that process likewise gets a correct Path.

Does this answer your question or do you mean something else with "default PATH"?

[KalleOlaviNiemitalo]

@dscho, are you able to test with NoDefaultCurrentDirectoryInExePath=1 yourself? And of course select "Check daily for Git for Windows updates" in the installer; otherwise it wouldn't try to run schtasks in the first place. (I see that is not listed in install-options.txt.)

I found it useful to configure Process Monitor to display only events in which Operation is Process Start.

[dscho]

And, if I double-click Git-2.23.0-64-bit.exe, then that process likewise gets a correct Path.

So, under which circumstances does it not get a correct PATH?

[KalleOlaviNiemitalo]

So, under which circumstances does it not get a correct PATH?

Git-2.23.0-64-bit.tmp starts several cmd.exe child processes. The first few of them get the correct PATH but later ones get the wrong one with unexpanded %SystemRoot%. I did not examine the environment of every child process, but the child process for changing ssh.variant was OK and the next one, for changing core.editor, was not. From the source code, it appears that SetEnvStrings is called between those steps, here.

[dscho]

it appears that SetEnvStrings is called between those steps, here.

Isn't that call's Expandable parameter set to True? That should expand the environment variables within the PATH components, no?

[KalleOlaviNiemitalo]

Isn't that call's Expandable parameter set to True?

Yes, it is.

That should expand the environment variables within the PATH components, no?

That only makes SetEnvStrings call RegWriteExpandStringValue rather than RegWriteStringValue. The Expandable parameter does not affect the SetEnvironmentVariable call at the end, and I think that messes up the child processes.

[dscho]

The Expandable parameter does not affect the SetEnvironmentVariable call at the end, and I think that messes up the child processes.

Good point.

I suppose an easy workaround might be to make the installer call SetEnvironmentVariable(TEXT("NoDefaultCurrentDirectoryInExePath"), nullptr);

I guess that would work, but it does make me uneasy.

How about changing SetEnvStrings() so that it replaces %SysRoot% by the value of the environment variable? It would be awfully specific, but I haven't seen any other environment variables in PATH values, and it would probably cover at least the vast majority of similar breakages.

Or we go the whole nine yards and expand all environment variables, with something like this:

function ExpandEnvironmentStrings(lpSrc,lpDst:String;nSize:DWORD):DWORD;
#ifdef UNICODE
external 'ExpandEnvironmentStringsW@Kernel32.dll stdcall delayload';
#else
external 'ExpandEnvironmentStringsA@Kernel32.dll stdcall delayload';
#endif

function ExpandEnvironmentVariables(S:String):String;
var
    Size:DWORD;
begin
    Size:=ExpandEnvironmentStrings(S,'',0);
    if (Size=0) then
        LogError('Could not determine size of expanded "'+S+'"');
    else begin
        SetLength(Result,Size);
        if (ExpandEnvironmentStrings(S,Result,Size)=0) then
            LogError('Could not expand "'+S+'"');
    end;
end;

and then using that function in that SetEnvironmentVariable() call:

SetEnvironmentVariable(VarName,ExpandEnvironmentVariables(Path));

@KalleOlaviNiemitalo what do you think?

[dscho]

what do you think?

@KalleOlaviNiemitalo if you think this is worth doing, can you

1. install Git for Windows' SDK,
2. sdk cd installer,
3. edit environment.iss
4. build a new installer via sdk build installer
5. verify that that installer works as expected?
6. open a PR?

[KalleOlaviNiemitalo]

@dscho, I think it is better to expand all environment variables (when Expandable is True) than make a special case for %SystemRoot%. I also think it is better to use ExpandEnvironmentStrings rather than ExpandEnvironmentStringsForUser, because the results will be applied to the current process and its child processes.

However, perhaps the SetEnvironmentVariable call is not necessary in the first place.

6. open a PR?

For contractual reasons, I am not sure I can do that.