From 6acd1add9c07436e5dc095434450b4daa33faba0 Mon Sep 17 00:00:00 2001 From: Om Singhal Date: Thu, 10 Sep 2026 13:47:00 -0400 Subject: [PATCH 1/4] Write session files atomically so a partial write cannot destroy them save() opened the target with 'w', which truncates it in place and then streams the document into it. While that runs the file on disk is empty or half written, and a concurrent process that reads it fails to parse it, at which point load() overwrites it with defaults. A process that only meant to read the file destroys it. The same truncation loses the file outright if the write itself fails partway, with no second process involved. It now writes a temporary file in the same directory and renames it over the target, so a reader sees either the old document or the new one. The guards each cover a case the plain rename would regress: realpath so a symlinked config file is followed rather than replaced, chmod so mkstemp's 0600 does not silently tighten an existing file, the OSError fallback so a read only config directory still saves the way it used to, and BaseException so an interrupt does not leave a stray temporary file. This does not serialize concurrent writers. Two saves can still overwrite each other, cleanly rather than corruptly. That is #14070 and needs a lock. --- src/azure-cli-core/azure/cli/core/_session.py | 40 +++++- .../azure/cli/core/tests/test_session.py | 120 ++++++++++++++++++ 2 files changed, 159 insertions(+), 1 deletion(-) create mode 100644 src/azure-cli-core/azure/cli/core/tests/test_session.py diff --git a/src/azure-cli-core/azure/cli/core/_session.py b/src/azure-cli-core/azure/cli/core/_session.py index 294cc923f74..a82736d0733 100644 --- a/src/azure-cli-core/azure/cli/core/_session.py +++ b/src/azure-cli-core/azure/cli/core/_session.py @@ -6,6 +6,8 @@ import json import logging import os +import stat +import tempfile import time from collections.abc import MutableMapping @@ -50,9 +52,45 @@ def load(self, filename, max_age=0): self.save() def save(self): - if self.filename: + if not self.filename: + return + + # Write to a temporary file in the same directory and rename it over the target, so the + # file on disk is never observably incomplete. Opening the target with 'w' truncates it + # in place, which leaves a window where a concurrent process reads zero or half a + # document. That reader then fails to parse it and load() overwrites it with defaults, + # so a process that only meant to read destroys the data. + target = os.path.realpath(self.filename) + directory = os.path.dirname(target) or os.curdir + + try: + fd, temp_name = tempfile.mkstemp(dir=directory, prefix=os.path.basename(target) + '.', + suffix='.tmp') + except OSError: + # The directory is not writable, which happens in locked down containers and CI + # images. Writing in place still works there, so keep the old behaviour rather than + # failing a save that used to succeed. with open(self.filename, 'w', encoding=self._encoding) as f: json.dump(self.data, f) + return + + try: + with os.fdopen(fd, 'w', encoding=self._encoding) as f: + json.dump(self.data, f) + # mkstemp creates the file 0o600. Carry over the permissions of the file being + # replaced so a save does not silently tighten them. + try: + os.chmod(temp_name, stat.S_IMODE(os.stat(target).st_mode)) + except OSError: + pass + os.replace(temp_name, target) + except BaseException: + # BaseException so that an interrupt does not leave the temporary file behind. + try: + os.remove(temp_name) + except OSError: + pass + raise def save_with_retry(self, retries=5): for _ in range(retries - 1): diff --git a/src/azure-cli-core/azure/cli/core/tests/test_session.py b/src/azure-cli-core/azure/cli/core/tests/test_session.py new file mode 100644 index 00000000000..d29fa481895 --- /dev/null +++ b/src/azure-cli-core/azure/cli/core/tests/test_session.py @@ -0,0 +1,120 @@ +# -------------------------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for license information. +# -------------------------------------------------------------------------------------------- + +import json +import os +import stat +import tempfile +import unittest + +from azure.cli.core._session import Session + + +class TestSession(unittest.TestCase): + + def setUp(self): + self.dir = tempfile.mkdtemp() + self.filename = os.path.join(self.dir, 'test.json') + + def _session(self, data): + session = Session() + session.filename = self.filename + session.data = data + return session + + def _read(self): + with open(self.filename, encoding='utf-8-sig') as f: + return json.load(f) + + def _temp_files(self): + return [name for name in os.listdir(self.dir) if name.endswith('.tmp')] + + def test_save_writes_the_data(self): + self._session({'a': 1}).save() + self.assertEqual(self._read(), {'a': 1}) + + def test_save_replaces_the_file_instead_of_truncating_it(self): + # A truncating write leaves the file empty or half written while it runs, and a concurrent + # reader that fails to parse it has load() overwrite it with defaults. Replacing the file + # means a reader always sees either the old contents or the new ones. + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({'a': 1}, f) + inode = os.stat(self.filename).st_ino + + self._session({'a': 2}).save() + + self.assertNotEqual(os.stat(self.filename).st_ino, inode) + self.assertEqual(self._read(), {'a': 2}) + + def test_save_leaves_the_file_intact_when_the_data_cannot_be_serialized(self): + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({'kept': True}, f) + + with self.assertRaises(TypeError): + self._session({'a': 'fine', 'b': {1, 2}}).save() + + self.assertEqual(self._read(), {'kept': True}) + self.assertFalse(self._temp_files()) + + def test_save_preserves_the_permissions_of_an_existing_file(self): + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({}, f) + os.chmod(self.filename, 0o644) + + self._session({'a': 1}).save() + + self.assertEqual(stat.S_IMODE(os.stat(self.filename).st_mode), 0o644) + + @unittest.skipUnless(hasattr(os, 'symlink'), 'requires symlink support') + def test_save_follows_a_symlink_rather_than_replacing_it(self): + target = os.path.join(self.dir, 'target.json') + link = os.path.join(self.dir, 'link.json') + with open(target, 'w', encoding='utf-8-sig') as f: + json.dump({'a': 1}, f) + os.symlink(target, link) + + session = Session() + session.filename = link + session.data = {'a': 2} + session.save() + + self.assertTrue(os.path.islink(link)) + with open(target, encoding='utf-8-sig') as f: + self.assertEqual(json.load(f), {'a': 2}) + + @unittest.skipIf(os.name == 'nt', 'directory permissions are not enforced the same way on Windows') + def test_save_still_writes_when_the_directory_is_not_writable(self): + # Locked down containers and build images mount the config directory read only while + # leaving the file itself writable. A save that used to succeed there must keep working. + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({}, f) + os.chmod(self.dir, 0o500) + try: + self._session({'a': 1}).save() + self.assertEqual(self._read(), {'a': 1}) + finally: + os.chmod(self.dir, 0o700) + + def test_load_reads_back_what_save_wrote(self): + self._session({'a': 1}).save() + + session = Session() + session.load(self.filename) + + self.assertEqual(session.data, {'a': 1}) + + def test_load_overrides_a_file_that_cannot_be_parsed(self): + with open(self.filename, 'w', encoding='utf-8-sig') as f: + f.write('{"truncated"') + + session = Session() + session.load(self.filename) + + self.assertEqual(session.data, {}) + self.assertEqual(self._read(), {}) + + +if __name__ == '__main__': + unittest.main() From 9ebb618f5e5e29efefbf3e4c1f356a2721c52a1e Mon Sep 17 00:00:00 2001 From: Om Singhal Date: Thu, 10 Sep 2026 19:46:19 -0400 Subject: [PATCH 2/4] Only fall back to the in place write when the directory denies permission The fallback caught every OSError from mkstemp. A full disk or an exceeded quota would land there too, and the in place write would fail the same way after having already truncated the file, which is the data loss this change exists to stop. It now falls back on EACCES, EPERM and EROFS and lets anything else surface. Note EROFS is a plain OSError rather than a PermissionError, so catching PermissionError alone would miss a read only file system. Also removes the temporary directory the tests were leaving behind. --- src/azure-cli-core/azure/cli/core/_session.py | 13 ++++++--- .../azure/cli/core/tests/test_session.py | 27 +++++++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_session.py b/src/azure-cli-core/azure/cli/core/_session.py index a82736d0733..772ef6484a7 100644 --- a/src/azure-cli-core/azure/cli/core/_session.py +++ b/src/azure-cli-core/azure/cli/core/_session.py @@ -3,6 +3,7 @@ # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import errno import json import logging import os @@ -66,10 +67,14 @@ def save(self): try: fd, temp_name = tempfile.mkstemp(dir=directory, prefix=os.path.basename(target) + '.', suffix='.tmp') - except OSError: - # The directory is not writable, which happens in locked down containers and CI - # images. Writing in place still works there, so keep the old behaviour rather than - # failing a save that used to succeed. + except OSError as ex: + if ex.errno not in (errno.EACCES, errno.EPERM, errno.EROFS): + # Anything else, a full disk for instance, would also break the in place write and + # would lose the file doing it, so let it surface instead. + raise + # The directory is not writable but the file is, which happens in locked down + # containers and CI images. Writing in place still works there, so keep the old + # behaviour rather than failing a save that used to succeed. with open(self.filename, 'w', encoding=self._encoding) as f: json.dump(self.data, f) return diff --git a/src/azure-cli-core/azure/cli/core/tests/test_session.py b/src/azure-cli-core/azure/cli/core/tests/test_session.py index d29fa481895..3eb1a862855 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_session.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_session.py @@ -3,11 +3,14 @@ # Licensed under the MIT License. See License.txt in the project root for license information. # -------------------------------------------------------------------------------------------- +import errno import json import os +import shutil import stat import tempfile import unittest +from unittest import mock from azure.cli.core._session import Session @@ -18,6 +21,9 @@ def setUp(self): self.dir = tempfile.mkdtemp() self.filename = os.path.join(self.dir, 'test.json') + def tearDown(self): + shutil.rmtree(self.dir, ignore_errors=True) + def _session(self, data): session = Session() session.filename = self.filename @@ -97,6 +103,27 @@ def test_save_still_writes_when_the_directory_is_not_writable(self): finally: os.chmod(self.dir, 0o700) + def test_save_does_not_fall_back_when_the_temporary_file_fails_for_another_reason(self): + # The in place write would also fail on a full disk, and it would lose the file doing it, + # so only a permission problem should reach the fallback. + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({'kept': True}, f) + + with mock.patch('tempfile.mkstemp', side_effect=OSError(errno.ENOSPC, 'No space left')): + with self.assertRaises(OSError): + self._session({'a': 1}).save() + + self.assertEqual(self._read(), {'kept': True}) + + def test_save_falls_back_on_a_read_only_file_system(self): + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({}, f) + + with mock.patch('tempfile.mkstemp', side_effect=OSError(errno.EROFS, 'Read-only file system')): + self._session({'a': 1}).save() + + self.assertEqual(self._read(), {'a': 1}) + def test_load_reads_back_what_save_wrote(self): self._session({'a': 1}).save() From d153d0e61d73280e6af6662f331a66ec4f81e3eb Mon Sep 17 00:00:00 2001 From: Om Singhal Date: Thu, 10 Sep 2026 21:36:29 -0400 Subject: [PATCH 3/4] Drop EROFS from the fallback and guard the POSIX tests on Windows EROFS does not belong in the fallback. If the file system is read only then the in place write cannot succeed either, so catching it only delays the same error while implying a recovery that does not exist. The real case is a directory that denies writes while the file itself is writable, which is EACCES or EPERM, and test_save_still_writes_when_the_directory_is_not_writable already covers it with a real 0500 directory rather than a mock. The permission and symlink tests assert POSIX behaviour, so they are skipped on Windows the way test_azlogging and the acs tests already do. os.symlink exists there but creating one needs a privilege, so hasattr was the wrong guard. --- src/azure-cli-core/azure/cli/core/_session.py | 10 +++++----- .../azure/cli/core/tests/test_session.py | 12 ++---------- 2 files changed, 7 insertions(+), 15 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_session.py b/src/azure-cli-core/azure/cli/core/_session.py index 772ef6484a7..adb8faabfbb 100644 --- a/src/azure-cli-core/azure/cli/core/_session.py +++ b/src/azure-cli-core/azure/cli/core/_session.py @@ -68,12 +68,12 @@ def save(self): fd, temp_name = tempfile.mkstemp(dir=directory, prefix=os.path.basename(target) + '.', suffix='.tmp') except OSError as ex: - if ex.errno not in (errno.EACCES, errno.EPERM, errno.EROFS): - # Anything else, a full disk for instance, would also break the in place write and - # would lose the file doing it, so let it surface instead. + if ex.errno not in (errno.EACCES, errno.EPERM): + # Anything else, a full disk or a read only file system, would break the in place + # write too, so there is nothing to fall back to and it should surface. raise - # The directory is not writable but the file is, which happens in locked down - # containers and CI images. Writing in place still works there, so keep the old + # The directory denies writes but the file itself is writable, which happens in locked + # down containers and CI images. Writing in place still works there, so keep the old # behaviour rather than failing a save that used to succeed. with open(self.filename, 'w', encoding=self._encoding) as f: json.dump(self.data, f) diff --git a/src/azure-cli-core/azure/cli/core/tests/test_session.py b/src/azure-cli-core/azure/cli/core/tests/test_session.py index 3eb1a862855..44888ccd763 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_session.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_session.py @@ -64,6 +64,7 @@ def test_save_leaves_the_file_intact_when_the_data_cannot_be_serialized(self): self.assertEqual(self._read(), {'kept': True}) self.assertFalse(self._temp_files()) + @unittest.skipIf(os.name == 'nt', 'POSIX file permissions not applicable on Windows') def test_save_preserves_the_permissions_of_an_existing_file(self): with open(self.filename, 'w', encoding='utf-8-sig') as f: json.dump({}, f) @@ -73,7 +74,7 @@ def test_save_preserves_the_permissions_of_an_existing_file(self): self.assertEqual(stat.S_IMODE(os.stat(self.filename).st_mode), 0o644) - @unittest.skipUnless(hasattr(os, 'symlink'), 'requires symlink support') + @unittest.skipIf(os.name == 'nt', 'Symlink test not applicable on Windows') def test_save_follows_a_symlink_rather_than_replacing_it(self): target = os.path.join(self.dir, 'target.json') link = os.path.join(self.dir, 'link.json') @@ -115,15 +116,6 @@ def test_save_does_not_fall_back_when_the_temporary_file_fails_for_another_reaso self.assertEqual(self._read(), {'kept': True}) - def test_save_falls_back_on_a_read_only_file_system(self): - with open(self.filename, 'w', encoding='utf-8-sig') as f: - json.dump({}, f) - - with mock.patch('tempfile.mkstemp', side_effect=OSError(errno.EROFS, 'Read-only file system')): - self._session({'a': 1}).save() - - self.assertEqual(self._read(), {'a': 1}) - def test_load_reads_back_what_save_wrote(self): self._session({'a': 1}).save() From adf242c8df6538f5ccf97fd9a72e13b98ea06174 Mon Sep 17 00:00:00 2001 From: Om Singhal Date: Thu, 10 Sep 2026 21:44:40 -0400 Subject: [PATCH 4/4] Keep the same guarantee on the fallback path The fallback opened the live file and then serialized into it, so data that failed to serialize partway left the file truncated. That is the failure this change exists to remove, still present on the one path that cannot use a temporary file. It now builds the document first and only opens the target once there is something complete to write. The read only directory test relied on the directory mode stopping mkstemp, which root ignores, so on a privileged runner it silently took the atomic path instead. It is skipped for root now, and a mocked EACCES test covers the fallback everywhere. --- src/azure-cli-core/azure/cli/core/_session.py | 6 +++-- .../azure/cli/core/tests/test_session.py | 23 +++++++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/src/azure-cli-core/azure/cli/core/_session.py b/src/azure-cli-core/azure/cli/core/_session.py index adb8faabfbb..4d8dd9c4e40 100644 --- a/src/azure-cli-core/azure/cli/core/_session.py +++ b/src/azure-cli-core/azure/cli/core/_session.py @@ -74,9 +74,11 @@ def save(self): raise # The directory denies writes but the file itself is writable, which happens in locked # down containers and CI images. Writing in place still works there, so keep the old - # behaviour rather than failing a save that used to succeed. + # behaviour rather than failing a save that used to succeed. Serialize first so that + # unserializable data raises before the file is opened and truncated. + content = json.dumps(self.data) with open(self.filename, 'w', encoding=self._encoding) as f: - json.dump(self.data, f) + f.write(content) return try: diff --git a/src/azure-cli-core/azure/cli/core/tests/test_session.py b/src/azure-cli-core/azure/cli/core/tests/test_session.py index 44888ccd763..b75914fd94e 100644 --- a/src/azure-cli-core/azure/cli/core/tests/test_session.py +++ b/src/azure-cli-core/azure/cli/core/tests/test_session.py @@ -92,6 +92,7 @@ def test_save_follows_a_symlink_rather_than_replacing_it(self): self.assertEqual(json.load(f), {'a': 2}) @unittest.skipIf(os.name == 'nt', 'directory permissions are not enforced the same way on Windows') + @unittest.skipIf(hasattr(os, 'geteuid') and os.geteuid() == 0, 'root ignores the directory mode') def test_save_still_writes_when_the_directory_is_not_writable(self): # Locked down containers and build images mount the config directory read only while # leaving the file itself writable. A save that used to succeed there must keep working. @@ -116,6 +117,28 @@ def test_save_does_not_fall_back_when_the_temporary_file_fails_for_another_reaso self.assertEqual(self._read(), {'kept': True}) + def test_save_falls_back_without_losing_the_file_when_the_data_is_bad(self): + # The fallback must keep the same guarantee as the atomic path: data that cannot be + # serialized has to fail before the target is opened. + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({'kept': True}, f) + + with mock.patch('tempfile.mkstemp', side_effect=OSError(errno.EACCES, 'Permission denied')): + with self.assertRaises(TypeError): + self._session({'a': 'fine', 'b': {1, 2}}).save() + + self.assertEqual(self._read(), {'kept': True}) + + def test_save_uses_the_fallback_when_the_directory_denies_permission(self): + # Runs everywhere, including as root where the directory mode would not stop mkstemp. + with open(self.filename, 'w', encoding='utf-8-sig') as f: + json.dump({}, f) + + with mock.patch('tempfile.mkstemp', side_effect=OSError(errno.EACCES, 'Permission denied')): + self._session({'a': 1}).save() + + self.assertEqual(self._read(), {'a': 1}) + def test_load_reads_back_what_save_wrote(self): self._session({'a': 1}).save()