From a653e216d907c3cba34d4c24373a07a6b2c89ca2 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Wed, 16 Feb 2022 23:13:24 +0100 Subject: [PATCH] SaveFile: fix race conditions Thanks to Andrey Bienkowski (@hexagonrecursion) for reporting this and writing reproducer code. Changes: - use different, randomly (but recognizably) named temp files while writing (securely made by os.mkstemp()) - make sure temp files are cleaned up in normal and error conditions - SyncFile can now get corresponding pair of path + open os-level fd - cleaned up: fd now means os-level fd, f means python-file-like object - fixed a caller of SaveFile --- src/borg/helpers.py | 10 +---- src/borg/platform/base.py | 74 +++++++++++++++++++++++-------------- src/borg/platform/linux.pyx | 18 ++++----- 3 files changed, 57 insertions(+), 45 deletions(-) diff --git a/src/borg/helpers.py b/src/borg/helpers.py index 6b2e14c8d..0736bce35 100644 --- a/src/borg/helpers.py +++ b/src/borg/helpers.py @@ -592,14 +592,8 @@ def get_cache_dir(): # http://www.bford.info/cachedir/spec.html """).encode('ascii') from .platform import SaveFile - try: - with SaveFile(cache_tag_fn, binary=True) as fd: - fd.write(cache_tag_contents) - except FileExistsError: - # if we have multiple SaveFile calls running in parallel for same cache_tag_fn, - # it is fine if just one (usually first/quicker one) of them run gets through - # and all others raise FileExistsError. - pass + with SaveFile(cache_tag_fn, binary=True) as fd: + fd.write(cache_tag_contents) return cache_dir diff --git a/src/borg/platform/base.py b/src/borg/platform/base.py index 234b02426..112c33742 100644 --- a/src/borg/platform/base.py +++ b/src/borg/platform/base.py @@ -1,6 +1,7 @@ import errno import os import socket +import tempfile import uuid from borg.helpers import safe_unlink @@ -103,10 +104,22 @@ class SyncFile: TODO: A Windows implementation should use CreateFile with FILE_FLAG_WRITE_THROUGH. """ - def __init__(self, path, binary=False): + def __init__(self, path, *, fd=None, binary=False): + """ + Open a SyncFile. + + :param path: full path/filename + :param fd: additionally to path, it is possible to give an already open OS-level fd + that corresponds to path (like from os.open(path, ...) or os.mkstemp(...)) + :param binary: whether to open in binary mode, default is False. + """ mode = 'xb' if binary else 'x' # x -> raise FileExists exception in open() if file exists already - self.fd = open(path, mode) - self.fileno = self.fd.fileno() + self.path = path + if fd is None: + self.f = open(path, mode=mode) # python file object + else: + self.f = os.fdopen(fd, mode=mode) + self.fd = self.f.fileno() # OS-level fd def __enter__(self): return self @@ -115,7 +128,7 @@ class SyncFile: self.close() def write(self, data): - self.fd.write(data) + self.f.write(data) def sync(self): """ @@ -123,21 +136,21 @@ class SyncFile: after sync(). """ from .. import platform - self.fd.flush() - platform.fdatasync(self.fileno) + self.f.flush() + platform.fdatasync(self.fd) # tell the OS that it does not need to cache what we just wrote, # avoids spoiling the cache for the OS and other processes. - safe_fadvise(self.fileno, 0, 0, 'DONTNEED') + safe_fadvise(self.fd, 0, 0, 'DONTNEED') def close(self): """sync() and close.""" from .. import platform dirname = None try: - dirname = os.path.dirname(self.fd.name) + dirname = os.path.dirname(self.path) self.sync() finally: - self.fd.close() + self.f.close() if dirname: platform.sync_dir(dirname) @@ -152,36 +165,41 @@ class SaveFile: atomically and won't become corrupted, even on power failures or crashes (for caveats see SyncFile). - Calling SaveFile(path) in parallel for same path is safe (even when using the same SUFFIX), but the - caller needs to catch potential FileExistsError exceptions that may happen in this racy situation. - The caller executing SaveFile->SyncFile->open() first will win. - All other callers will raise a FileExistsError in open(), at least until the os.replace is executed. + SaveFile can safely by used in parallel (e.g. by multiple processes) to write + to the same target path. Whatever writer finishes last (executes the os.replace + last) "wins" and has successfully written its content to the target path. + Internally used temporary files are created in the target directory and are + named -.tmp and cleaned up in normal and error conditions. """ - - SUFFIX = '.tmp' - def __init__(self, path, binary=False): self.binary = binary self.path = path - self.tmppath = self.path + self.SUFFIX + self.dir = os.path.dirname(path) + self.tmp_prefix = os.path.basename(path) + '-' + self.tmp_fd = None # OS-level fd + self.tmp_fname = None # full path/filename corresponding to self.tmp_fd + self.f = None # python-file-like SyncFile def __enter__(self): from .. import platform - try: - safe_unlink(self.tmppath) - except FileNotFoundError: - pass - self.fd = platform.SyncFile(self.tmppath, self.binary) - return self.fd + self.tmp_fd, self.tmp_fname = tempfile.mkstemp(prefix=self.tmp_prefix, suffix='.tmp', dir=self.dir) + self.f = platform.SyncFile(self.tmp_fname, fd=self.tmp_fd, binary=self.binary) + return self.f def __exit__(self, exc_type, exc_val, exc_tb): from .. import platform - self.fd.close() + self.f.close() # this indirectly also closes self.tmp_fd + self.tmp_fd = None if exc_type is not None: - safe_unlink(self.tmppath) - return - os.replace(self.tmppath, self.path) - platform.sync_dir(os.path.dirname(self.path)) + safe_unlink(self.tmp_fname) # with-body has failed, clean up tmp file + return # continue processing the exception normally + try: + os.replace(self.tmp_fname, self.path) # POSIX: atomic rename + except OSError: + safe_unlink(self.tmp_fname) # rename has failed, clean up tmp file + raise + finally: + platform.sync_dir(self.dir) def swidth(s): diff --git a/src/borg/platform/linux.pyx b/src/borg/platform/linux.pyx index 4a5b27513..6e582275c 100644 --- a/src/borg/platform/linux.pyx +++ b/src/borg/platform/linux.pyx @@ -256,28 +256,28 @@ else: disk in the immediate future. """ - def __init__(self, path, binary=False): - super().__init__(path, binary) + def __init__(self, path, *, fd=None, binary=False): + super().__init__(path, fd=fd, binary=binary) self.offset = 0 self.write_window = (16 * 1024 ** 2) & ~PAGE_MASK self.last_sync = 0 self.pending_sync = None def write(self, data): - self.offset += self.fd.write(data) + self.offset += self.f.write(data) offset = self.offset & ~PAGE_MASK if offset >= self.last_sync + self.write_window: - self.fd.flush() - _sync_file_range(self.fileno, self.last_sync, offset - self.last_sync, SYNC_FILE_RANGE_WRITE) + self.f.flush() + _sync_file_range(self.fd, self.last_sync, offset - self.last_sync, SYNC_FILE_RANGE_WRITE) if self.pending_sync is not None: - _sync_file_range(self.fileno, self.pending_sync, self.last_sync - self.pending_sync, + _sync_file_range(self.fd, self.pending_sync, self.last_sync - self.pending_sync, SYNC_FILE_RANGE_WRITE | SYNC_FILE_RANGE_WAIT_BEFORE | SYNC_FILE_RANGE_WAIT_AFTER) self.pending_sync = self.last_sync self.last_sync = offset def sync(self): - self.fd.flush() - os.fdatasync(self.fileno) + self.f.flush() + os.fdatasync(self.fd) # tell the OS that it does not need to cache what we just wrote, # avoids spoiling the cache for the OS and other processes. - safe_fadvise(self.fileno, 0, 0, 'DONTNEED') + safe_fadvise(self.fd, 0, 0, 'DONTNEED')