Fsync atomic_open temp file before rename (#69583)

changelog/69583.fixed.md

@@ -0,0 +1 @@
+Fixed `salt.utils.atomicfile.atomic_open` to fsync the temp file before the atomic rename so a crash after the rename cannot expose a truncated or partial file.

salt/utils/atomicfile.py

@@ -128,6 +128,19 @@ def __enter__(self):
     def close(self):
         if self._fh.closed:
             return
+        # Flush user-space buffers and fsync the file's data + metadata
+        # before the atomic rename. Without this, a crash after the rename
+        # can expose a written-but-unsynced (truncated/partial) file:
+        # POSIX does not require an implicit fsync on rename, and the
+        # fsync-on-rename heuristic of ext4 et al. is not universal. The
+        # fileno() / fsync() pair is guarded because some file-like objects
+        # do not expose a real fd (e.g. an in-memory wrapper); in that case
+        # there is nothing useful to sync at the fd level.
+        try:
+            self._fh.flush()
+            os.fsync(self._fh.fileno())
+        except (AttributeError, OSError, ValueError):
+            pass
         self._fh.close()
         if salt.utils.win_dacl.HAS_WIN32:
             if os.path.isfile(self._filename):

tests/pytests/unit/utils/test_atomicfile.py

@@ -2,10 +2,14 @@
 Tests for atomicfile utility module.
 """
 
+import os
+
 import pytest
 
+import salt.utils.atomicfile
 import salt.utils.files
 from salt.utils.atomicfile import atomic_open
+from tests.support.mock import patch
 
 
 @pytest.mark.skip_on_windows(reason="Not a Windows test")
@@ -25,3 +29,41 @@ def test_atomicfile_respects_umask(tmp_path):
 
     assert new_file.read_text() == contents
     assert oct(new_file.stat().st_mode)[-3:] == "644"
+
+
+def test_atomicfile_fsyncs_before_rename(tmp_path):
+    """
+    The atomic_open close path must fsync the temp file before renaming it
+    over the destination, otherwise a crash after the rename can expose a
+    written-but-unsynced (truncated/partial) file. See #69583.
+    """
+    new_file = tmp_path / "foo"
+    fsynced_fds = []
+    call_order = []
+
+    real_fsync = os.fsync
+
+    def tracking_fsync(fd):
+        fsynced_fds.append(fd)
+        call_order.append("fsync")
+        return real_fsync(fd)
+
+    real_rename = salt.utils.atomicfile.atomic_rename
+
+    def tracking_rename(src, dst):
+        call_order.append("rename")
+        return real_rename(src, dst)
+
+    with patch.object(salt.utils.atomicfile.os, "fsync", tracking_fsync), patch.object(
+        salt.utils.atomicfile, "atomic_rename", tracking_rename
+    ):
+        with atomic_open(str(new_file), "w") as fh_:
+            tmp_fd = fh_.fileno()
+            fh_.write("bar")
+
+    assert tmp_fd in fsynced_fds, "expected os.fsync on the temp file fd"
+    assert "fsync" in call_order and "rename" in call_order
+    assert call_order.index("fsync") < call_order.index(
+        "rename"
+    ), "fsync must be called before the atomic rename"
+    assert new_file.read_text() == "bar"