From 7616597edd34ef7267bf1131ec314355f900209f Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Mon, 22 Sep 2025 18:06:15 -0400 Subject: [PATCH 1/6] Test --- botocore/utils.py | 35 +++++++++++++--- tests/unit/test_utils.py | 86 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+), 5 deletions(-) diff --git a/botocore/utils.py b/botocore/utils.py index 3b5d9e7a20..da442be62d 100644 --- a/botocore/utils.py +++ b/botocore/utils.py @@ -22,6 +22,7 @@ import random import re import socket +import tempfile import time import warnings import weakref @@ -3577,11 +3578,35 @@ def __setitem__(self, cache_key, value): ) if not os.path.isdir(self._working_dir): os.makedirs(self._working_dir, exist_ok=True) - with os.fdopen( - os.open(full_key, os.O_WRONLY | os.O_CREAT, 0o600), 'w' - ) as f: - f.truncate() - f.write(file_content) + try: + temp_fd, temp_path = tempfile.mkstemp( + dir=self._working_dir, suffix='.tmp' + ) + if os.name == 'posix': + os.fchmod(temp_fd, 0o600) + else: + os.chmod(temp_fd, 0o600) + with os.fdopen(temp_fd, 'w') as f: + temp_fd = None + f.write(file_content) + f.flush() + os.fsync(f.fileno()) + + os.replace(temp_path, full_key) + temp_path = None + + except Exception: + if temp_fd is not None: + try: + os.close(temp_fd) + except OSError: + pass + if temp_path is not None and os.path.exists(temp_path): + try: + os.unlink(temp_path) + except OSError: + pass + raise def _convert_cache_key(self, cache_key): full_path = os.path.join(self._working_dir, cache_key + '.json') diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index ad37324ad4..ba2eb9915f 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -14,6 +14,9 @@ import datetime import io import operator +import os +import shutil +import tempfile from contextlib import contextmanager from sys import getrefcount @@ -59,6 +62,7 @@ InstanceMetadataFetcher, InstanceMetadataRegionFetcher, InvalidArnException, + JSONFileCache, S3ArnParamHandler, S3EndpointSetter, S3RegionRedirectorv2, @@ -3679,3 +3683,85 @@ def test_get_token_from_environment_returns_none( ): monkeypatch.delenv(env_var, raising=False) assert get_token_from_environment(signing_name) is None + + +class TestJSONFileCacheAtomicWrites(unittest.TestCase): + """Test atomic write operations in JSONFileCache.""" + + def setUp(self): + self.temp_dir = tempfile.mkdtemp() + self.cache = JSONFileCache(working_dir=self.temp_dir) + + def tearDown(self): + shutil.rmtree(self.temp_dir, ignore_errors=True) + + @mock.patch('os.replace') + def test_uses_tempfile_and_replace_for_atomic_write(self, mock_replace): + self.cache['test_key'] = {'data': 'test_value'} + mock_replace.assert_called_once() + + call_args = mock_replace.call_args[0] + temp_path = call_args[0] + # final_path = call_args[1] + + assert '.tmp' in temp_path + + def test_concurrent_writes_same_key(self): + """Test concurrent writes to same key don't cause corruption.""" + import threading + + key = 'concurrent_test' + errors = [] + + def write_worker(thread_id): + try: + for i in range(3): + self.cache[key] = {'thread': thread_id, 'iteration': i} + except Exception as e: + errors.append(f'Thread {thread_id}: {e}') + + threads = [ + threading.Thread(target=write_worker, args=(i,)) for i in range(3) + ] + + for thread in threads: + thread.start() + for thread in threads: + thread.join() + + self.assertEqual(len(errors), 0, f'Concurrent write errors: {errors}') + + # Verify final data is valid + final_data = self.cache[key] + self.assertIsInstance(final_data, dict) + self.assertIn('thread', final_data) + + def test_atomic_write_preserves_data_on_failure(self): + """Test write failures don't corrupt existing data.""" + key = 'atomic_test' + original_data = {'status': 'original'} + + self.cache[key] = original_data + + # Mock write failure + original_dumps = self.cache._dumps + self.cache._dumps = mock.Mock(side_effect=ValueError('Write failed')) + + with self.assertRaises(ValueError): + self.cache[key] = {'status': 'should_fail'} + + self.cache._dumps = original_dumps + + # Verify original data intact + self.assertEqual(self.cache[key], original_data) + + def test_no_temp_files_after_write(self): + """Test temporary files cleaned up after writes.""" + self.cache['test'] = {'data': 'value'} + + temp_files = [ + f for f in os.listdir(self.temp_dir) if f.endswith('.tmp') + ] + self.assertEqual( + len(temp_files), 0, f'Temp files not cleaned: {temp_files}' + ) From 8adc1bee1cfb6d0e313d8a9e932802e003a6ce3c Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Mon, 22 Sep 2025 18:45:36 -0400 Subject: [PATCH 2/6] Test 1 --- botocore/utils.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/botocore/utils.py b/botocore/utils.py index da442be62d..10cffa6bb0 100644 --- a/botocore/utils.py +++ b/botocore/utils.py @@ -22,6 +22,7 @@ import random import re import socket +import stat import tempfile import time import warnings @@ -3585,7 +3586,7 @@ def __setitem__(self, cache_key, value): if os.name == 'posix': os.fchmod(temp_fd, 0o600) else: - os.chmod(temp_fd, 0o600) + os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) with os.fdopen(temp_fd, 'w') as f: temp_fd = None f.write(file_content) From 1d6fb7563b486d58b81e9b6b0901599722b7d833 Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Wed, 24 Sep 2025 11:58:28 -0400 Subject: [PATCH 3/6] test two --- botocore/utils.py | 8 +++++--- tests/unit/test_utils.py | 6 ++++-- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/botocore/utils.py b/botocore/utils.py index 10cffa6bb0..7a5993a9b3 100644 --- a/botocore/utils.py +++ b/botocore/utils.py @@ -3583,9 +3583,11 @@ def __setitem__(self, cache_key, value): temp_fd, temp_path = tempfile.mkstemp( dir=self._working_dir, suffix='.tmp' ) - if os.name == 'posix': - os.fchmod(temp_fd, 0o600) - else: + # if os.name == 'posix': + # os.fchmod(temp_fd, 0o600) + # else: + # os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) + if os.chmod: os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) with os.fdopen(temp_fd, 'w') as f: temp_fd = None diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index ba2eb9915f..24ef1da883 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -3714,9 +3714,11 @@ def test_concurrent_writes_same_key(self): errors = [] def write_worker(thread_id): + lock = threading.Lock() try: - for i in range(3): - self.cache[key] = {'thread': thread_id, 'iteration': i} + with lock: + for i in range(3): + self.cache[key] = {'thread': thread_id, 'iteration': i} except Exception as e: errors.append(f'Thread {thread_id}: {e}') From 811cbd72dc692cc34cd0024fce0aceea8dac74da Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Wed, 24 Sep 2025 13:55:06 -0400 Subject: [PATCH 4/6] test 3 --- tests/unit/test_utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 24ef1da883..097fcbeecd 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -3712,9 +3712,9 @@ def test_concurrent_writes_same_key(self): key = 'concurrent_test' errors = [] + lock = threading.Lock() def write_worker(thread_id): - lock = threading.Lock() try: with lock: for i in range(3): From fce4c0a51cf5e5a0c8dce810e6f10bd0546a2b68 Mon Sep 17 00:00:00 2001 From: ubaskota <19787410+ubaskota@users.noreply.github.com> Date: Fri, 10 Oct 2025 18:41:12 -0400 Subject: [PATCH 5/6] test 3 --- botocore/utils.py | 7 ++++--- tests/unit/test_utils.py | 21 +++++++++++---------- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/botocore/utils.py b/botocore/utils.py index 7a5993a9b3..b018629692 100644 --- a/botocore/utils.py +++ b/botocore/utils.py @@ -22,7 +22,6 @@ import random import re import socket -import stat import tempfile import time import warnings @@ -3587,8 +3586,10 @@ def __setitem__(self, cache_key, value): # os.fchmod(temp_fd, 0o600) # else: # os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) - if os.chmod: - os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) + # if os.chmod: + # os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) + if hasattr(os, 'fchmod'): + os.fchmod(temp_fd, 0o600) with os.fdopen(temp_fd, 'w') as f: temp_fd = None f.write(file_content) diff --git a/tests/unit/test_utils.py b/tests/unit/test_utils.py index 097fcbeecd..1ec33c58d1 100644 --- a/tests/unit/test_utils.py +++ b/tests/unit/test_utils.py @@ -3706,19 +3706,17 @@ def test_uses_tempfile_and_replace_for_atomic_write(self, mock_replace): assert '.tmp' in temp_path - def test_concurrent_writes_same_key(self): + def test_concurrent_writes_to_multiple_temp_files(self): """Test concurrent writes to same key don't cause corruption.""" import threading - key = 'concurrent_test' errors = [] - lock = threading.Lock() def write_worker(thread_id): try: - with lock: - for i in range(3): - self.cache[key] = {'thread': thread_id, 'iteration': i} + key = f'concurrent_test_{thread_id}' + for i in range(3): + self.cache[key] = {'thread': thread_id, 'iteration': i} except Exception as e: errors.append(f'Thread {thread_id}: {e}') @@ -3733,10 +3731,13 @@ def write_worker(thread_id): self.assertEqual(len(errors), 0, f'Concurrent write errors: {errors}') - # Verify final data is valid - final_data = self.cache[key] - self.assertIsInstance(final_data, dict) - self.assertIn('thread', final_data) + for thread_id in range(3): + key = f'concurrent_test_{thread_id}' + final_data = self.cache[key] + self.assertIsInstance(final_data, dict) + self.assertEqual(final_data['thread'], thread_id) + self.assertIn('thread', final_data) + self.assertIn('iteration', final_data) def test_atomic_write_preserves_data_on_failure(self): """Test write failures don't corrupt existing data.""" From 234da9c7adfacc9480b712da168fd0a016dca749 Mon Sep 17 00:00:00 2001 From: Ujjwal <19787410+ubaskota@users.noreply.github.com> Date: Thu, 20 Nov 2025 20:36:05 -0500 Subject: [PATCH 6/6] Comment out fchmod usage in utils.py Comment out fchmod call to avoid permission issues. --- botocore/utils.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/botocore/utils.py b/botocore/utils.py index b018629692..30cc6013f0 100644 --- a/botocore/utils.py +++ b/botocore/utils.py @@ -3588,8 +3588,9 @@ def __setitem__(self, cache_key, value): # os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) # if os.chmod: # os.chmod(temp_path, stat.S_IREAD | stat.S_IWRITE) - if hasattr(os, 'fchmod'): - os.fchmod(temp_fd, 0o600) + + # if hasattr(os, 'fchmod'): + # os.fchmod(temp_fd, 0o600) with os.fdopen(temp_fd, 'w') as f: temp_fd = None f.write(file_content)