diff --git a/changelog/50273.fixed.md b/changelog/50273.fixed.md new file mode 100644 index 000000000000..55f21b18309e --- /dev/null +++ b/changelog/50273.fixed.md @@ -0,0 +1 @@ +Fixed ``cmd.script`` with ``bg=True`` deleting the temporary script before the background process could execute it, which caused ``No such file or directory`` on POSIX. Background runs now use a self-cleaning wrapper so the child removes the tempfile after exit. Refs #50273 #69959 diff --git a/changelog/69959.fixed.md b/changelog/69959.fixed.md new file mode 100644 index 000000000000..51ca78974368 --- /dev/null +++ b/changelog/69959.fixed.md @@ -0,0 +1 @@ +Fixed ``cmd.script`` deleting the temporary script before a background (``bg=True``) process could run it. This caused PowerShell ``-File`` "does not exist" errors on Windows and "No such file or directory" on POSIX. Background runs now use a self-cleaning wrapper so the child removes the tempfile after exit. Refs #69959 #50273 diff --git a/salt/modules/cmdmod.py b/salt/modules/cmdmod.py index 5fe699f375f8..c69344fd76d7 100644 --- a/salt/modules/cmdmod.py +++ b/salt/modules/cmdmod.py @@ -289,6 +289,122 @@ def _prep_powershell_cmd(win_shell, cmd, encoded_cmd): return new_cmd +def _ps_single_quote(value): + """Escape a string for use inside a PowerShell single-quoted literal.""" + return str(value).replace("'", "''") + + +def _is_powershell_shell(shell): + """Return True if shell names Windows PowerShell or PowerShell Core.""" + if not shell: + return False + shell_l = str(shell).lower().strip() + return shell_l in ("powershell", "pwsh") or shell_l.endswith( + ("powershell.exe", "pwsh.exe") + ) + + +def _is_cmd_shell(shell): + """Return True if shell names cmd.exe.""" + if not shell: + return False + shell_l = str(shell).lower().strip() + return shell_l in ("cmd", "cmd.exe") or shell_l.endswith("cmd.exe") + + +def _prepare_bg_script(path, args, shell=None, win_cwd=None, cwd=None): + """ + Build a self-cleaning command for ``cmd.script`` when ``bg=True``. + + The parent must not delete ``path`` (or ``win_cwd``) after spawning the + background process. Instead we invoke a wrapper that runs the real script + and removes the tempfile(s) when finished. Refs #69959 #50273. + """ + args = list(args) if args else [] + + if salt.utils.platform.is_windows() and _is_powershell_shell(shell): + wrapper_dir = cwd if cwd else None + wrapper_path = salt.utils.files.mkstemp(dir=wrapper_dir, suffix=".ps1") + script_q = _ps_single_quote(path) + win_cwd_block = "" + if win_cwd: + cwd_q = _ps_single_quote(win_cwd) + win_cwd_block = ( + f" Set-Location $env:TEMP\n" + f" Remove-Item -LiteralPath '{cwd_q}' -Recurse -Force " + f"-ErrorAction SilentlyContinue\n" + ) + content = ( + f"$script = '{script_q}'\n" + "try {\n" + " & $script @args\n" + " exit $LASTEXITCODE\n" + "} finally {\n" + " Remove-Item -LiteralPath $script -Force " + "-ErrorAction SilentlyContinue\n" + " $wrapper = $PSCommandPath\n" + f"{win_cwd_block}" + " Remove-Item -LiteralPath $wrapper -Force " + "-ErrorAction SilentlyContinue\n" + "}\n" + ) + with salt.utils.files.fopen(wrapper_path, "w") as fh_: + fh_.write(content) + log.debug( + "cmd.script: bg=True PowerShell wrapper %s for script %s", + wrapper_path, + path, + ) + return [wrapper_path, *args] + + if salt.utils.platform.is_windows() and ( + _is_cmd_shell(shell) or str(path).lower().endswith((".bat", ".cmd")) + ): + wrapper_dir = cwd if cwd else None + wrapper_path = salt.utils.files.mkstemp(dir=wrapper_dir, suffix=".cmd") + lines = [ + "@echo off", + f'set "SALT_BG_SCRIPT={path}"', + 'call "%SALT_BG_SCRIPT%" %*', + "set SALT_BG_EC=%ERRORLEVEL%", + 'del /f /q "%SALT_BG_SCRIPT%" >nul 2>&1', + ] + if win_cwd: + lines.append(f'set "SALT_BG_CWD={win_cwd}"') + lines.append("cd /d %TEMP%") + lines.append('rd /s /q "%SALT_BG_CWD%" >nul 2>&1') + lines.extend( + [ + 'del /f /q "%~f0" >nul 2>&1', + "exit /b %SALT_BG_EC%", + ] + ) + with salt.utils.files.fopen(wrapper_path, "w") as fh_: + fh_.write("\r\n".join(lines) + "\r\n") + log.debug( + "cmd.script: bg=True cmd wrapper %s for script %s", + wrapper_path, + path, + ) + return [wrapper_path, *args] + + # POSIX: wrap with /bin/sh. Do not use exec — replacing the shell would + # skip the EXIT trap and leave the tempfile behind. + # argv: sh -c BODY salt-cmd-script "$path" "$path" args... + # $1 is the tempfile to remove; "$@" after shift is the real script argv. + sh_body = 'script="$1"; shift; trap \'rm -f -- "$script"\' EXIT; "$@"' + log.debug("cmd.script: bg=True POSIX /bin/sh wrapper for script %s", path) + return [ + "/bin/sh", + "-c", + sh_body, + "salt-cmd-script", + path, + path, + *args, + ] + + def _run( cmd, cwd=None, @@ -3091,7 +3207,12 @@ def _cleanup_tempfile(path): if isinstance(args, str): args = salt.utils.args.shlex_split(args) - new_cmd = [path, *args] if args else [path] + if bg: + new_cmd = _prepare_bg_script( + path, args, shell=shell, win_cwd=cwd if win_cwd else None, cwd=cwd + ) + else: + new_cmd = [path, *args] if args else [path] ret = {} try: @@ -3126,10 +3247,13 @@ def _cleanup_tempfile(path): exc, exc_info_on_loglevel=logging.DEBUG, ) - _cleanup_tempfile(path) - # If a temp working directory was created (Windows), let's remove that - if win_cwd: - _cleanup_tempfile(cwd) + # Background runs own tempfile cleanup via _prepare_bg_script wrappers. + # Deleting here races the child and causes missing-file errors (#69959). + if not bg: + _cleanup_tempfile(path) + # If a temp working directory was created (Windows), let's remove that + if win_cwd: + _cleanup_tempfile(cwd) if hide_output: ret["stdout"] = ret["stderr"] = "" diff --git a/tests/pytests/functional/modules/cmd/test_script.py b/tests/pytests/functional/modules/cmd/test_script.py index b8e1d0a48f91..9594bd3074b0 100644 --- a/tests/pytests/functional/modules/cmd/test_script.py +++ b/tests/pytests/functional/modules/cmd/test_script.py @@ -1,5 +1,7 @@ +import os import shlex import stat +import time from textwrap import dedent import pytest @@ -323,3 +325,64 @@ def test_script_pipe_spaces_runas(modules, pipe_script_with_space_runas, account password=account.password, ) assert result["stdout"] == "1" + + +@pytest.fixture +def bg_marker_script(state_tree, tmp_path): + """ + Script that writes a marker file (and its own path) for bg=True tests. + """ + marker = tmp_path / "bg_marker.txt" + if salt.utils.platform.is_windows(): + file_name = "bg_marker.bat" + # %~f0 is the full path to this bat file + contents = dedent( + f"""\ + @echo off + echo bg-ok^|%~f0>"{marker}" + """ + ) + else: + file_name = "bg_marker.sh" + contents = dedent( + f"""\ + #!/bin/sh + printf 'bg-ok|%s\\n' "$0" > "{marker}" + """ + ) + with pytest.helpers.temp_file(file_name, contents, state_tree) as script_path: + if not salt.utils.platform.is_windows(): + script_path.chmod(0o755) + yield file_name, marker + + +def _wait_for_marker(marker_path, timeout=30): + deadline = time.time() + timeout + while time.time() < deadline: + if marker_path.is_file() and marker_path.stat().st_size > 0: + return marker_path.read_text(encoding="utf-8").strip() + time.sleep(0.1) + raise AssertionError(f"Marker file not written within {timeout}s: {marker_path}") + + +def _wait_until_gone(path, timeout=30): + deadline = time.time() + timeout + while time.time() < deadline: + if not os.path.exists(path): + return + time.sleep(0.1) + raise AssertionError(f"Temp path still present after {timeout}s: {path}") + + +def test_script_bg_writes_marker_and_cleans_temp(modules, bg_marker_script): + """ + Regression for #69959 / #50273: cmd.script with bg=True must leave the + tempfile in place until the child runs, then clean it up. + """ + file_name, marker = bg_marker_script + ret = modules.cmd.script(f"salt://{file_name}", bg=True) + assert isinstance(ret["pid"], int) + contents = _wait_for_marker(marker) + payload, script_path = contents.split("|", 1) + assert payload == "bg-ok" + _wait_until_gone(script_path) diff --git a/tests/pytests/functional/modules/cmd/test_script_powershell.py b/tests/pytests/functional/modules/cmd/test_script_powershell.py index 08b792ac5294..6332f7db595e 100644 --- a/tests/pytests/functional/modules/cmd/test_script_powershell.py +++ b/tests/pytests/functional/modules/cmd/test_script_powershell.py @@ -1,3 +1,5 @@ +import os +import time from textwrap import dedent import pytest @@ -51,6 +53,26 @@ def echo_script(state_tree): yield exit_code +@pytest.fixture(scope="module") +def marker_script(state_tree): + """ + Write a marker file so bg=True tests can observe that the real script ran. + Also records $PSCommandPath so we can assert tempfile cleanup. + """ + script_contents = dedent( + """\ + param ( + [Parameter(Mandatory=$true)] + [string]$OutFile, + [string]$Payload = "ok" + ) + Set-Content -LiteralPath $OutFile -Value "$Payload|$PSCommandPath" + """ + ) + with pytest.helpers.temp_file("marker.ps1", script_contents, state_tree): + yield + + @pytest.fixture(params=["powershell", "pwsh"]) def shell(request): """ @@ -115,3 +137,45 @@ def test_echo_runas(cmd, shell, account, echo_script, args, expected): assert ret["retcode"] == 0 assert ret["stderr"] == "" assert ret["stdout"] == expected + + +def _wait_for_marker(marker_path, timeout=30): + """Poll until the background script writes the marker file.""" + deadline = time.time() + timeout + while time.time() < deadline: + if marker_path.is_file() and marker_path.stat().st_size > 0: + return marker_path.read_text(encoding="utf-8").strip() + time.sleep(0.1) + raise AssertionError(f"Marker file not written within {timeout}s: {marker_path}") + + +def _wait_until_gone(path, timeout=30): + """Poll until path is removed by the bg self-cleanup wrapper.""" + deadline = time.time() + timeout + while time.time() < deadline: + if not os.path.exists(path): + return + time.sleep(0.1) + raise AssertionError(f"Temp path still present after {timeout}s: {path}") + + +def test_script_bg_writes_marker_and_cleans_temp(cmd, shell, marker_script, tmp_path): + """ + Regression for #69959 / #50273: cmd.script bg=True must not delete the + tempfile before PowerShell can open it, and must still clean up afterward. + """ + marker = tmp_path / "marker.txt" + ret = cmd.script( + "salt://marker.ps1", + args=["-OutFile", str(marker), "-Payload", "bg-ok"], + shell=shell, + saltenv="base", + bg=True, + ) + assert isinstance(ret["pid"], int) + # Background runs do not wait for the process; retcode is not meaningful. + contents = _wait_for_marker(marker) + payload, script_path = contents.split("|", 1) + assert payload == "bg-ok" + assert script_path.lower().endswith(".ps1") + _wait_until_gone(script_path) diff --git a/tests/pytests/unit/modules/test_cmdmod.py b/tests/pytests/unit/modules/test_cmdmod.py index a4eedfb9d80c..76cde261e1f7 100644 --- a/tests/pytests/unit/modules/test_cmdmod.py +++ b/tests/pytests/unit/modules/test_cmdmod.py @@ -1497,3 +1497,71 @@ def test_prep_powershell_json(text, expected): """ result = cmdmod._prep_powershell_json(text) assert result == expected + + +def test_ps_single_quote(): + assert cmdmod._ps_single_quote(r"C:\temp\file.ps1") == r"C:\temp\file.ps1" + assert cmdmod._ps_single_quote("O'Brien") == "O''Brien" + + +@pytest.mark.parametrize( + "shell, expected", + [ + ("powershell", True), + ("pwsh", True), + (r"C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe", True), + ("cmd", False), + (None, False), + ], +) +def test_is_powershell_shell(shell, expected): + assert cmdmod._is_powershell_shell(shell) is expected + + +def test_prepare_bg_script_posix(): + path = "/tmp/__salt.tmp.abc123.sh" + ret = cmdmod._prepare_bg_script(path, ["arg1", "arg two"], shell="/bin/sh") + assert ret[0] == "/bin/sh" + assert ret[1] == "-c" + assert "trap" in ret[2] + # exec would replace /bin/sh and skip the EXIT trap (tempfile leak). + assert "exec" not in ret[2] + assert ret[3] == "salt-cmd-script" + assert ret[4] == path + assert ret[5] == path + assert ret[6:] == ["arg1", "arg two"] + + +@pytest.mark.skip_unless_on_windows +def test_prepare_bg_script_powershell(tmp_path): + script = tmp_path / "__salt.tmp.real.ps1" + script.write_text("Write-Output hi\n", encoding="utf-8") + ret = cmdmod._prepare_bg_script( + str(script), ["-OutFile", "x"], shell="powershell", cwd=str(tmp_path) + ) + assert len(ret) == 3 + wrapper = ret[0] + assert wrapper.endswith(".ps1") + assert ret[1:] == ["-OutFile", "x"] + with salt.utils.files.fopen(wrapper) as fh_: + content = fh_.read() + assert str(script) in content + assert "& $script @args" in content + assert "Remove-Item -LiteralPath $script" in content + os.remove(wrapper) + + +@pytest.mark.skip_unless_on_windows +def test_prepare_bg_script_cmd(tmp_path): + script = tmp_path / "__salt.tmp.real.bat" + script.write_text("@echo off\necho hi\n", encoding="utf-8") + ret = cmdmod._prepare_bg_script( + str(script), ["a", "b"], shell="cmd", cwd=str(tmp_path) + ) + assert ret[0].endswith(".cmd") + assert ret[1:] == ["a", "b"] + with salt.utils.files.fopen(ret[0]) as fh_: + content = fh_.read() + assert str(script) in content + assert "SALT_BG_SCRIPT" in content + os.remove(ret[0])