From cb6faf6828774634122eaf4ab2ac77208904e5ab Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Tue, 15 Feb 2022 12:58:37 +0100 Subject: [PATCH 1/3] safer truncate_and_unlink implementation the previous implementation caused collateral damage on hardlink-copies of a repository, see: https://github.com/borgbackup/borg/discussions/6286 --- src/borg/helpers.py | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/borg/helpers.py b/src/borg/helpers.py index f9080e0f0..7fe9c274f 100644 --- a/src/borg/helpers.py +++ b/src/borg/helpers.py @@ -2401,10 +2401,10 @@ def secure_erase(path): def truncate_and_unlink(path): """ - Truncate and then unlink *path*. + Safely unlink (delete) *path*. - Do not create *path* if it does not exist. - Open *path* for truncation in r+b mode (=O_RDWR|O_BINARY). + If we run out of space while deleting the file, we try truncating it first. + BUT we truncate only if path is the only hardlink referring to this content. Use this when deleting potentially large files when recovering from a VFS error such as ENOSPC. It can help a full file system @@ -2412,13 +2412,27 @@ def truncate_and_unlink(path): in repository.py for further explanations. """ try: - with open(path, 'r+b') as fd: - fd.truncate() - except OSError as err: - if err.errno != errno.ENOTSUP: + os.unlink(path) + except OSError as unlink_err: + if unlink_err.errno != errno.ENOSPC: + # not free space related, give up here. raise - # don't crash if the above ops are not supported. - os.unlink(path) + # we ran out of space while trying to delete the file. + st = os.stat(path) + if st.st_nlink > 1: + # rather give up here than cause collateral damage to the other hardlink. + raise + # no other hardlink! try to recover free space by truncating this file. + try: + # Do not create *path* if it does not exist, open for truncation in r+b mode (=O_RDWR|O_BINARY). + with open(path, 'r+b') as fd: + fd.truncate() + except OSError: + # truncate didn't work, so we still have the original unlink issue - give up: + raise unlink_err + else: + # successfully truncated the file, try again deleting it: + os.unlink(path) def popen_with_error_handling(cmd_line: str, log_prefix='', **kwargs): From 0adcbca6991481d1d30aec7389ce4ea6cc6fb2f9 Mon Sep 17 00:00:00 2001 From: Thomas Waldmann Date: Tue, 15 Feb 2022 19:39:58 +0100 Subject: [PATCH 2/3] rename truncate_and_unlink to safe_unlink it usually does not truncate any more, only under "disk full" circumstances and only if there is only one hardlink. --- src/borg/cache.py | 4 ++-- src/borg/helpers.py | 2 +- src/borg/platform/base.py | 6 +++--- src/borg/remote.py | 4 ++-- src/borg/repository.py | 6 +++--- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/borg/cache.py b/src/borg/cache.py index 282a2fc82..11ef36f69 100644 --- a/src/borg/cache.py +++ b/src/borg/cache.py @@ -25,7 +25,7 @@ from .helpers import yes, hostname_is_unique from .helpers import remove_surrogates from .helpers import ProgressIndicatorPercent, ProgressIndicatorMessage from .helpers import set_ec, EXIT_WARNING -from .helpers import truncate_and_unlink +from .helpers import safe_unlink from .helpers import msgpack from .item import ArchiveItem, ChunkListEntry from .crypto.key import PlaintextKey @@ -745,7 +745,7 @@ class LocalCache(CacheStatsMixin): filename=bin_to_hex(archive_id) + '.compact') as fd: chunk_idx.write(fd) except Exception: - truncate_and_unlink(fn_tmp) + safe_unlink(fn_tmp) else: os.rename(fn_tmp, fn) diff --git a/src/borg/helpers.py b/src/borg/helpers.py index 7fe9c274f..6b2e14c8d 100644 --- a/src/borg/helpers.py +++ b/src/borg/helpers.py @@ -2399,7 +2399,7 @@ def secure_erase(path): os.unlink(path) -def truncate_and_unlink(path): +def safe_unlink(path): """ Safely unlink (delete) *path*. diff --git a/src/borg/platform/base.py b/src/borg/platform/base.py index c0b990c9c..234b02426 100644 --- a/src/borg/platform/base.py +++ b/src/borg/platform/base.py @@ -3,7 +3,7 @@ import os import socket import uuid -from borg.helpers import truncate_and_unlink +from borg.helpers import safe_unlink """ platform base module @@ -168,7 +168,7 @@ class SaveFile: def __enter__(self): from .. import platform try: - truncate_and_unlink(self.tmppath) + safe_unlink(self.tmppath) except FileNotFoundError: pass self.fd = platform.SyncFile(self.tmppath, self.binary) @@ -178,7 +178,7 @@ class SaveFile: from .. import platform self.fd.close() if exc_type is not None: - truncate_and_unlink(self.tmppath) + safe_unlink(self.tmppath) return os.replace(self.tmppath, self.path) platform.sync_dir(os.path.dirname(self.path)) diff --git a/src/borg/remote.py b/src/borg/remote.py index 56ef54c26..c19531f3a 100644 --- a/src/borg/remote.py +++ b/src/borg/remote.py @@ -27,7 +27,7 @@ from .helpers import hostname_is_unique from .helpers import replace_placeholders from .helpers import sysinfo from .helpers import format_file_size -from .helpers import truncate_and_unlink +from .helpers import safe_unlink from .helpers import prepare_subprocess_env from .logger import create_logger, setup_logging from .helpers import msgpack @@ -1144,7 +1144,7 @@ class RepositoryCache(RepositoryNoCache): fd.write(packed) except OSError as os_error: try: - truncate_and_unlink(file) + safe_unlink(file) except FileNotFoundError: pass # open() could have failed as well if os_error.errno == errno.ENOSPC: diff --git a/src/borg/repository.py b/src/borg/repository.py index ca8faadfd..e5c09c3c5 100644 --- a/src/borg/repository.py +++ b/src/borg/repository.py @@ -19,7 +19,7 @@ from .helpers import Location from .helpers import ProgressIndicatorPercent from .helpers import bin_to_hex from .helpers import hostname_is_unique -from .helpers import secure_erase, truncate_and_unlink +from .helpers import secure_erase, safe_unlink from .helpers import msgpack from .locking import Lock, LockError, LockErrorT from .logger import create_logger @@ -1294,7 +1294,7 @@ class LoggedIO: if segment > transaction_id: if segment in self.fds: del self.fds[segment] - truncate_and_unlink(filename) + safe_unlink(filename) count += 1 else: break @@ -1402,7 +1402,7 @@ class LoggedIO: if segment in self.fds: del self.fds[segment] try: - truncate_and_unlink(self.segment_filename(segment)) + safe_unlink(self.segment_filename(segment)) except FileNotFoundError: pass From a26a6e80f25f451b8db6c56a4f89dc5c29125fb4 Mon Sep 17 00:00:00 2001 From: Andrey Bienkowski Date: Sat, 19 Feb 2022 14:54:20 +0300 Subject: [PATCH 3/3] Write a test for safe_unlink --- src/borg/testsuite/helpers.py | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/src/borg/testsuite/helpers.py b/src/borg/testsuite/helpers.py index 404f92bf4..fa4af1de1 100644 --- a/src/borg/testsuite/helpers.py +++ b/src/borg/testsuite/helpers.py @@ -1,3 +1,4 @@ +import errno import hashlib import io import os @@ -27,6 +28,7 @@ from ..helpers import chunkit from ..helpers import safe_ns, safe_s, SUPPORT_32BIT_PLATFORMS from ..helpers import popen_with_error_handling from ..helpers import dash_open +from ..helpers import safe_unlink from . import BaseTestCase, FakeInputs @@ -999,3 +1001,32 @@ def test_dash_open(): assert dash_open('-', 'w') is sys.stdout assert dash_open('-', 'rb') is sys.stdin.buffer assert dash_open('-', 'wb') is sys.stdout.buffer + + +def test_safe_unlink_is_safe(tmpdir): + contents = b"Hello, world\n" + victim = tmpdir / 'victim' + victim.write_binary(contents) + hard_link = tmpdir / 'hardlink' + hard_link.mklinkto(victim) + + safe_unlink(str(hard_link)) + + assert victim.read_binary() == contents + + +def test_safe_unlink_is_safe_ENOSPC(tmpdir, monkeypatch): + contents = b"Hello, world\n" + victim = tmpdir / 'victim' + victim.write_binary(contents) + hard_link = tmpdir / 'hardlink' + hard_link.mklinkto(victim) + + def os_unlink(_): + raise OSError(errno.ENOSPC, "Pretend that we ran out of space") + monkeypatch.setattr(os, "unlink", os_unlink) + + with pytest.raises(OSError): + safe_unlink(str(hard_link)) + + assert victim.read_binary() == contents