From ef19577dae726cc4239b58fa6af24c54d6308773 Mon Sep 17 00:00:00 2001 From: Tim Hatch Date: Wed, 8 May 2024 14:58:11 -0700 Subject: [PATCH 1/3] Attempt to overwrite without race condition As explained in #332, there was previously a small window of time where the file is deleted before its new contents get written. Because reads don't happen with the lock held, this resulted in an empty-body cache hit when it shouldn't. Fixes #332 --- cachecontrol/caches/file_cache.py | 55 +++++-------------------------- 1 file changed, 8 insertions(+), 47 deletions(-) diff --git a/cachecontrol/caches/file_cache.py b/cachecontrol/caches/file_cache.py index 0f941713..34ff67e5 100644 --- a/cachecontrol/caches/file_cache.py +++ b/cachecontrol/caches/file_cache.py @@ -5,6 +5,7 @@ import hashlib import os +import tempfile from textwrap import dedent from typing import IO, TYPE_CHECKING, Union from pathlib import Path @@ -18,47 +19,6 @@ from filelock import BaseFileLock -def _secure_open_write(filename: str, fmode: int) -> IO[bytes]: - # We only want to write to this file, so open it in write only mode - flags = os.O_WRONLY - - # os.O_CREAT | os.O_EXCL will fail if the file already exists, so we only - # will open *new* files. - # We specify this because we want to ensure that the mode we pass is the - # mode of the file. - flags |= os.O_CREAT | os.O_EXCL - - # Do not follow symlinks to prevent someone from making a symlink that - # we follow and insecurely open a cache file. - if hasattr(os, "O_NOFOLLOW"): - flags |= os.O_NOFOLLOW - - # On Windows we'll mark this file as binary - if hasattr(os, "O_BINARY"): - flags |= os.O_BINARY - - # Before we open our file, we want to delete any existing file that is - # there - try: - os.remove(filename) - except OSError: - # The file must not exist already, so we can just skip ahead to opening - pass - - # Open our file, the use of os.O_CREAT | os.O_EXCL will ensure that if a - # race condition happens between the os.remove and this line, that an - # error will be raised. Because we utilize a lockfile this should only - # happen if someone is attempting to attack us. - fd = os.open(filename, flags, fmode) - try: - return os.fdopen(fd, "wb") - - except: - # An error occurred wrapping our FD in a file object - os.close(fd) - raise - - class _FileCacheMixin: """Shared implementation for both FileCache variants.""" @@ -122,15 +82,16 @@ def _write(self, path: str, data: bytes) -> None: Safely write the data to the given path. """ # Make sure the directory exists - try: - os.makedirs(os.path.dirname(path), self.dirmode) - except OSError: - pass + dirname = os.path.dirname(path) + os.makedirs(dirname, self.dirmode, exist_ok=True) with self.lock_class(path + ".lock"): # Write our actual file - with _secure_open_write(path, self.filemode) as fh: - fh.write(data) + (fd, name) = tempfile.mkstemp(dir=dirname) + os.write(fd, data) + os.fchmod(fd, self.filemode) + os.close(fd) + os.replace(name, path) def _delete(self, key: str, suffix: str) -> None: name = self._fn(key) + suffix From 05e1119026bd525432ae56b332eaf6ab6b0485a4 Mon Sep 17 00:00:00 2001 From: Tim Hatch Date: Wed, 8 May 2024 15:22:20 -0700 Subject: [PATCH 2/3] Use chmod after close for windows compat --- cachecontrol/caches/file_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cachecontrol/caches/file_cache.py b/cachecontrol/caches/file_cache.py index 34ff67e5..5fc59524 100644 --- a/cachecontrol/caches/file_cache.py +++ b/cachecontrol/caches/file_cache.py @@ -89,8 +89,8 @@ def _write(self, path: str, data: bytes) -> None: # Write our actual file (fd, name) = tempfile.mkstemp(dir=dirname) os.write(fd, data) - os.fchmod(fd, self.filemode) os.close(fd) + os.chmod(name, self.filemode) os.replace(name, path) def _delete(self, key: str, suffix: str) -> None: From 51d816d561302a130bfb3bf883408663ce90ee9d Mon Sep 17 00:00:00 2001 From: William Woodruff Date: Sat, 9 Nov 2024 18:00:40 -0500 Subject: [PATCH 3/3] Update file_cache.py Co-authored-by: Thomas Grainger --- cachecontrol/caches/file_cache.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cachecontrol/caches/file_cache.py b/cachecontrol/caches/file_cache.py index 5fc59524..c311c36d 100644 --- a/cachecontrol/caches/file_cache.py +++ b/cachecontrol/caches/file_cache.py @@ -88,8 +88,10 @@ def _write(self, path: str, data: bytes) -> None: with self.lock_class(path + ".lock"): # Write our actual file (fd, name) = tempfile.mkstemp(dir=dirname) - os.write(fd, data) - os.close(fd) + try: + os.write(fd, data) + finally: + os.close(fd) os.chmod(name, self.filemode) os.replace(name, path)