From 7a361c50b2954194c0e42121c2099701171a341b Mon Sep 17 00:00:00 2001 From: Wang Bo Date: Sat, 28 Mar 2026 07:43:26 +0800 Subject: [PATCH 1/2] Fix race condition in copyfile() causing inotify permission errors Apply the destination file's ownership and permissions to the temp file BEFORE the atomic move, instead of after. Previously, mkstemp() created the temp file with default 0600 permissions. After shutil.move() atomically replaced the destination, there was a brief window where the file was visible with restrictive permissions. Services using inotify to watch managed files (e.g. nscd watching /etc/resolv.conf) would detect the replacement via IN_DELETE_SELF, then fail to re-add a file watch with "Permission denied" due to the 0600 permissions, leading to service failures such as DNS resolution loss. By setting chown/chmod on the temp file before shutil.move(), the destination file is never visible with incorrect permissions, eliminating the race condition entirely. Fixes #65651 --- changelog/65651.fixed.md | 1 + salt/utils/files.py | 29 ++++--- tests/pytests/unit/utils/test_files.py | 109 +++++++++++++++++++++++++ 3 files changed, 124 insertions(+), 15 deletions(-) create mode 100644 changelog/65651.fixed.md diff --git a/changelog/65651.fixed.md b/changelog/65651.fixed.md new file mode 100644 index 000000000000..f15e695cc43f --- /dev/null +++ b/changelog/65651.fixed.md @@ -0,0 +1 @@ +Fixed a race condition in salt.utils.files.copyfile() where the temporary file was moved to the destination with mkstemp's default 0600 permissions before ownership and permissions were restored. Services using inotify to watch managed files (e.g. nscd watching /etc/resolv.conf) could fail to re-add a file watch during this window, leading to service failures such as DNS resolution loss. diff --git a/salt/utils/files.py b/salt/utils/files.py index f4bd53d84c81..3851a5d11cfb 100644 --- a/salt/utils/files.py +++ b/salt/utils/files.py @@ -209,21 +209,24 @@ def copyfile(source, dest, backup_mode="", cachedir=""): if backup_mode == "master" or backup_mode == "both" and bkroot: # TODO, backup to master pass - # Get current file stats to they can be replicated after the new file is - # moved to the destination path. - fstat = None - # We must check for platform availability first, or use a conditional import - # salt.utils.platform is available, but if this function is called early, it might fail? - # Actually, the error says 'salt' variable is used before assignment. - # This means 'import salt.utils.platform' is likely missing or 'salt' is not in scope. - # But this file starts with 'import salt.utils.platform'. - # Ah, 'import salt.utils.platform' is NOT at the top level of this file in the provided context? - # Let me check the imports. + # Get current file stats so they can be applied to the temp file BEFORE + # it is moved to the destination path. This eliminates a race condition + # where inotify-based services (e.g. nscd) detect the new file with + # mkstemp's default 0600 permissions before chown/chmod can restore + # them, causing "Permission denied" on inotify re-watch. if not salt.utils.platform.is_windows(): try: fstat = os.stat(dest) except OSError: - pass + # dest does not exist yet; nothing to preserve + fstat = None + if fstat is not None: + try: + os.chown(tgt, fstat.st_uid, fstat.st_gid) + os.chmod(tgt, fstat.st_mode) + except OSError: + __clean_tmp(tgt) + raise # The move could fail if the dest has xattr protections, so delete the # temp file in this case @@ -232,10 +235,6 @@ def copyfile(source, dest, backup_mode="", cachedir=""): except Exception: # pylint: disable=broad-except __clean_tmp(tgt) raise - - if fstat is not None: - os.chown(dest, fstat.st_uid, fstat.st_gid) - os.chmod(dest, fstat.st_mode) # If SELINUX is available run a restorecon on the file rcon = salt.utils.path.which("restorecon") if rcon: diff --git a/tests/pytests/unit/utils/test_files.py b/tests/pytests/unit/utils/test_files.py index 470fc3a582fb..3c265c8d291f 100644 --- a/tests/pytests/unit/utils/test_files.py +++ b/tests/pytests/unit/utils/test_files.py @@ -5,6 +5,7 @@ import copy import io import os +import shutil import pytest @@ -324,3 +325,111 @@ def test_is_binary_returns_false_for_text_files(): ): result = salt.utils.files.is_binary("/fake/file") assert result is False + + +@pytest.mark.skip_on_windows(reason="Unix-only permission test") +def test_copyfile_preserves_perms_before_move(tmp_path): + """ + Test that copyfile applies the destination file's mode to the result + and copies the content correctly. + """ + source = str(tmp_path / "source") + dest = str(tmp_path / "dest") + + with open(source, "w") as f: + f.write("new content") + with open(dest, "w") as f: + f.write("old content") + os.chmod(dest, 0o644) + + import stat as stat_mod + + dest_mode_before = stat_mod.S_IMODE(os.stat(dest).st_mode) + + salt.utils.files.copyfile(source, dest) + + dest_mode_after = stat_mod.S_IMODE(os.stat(dest).st_mode) + assert dest_mode_after == dest_mode_before + with open(dest) as f: + assert f.read() == "new content" + + +@pytest.mark.skip_on_windows(reason="Unix-only permission test") +def test_copyfile_perms_applied_before_move(tmp_path): + """ + Test that ownership and permissions are set on the temp file before + shutil.move, not after. We patch shutil.move to capture the temp + file's mode at move time, and patch os.chown to verify it is called + with the correct (fake) uid/gid before the move happens. + """ + import stat as stat_mod + + source = str(tmp_path / "source") + dest = str(tmp_path / "dest") + + with open(source, "w") as f: + f.write("new content") + with open(dest, "w") as f: + f.write("old content") + os.chmod(dest, 0o644) + + # Fake stat result with uid/gid different from current process + fake_uid = os.getuid() + 42 + fake_gid = os.getgid() + 42 + real_stat = os.stat(dest) + fake_stat = MagicMock() + fake_stat.st_uid = fake_uid + fake_stat.st_gid = fake_gid + fake_stat.st_mode = real_stat.st_mode + + captured_modes = [] + chown_calls = [] + move_called = [] + original_move = shutil.move + + def capturing_move(src, dst): + captured_modes.append(os.stat(src).st_mode) + move_called.append(True) + return original_move(src, dst) + + def capturing_chown(path, uid, gid): + chown_calls.append((uid, gid)) + # Verify chown is called BEFORE move + assert not move_called, "os.chown must be called before shutil.move" + + original_stat = os.stat + + def patched_stat(path): + result = original_stat(path) + if path == dest: + return fake_stat + return result + + with patch("salt.utils.files.shutil.move", side_effect=capturing_move), \ + patch("salt.utils.files.os.chown", side_effect=capturing_chown), \ + patch("salt.utils.files.os.stat", side_effect=patched_stat): + salt.utils.files.copyfile(source, dest) + + assert move_called, "shutil.move was not called" + # Mode should be 0o644 at move time, not mkstemp's default 0o600 + assert stat_mod.S_IMODE(captured_modes[0]) == 0o644 + # chown should have been called with the fake uid/gid + assert chown_calls, "os.chown was not called" + assert chown_calls[0] == (fake_uid, fake_gid) + + +def test_copyfile_new_file(tmp_path): + """ + Test that copyfile works when the destination does not exist yet. + No permissions to preserve in this case. + """ + source = str(tmp_path / "source") + dest = str(tmp_path / "dest") + + with open(source, "w") as f: + f.write("content") + + salt.utils.files.copyfile(source, dest) + + with open(dest) as f: + assert f.read() == "content" From e4108564d391f75b861274186045c1123c8be294 Mon Sep 17 00:00:00 2001 From: Wang Bo Date: Sat, 4 Apr 2026 20:52:27 +0800 Subject: [PATCH 2/2] Fix lint and formatting issues in copyfile tests - Replace open() with salt.utils.files.fopen() to satisfy W8470 and W1514 - Reformat multi-line with-patch statement for black compliance --- tests/pytests/unit/utils/test_files.py | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/pytests/unit/utils/test_files.py b/tests/pytests/unit/utils/test_files.py index 3c265c8d291f..d7acbbfa2650 100644 --- a/tests/pytests/unit/utils/test_files.py +++ b/tests/pytests/unit/utils/test_files.py @@ -336,9 +336,9 @@ def test_copyfile_preserves_perms_before_move(tmp_path): source = str(tmp_path / "source") dest = str(tmp_path / "dest") - with open(source, "w") as f: + with salt.utils.files.fopen(source, "w") as f: f.write("new content") - with open(dest, "w") as f: + with salt.utils.files.fopen(dest, "w") as f: f.write("old content") os.chmod(dest, 0o644) @@ -350,7 +350,7 @@ def test_copyfile_preserves_perms_before_move(tmp_path): dest_mode_after = stat_mod.S_IMODE(os.stat(dest).st_mode) assert dest_mode_after == dest_mode_before - with open(dest) as f: + with salt.utils.files.fopen(dest) as f: assert f.read() == "new content" @@ -367,9 +367,9 @@ def test_copyfile_perms_applied_before_move(tmp_path): source = str(tmp_path / "source") dest = str(tmp_path / "dest") - with open(source, "w") as f: + with salt.utils.files.fopen(source, "w") as f: f.write("new content") - with open(dest, "w") as f: + with salt.utils.files.fopen(dest, "w") as f: f.write("old content") os.chmod(dest, 0o644) @@ -405,9 +405,9 @@ def patched_stat(path): return fake_stat return result - with patch("salt.utils.files.shutil.move", side_effect=capturing_move), \ - patch("salt.utils.files.os.chown", side_effect=capturing_chown), \ - patch("salt.utils.files.os.stat", side_effect=patched_stat): + with patch("salt.utils.files.shutil.move", side_effect=capturing_move), patch( + "salt.utils.files.os.chown", side_effect=capturing_chown + ), patch("salt.utils.files.os.stat", side_effect=patched_stat): salt.utils.files.copyfile(source, dest) assert move_called, "shutil.move was not called" @@ -426,10 +426,10 @@ def test_copyfile_new_file(tmp_path): source = str(tmp_path / "source") dest = str(tmp_path / "dest") - with open(source, "w") as f: + with salt.utils.files.fopen(source, "w") as f: f.write("content") salt.utils.files.copyfile(source, dest) - with open(dest) as f: + with salt.utils.files.fopen(dest) as f: assert f.read() == "content"