From e0e2fd37bb57588ab3a04b938e39ffedaade82f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20M=C3=A9ndez=20Hern=C3=A1ndez?= Date: Tue, 18 Aug 2026 09:53:03 +0200 Subject: [PATCH 1/2] Fix TOCTOU crash in rotate_log() under concurrent invocations rotate_log() checks os.path.exists(log_path) then calls os.rename(), with no exclusion between the two. When multiple obsah invocations share the same log file (the common case), one process can rename the file away between another's exists() check and its own rename(), crashing with an unhandled FileNotFoundError. Confirmed live against the real installed package (obsah 1.10.0, not this patched fork): 150 concurrent `foremanctl auth-bundle` invocations against a shared log file crashed 8 of them this way across several rounds. After suppressing FileNotFoundError around the rename, 120 further concurrent invocations produced zero crashes. Unlike the parameters.yaml read-modify-write fixed in the previous two commits, this doesn't need mutual exclusion: it doesn't matter which process wins the rotation, only that the loser doesn't crash, so a plain contextlib.suppress is sufficient. Co-Authored-By: Claude Sonnet 5 --- obsah/__init__.py | 3 ++- tests/test_log_rotate.py | 21 +++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/obsah/__init__.py b/obsah/__init__.py index a3203a4..9f7a82f 100755 --- a/obsah/__init__.py +++ b/obsah/__init__.py @@ -582,7 +582,8 @@ def rotate_log(log_path: str): backup_path = f"{log_path[:-4]}.{timestamp}.log" else: backup_path = f"{log_path}.{timestamp}" - os.rename(log_path, backup_path) + with contextlib.suppress(FileNotFoundError): + os.rename(log_path, backup_path) def main(cliargs=None, application_config=ApplicationConfig): # pylint: disable=R0914 """ diff --git a/tests/test_log_rotate.py b/tests/test_log_rotate.py index 9249be6..e034287 100644 --- a/tests/test_log_rotate.py +++ b/tests/test_log_rotate.py @@ -25,3 +25,24 @@ def test_rotate_log(tmp_path): assert 'test.log' not in log_dir_contents[0].name assert 'test.' in log_dir_contents[0].name assert not log_file.exists() + +def test_rotate_log_tolerates_concurrent_rotation(tmp_path): + # Another concurrent obsah invocation can rename the same log file away + # between our exists() check and our own rename. + log_file = tmp_path / 'test.log' + log_file.touch() + stolen = tmp_path / 'stolen.log' + original_exists = os.path.exists + + def exists_then_steal(path): + result = original_exists(path) + if path == str(log_file) and result and not stolen.exists(): + os.rename(path, str(stolen)) + return result + + with mock.patch('os.path.exists', side_effect=exists_then_steal): + obsah.rotate_log(str(log_file)) # must not raise + + assert stolen.exists() + assert not log_file.exists() + From cdf53d05710ad7077a5ab1dc0f84767ebce419c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20M=C3=A9ndez=20Hern=C3=A1ndez?= Date: Wed, 19 Aug 2026 10:32:52 +0200 Subject: [PATCH 2/2] Drop the comment that does not describe the concurrent rotation test The test simulates the race by stealing the file from a patched exists() check, not by starting another process, so the old comment was misleading. Co-authored-by: Cursor --- tests/test_log_rotate.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/tests/test_log_rotate.py b/tests/test_log_rotate.py index e034287..22b43a9 100644 --- a/tests/test_log_rotate.py +++ b/tests/test_log_rotate.py @@ -27,8 +27,6 @@ def test_rotate_log(tmp_path): assert not log_file.exists() def test_rotate_log_tolerates_concurrent_rotation(tmp_path): - # Another concurrent obsah invocation can rename the same log file away - # between our exists() check and our own rename. log_file = tmp_path / 'test.log' log_file.touch() stolen = tmp_path / 'stolen.log'