diff --git a/changelog/68269.fixed.md b/changelog/68269.fixed.md new file mode 100644 index 00000000000..230abebf002 --- /dev/null +++ b/changelog/68269.fixed.md @@ -0,0 +1 @@ +Fixed the salt-minion (and salt-api, salt-cloud, salt-master, salt-syndic) Debian postinst scripts hanging or erroring with "Bad file descriptor" when run from a non-interactive Debian preseed late_command chroot, by tearing down the debconf protocol with ``db_stop`` and explicitly closing file descriptor 3 before the auto-generated ``#DEBHELPER#`` section runs. diff --git a/pkg/debian/salt-api.postinst b/pkg/debian/salt-api.postinst index 8bb03451c44..668d36d953e 100644 --- a/pkg/debian/salt-api.postinst +++ b/pkg/debian/salt-api.postinst @@ -43,6 +43,17 @@ case "$1" in ;; esac +# Terminate the debconf protocol and close fd 3 before the +# debhelper-generated section runs. Without this, fd 3 (opened by +# confmodule above) leaks into commands such as deb-systemd-helper and +# produces "Bad file descriptor" errors in non-interactive preseed +# late_command chroot installs where the parent debconf frontend is +# not reachable. See https://github.com/saltstack/salt/issues/68269. +if [ "$1" = "configure" ] && command -v db_stop >/dev/null 2>&1; then + db_stop + exec 3>&- || true +fi + # dh_installdeb will replace this with shell code automatically # generated by other debhelper scripts. diff --git a/pkg/debian/salt-cloud.postinst b/pkg/debian/salt-cloud.postinst index b5b7257c9fc..08eb932503e 100644 --- a/pkg/debian/salt-cloud.postinst +++ b/pkg/debian/salt-cloud.postinst @@ -41,6 +41,17 @@ case "$1" in ;; esac +# Terminate the debconf protocol and close fd 3 before the +# debhelper-generated section runs. Without this, fd 3 (opened by +# confmodule above) leaks into commands such as deb-systemd-helper and +# produces "Bad file descriptor" errors in non-interactive preseed +# late_command chroot installs where the parent debconf frontend is +# not reachable. See https://github.com/saltstack/salt/issues/68269. +if [ "$1" = "configure" ] && command -v db_stop >/dev/null 2>&1; then + db_stop + exec 3>&- || true +fi + # dh_installdeb will replace this with shell code automatically # generated by other debhelper scripts. diff --git a/pkg/debian/salt-master.postinst b/pkg/debian/salt-master.postinst index 86a76a6ade9..1a64bbcde8e 100644 --- a/pkg/debian/salt-master.postinst +++ b/pkg/debian/salt-master.postinst @@ -51,6 +51,17 @@ case "$1" in ;; esac +# Terminate the debconf protocol and close fd 3 before the +# debhelper-generated section runs. Without this, fd 3 (opened by +# confmodule above) leaks into commands such as deb-systemd-helper and +# produces "Bad file descriptor" errors in non-interactive preseed +# late_command chroot installs where the parent debconf frontend is +# not reachable. See https://github.com/saltstack/salt/issues/68269. +if [ "$1" = "configure" ] && command -v db_stop >/dev/null 2>&1; then + db_stop + exec 3>&- || true +fi + # dh_installdeb will replace this with shell code automatically # generated by other debhelper scripts. diff --git a/pkg/debian/salt-minion.postinst b/pkg/debian/salt-minion.postinst index 7378a97cc6c..b7bd71a977a 100644 --- a/pkg/debian/salt-minion.postinst +++ b/pkg/debian/salt-minion.postinst @@ -65,6 +65,17 @@ case "$1" in ;; esac +# Terminate the debconf protocol and close fd 3 before the +# debhelper-generated section runs. Without this, fd 3 (opened by +# confmodule above) leaks into commands such as deb-systemd-helper and +# produces "Bad file descriptor" errors in non-interactive preseed +# late_command chroot installs where the parent debconf frontend is +# not reachable. See https://github.com/saltstack/salt/issues/68269. +if [ "$1" = "configure" ] && command -v db_stop >/dev/null 2>&1; then + db_stop + exec 3>&- || true +fi + # dh_installdeb will replace this with shell code automatically # generated by other debhelper scripts. diff --git a/pkg/debian/salt-syndic.postinst b/pkg/debian/salt-syndic.postinst index ed873caa634..9a94793124c 100644 --- a/pkg/debian/salt-syndic.postinst +++ b/pkg/debian/salt-syndic.postinst @@ -45,6 +45,17 @@ case "$1" in ;; esac +# Terminate the debconf protocol and close fd 3 before the +# debhelper-generated section runs. Without this, fd 3 (opened by +# confmodule above) leaks into commands such as deb-systemd-helper and +# produces "Bad file descriptor" errors in non-interactive preseed +# late_command chroot installs where the parent debconf frontend is +# not reachable. See https://github.com/saltstack/salt/issues/68269. +if [ "$1" = "configure" ] && command -v db_stop >/dev/null 2>&1; then + db_stop + exec 3>&- || true +fi + # dh_installdeb will replace this with shell code automatically # generated by other debhelper scripts. diff --git a/tests/pytests/unit/pkg/__init__.py b/tests/pytests/unit/pkg/__init__.py new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/pytests/unit/pkg/test_debian_postinst.py b/tests/pytests/unit/pkg/test_debian_postinst.py new file mode 100644 index 00000000000..5e5d11e744a --- /dev/null +++ b/tests/pytests/unit/pkg/test_debian_postinst.py @@ -0,0 +1,183 @@ +""" +Regression tests for the Debian postinst scripts. + +These verify that the postinst scripts source debconf via ``confmodule`` +but tear it down with ``db_stop`` and explicitly close file descriptor 3 +before the auto-generated ``#DEBHELPER#`` section runs. + +Without this teardown, the debconf passthrough fd 3 leaks into the +debhelper-added systemd commands. In non-interactive Debian preseed +``late_command`` chroot installs (where the parent debconf frontend is +not reachable) this produces "Bad file descriptor" errors and can hang +the install. See https://github.com/saltstack/salt/issues/68269. +""" + +import shutil +import subprocess +import sys +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[4] +PKG_DEBIAN = REPO_ROOT / "pkg" / "debian" + +# All postinst scripts that source ``confmodule``. They must all be made +# safe; the user-reported failure was salt-minion but salt-master, +# salt-api, salt-cloud, salt-syndic share the same defective structure. +POSTINST_SCRIPTS = [ + "salt-api.postinst", + "salt-cloud.postinst", + "salt-master.postinst", + "salt-minion.postinst", + "salt-syndic.postinst", +] + + +pytestmark = [ + pytest.mark.skipif( + sys.platform == "win32", + reason="Debian postinst scripts only run on Linux/POSIX shells", + ), + pytest.mark.skipif( + shutil.which("sh") is None, + reason="POSIX /bin/sh shell is required to exercise postinst", + ), +] + + +def _build_confmodule_stub(stub_path): + """ + Write a stub that mimics the real /usr/share/debconf/confmodule. + + Sets up fd 3 (the debconf passthrough) so it points somewhere + writable (real confmodule does ``exec 3>&1``), exports + ``DEBIAN_HAS_FRONTEND`` so we don't try to re-exec a frontend, and + defines minimal stubs for ``db_get`` and ``db_stop`` matching the + real confmodule semantics. + + Crucially, ``db_stop`` here mirrors the real implementation: + ``echo STOP >&3``. It does NOT close fd 3 itself; the *postinst* is + responsible for ``exec 3>&-`` after ``db_stop`` so that any leak of + fd 3 into subsequently-executed commands cannot occur. + """ + stub_path.write_text( + # Mimic the real confmodule: open fd 3 onto original stdout, + # then redirect stdout to stderr (as confmodule itself does). + # If the script later writes to fd 3 after we close it, this + # would error with "Bad file descriptor". + "exec 3>&1\n" + "exec 1>&2\n" + "DEBCONF_REDIR=1\n" + "export DEBCONF_REDIR\n" + "DEBIAN_HAS_FRONTEND=1\n" + "export DEBIAN_HAS_FRONTEND\n" + # db_get: return the template default ('root' for salt-*/user + # questions). The real db_get sets $RET and returns 0. + "db_get() { RET=root; return 0; }\n" + # db_stop: real one sends STOP to fd 3. We do the same so we + # can detect whether the postinst called it. + 'db_stop() { echo STOP >&3 2>/dev/null || true; touch "$_DB_STOP_MARKER"; }\n' + ) + + +def _build_debhelper_stub(marker_path): + """ + Stand in for the auto-generated ``#DEBHELPER#`` block that dh adds + to the installed postinst (deb-systemd-helper unmask/enable/etc.). + + The real failure mode is that these inherited fd 3 (still pointing + at a stale debconf passthrough). We mimic that by asserting fd 3 + is *not* open here. If fd 3 is still open, we fail loudly — exactly + what surfaces as 'Bad file descriptor' in the wild. + + Note: we use ``if echo test 2>/dev/null >&3`` (a simple command, + not a brace-group) so that ``set -e`` plus the redirection failure + is captured into the ``if`` test rather than terminating the script. + """ + # Briefly disable set -e because dash treats a failed redirection + # in the *test* of an ``if`` differently depending on whether the + # command is simple or a group, and we want the leak check to be + # purely a probe — not affect the script's exit semantics. + return ( + "# Stand-in for the debhelper-generated systemd section.\n" + "# In the buggy case, fd 3 leaks into here from confmodule.\n" + "set +e\n" + "if echo probe 2>/dev/null >&3; then\n" + f' echo "FAIL: fd 3 still open in #DEBHELPER# section" > "{marker_path}"\n' + " exit 42\n" + "fi\n" + "set -e\n" + ) + + +def _materialize_postinst(script_name, tmp_path): + """ + Copy a postinst script into ``tmp_path`` with two substitutions: + + 1. ``. /usr/share/debconf/confmodule`` → source our stub instead. + 2. ``#DEBHELPER#`` token → the fd-3-leak detector. + + Returns the path to the rewritten script and the marker paths used + by the stubs. + """ + src = (PKG_DEBIAN / script_name).read_text() + confmodule_stub = tmp_path / "confmodule" + debhelper_marker = tmp_path / "debhelper_failed" + db_stop_marker = tmp_path / "db_stop_called" + _build_confmodule_stub(confmodule_stub) + + rewritten = src.replace( + ". /usr/share/debconf/confmodule", + f". {confmodule_stub}", + ).replace( + "#DEBHELPER#", + _build_debhelper_stub(debhelper_marker), + ) + script = tmp_path / script_name + script.write_text(rewritten) + script.chmod(0o755) + return script, debhelper_marker, db_stop_marker + + +@pytest.mark.parametrize("script_name", POSTINST_SCRIPTS) +def test_postinst_closes_debconf_fd3_before_debhelper(script_name, tmp_path): + """ + The configure branch of each postinst must call ``db_stop`` and + close fd 3 before the auto-generated ``#DEBHELPER#`` section runs. + + Without this, fd 3 (the debconf protocol passthrough opened by + ``confmodule``) leaks into ``deb-systemd-helper`` invocations and + produces ``Bad file descriptor`` errors in non-interactive + preseed installs (issue #68269). + """ + script, debhelper_failed, db_stop_marker = _materialize_postinst( + script_name, tmp_path + ) + env = { + "PATH": "/usr/bin:/bin", + # Stub points db_stop's marker writer here. + "_DB_STOP_MARKER": str(db_stop_marker), + # Force a non-interactive, no-prompts environment. + "DEBIAN_FRONTEND": "noninteractive", + } + result = subprocess.run( + ["/bin/sh", str(script), "configure"], + env=env, + capture_output=True, + text=True, + timeout=30, + check=False, + ) + assert not debhelper_failed.exists(), ( + f"#DEBHELPER# section observed fd 3 still open in {script_name}: " + f"stdout={result.stdout!r} stderr={result.stderr!r}" + ) + assert result.returncode == 0, ( + f"{script_name} exited {result.returncode}: " + f"stdout={result.stdout!r} stderr={result.stderr!r}" + ) + assert db_stop_marker.exists(), ( + f"{script_name} configure branch must call db_stop to terminate " + f"the debconf protocol; marker missing" + )